Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential Memory Leak in string_configuration_get Due to Ownership Mismatch #177

Open
dgershko opened this issue Feb 18, 2025 · 4 comments

Comments

@dgershko
Copy link

There seems to be a mismatch in ownership semantics between valkeymodule-rs and the Valkey C API when handling string config getters, which may be causing a memory leak.

Problem:

  • The Rust function string_configuration_get returns a *mut RedisModuleString using .take(), which appears to transfer ownership.
  • The C function getModuleStringConfig calls this getter and duplicates the string (sdsdup(val->ptr)), assuming the original remains valid.
  • This is done on purpose, as Valkey’s documentation states that "On string config gets, the string will not be consumed and will be valid after execution."
  • Since the C code does not free the original, and Rust seems to allocate a new copy, this could lead to a memory leak.

Reproduction Steps:

  1. Compile Valkey with AddressSanitizer (ASan).
  2. Run Valkey with a module that registers string configs (e.g., valkey-bloom).
  3. In the CLI, run CONFIG GET bf.bloom-fp-rate (or another registered config).
  4. Stop the server – ASan should report a memory leak.

Additional Context:

I'm not a Rust expert, but based on my understanding, .take() combined with ConfigurationContext::new() might be the cause.

extern "C" fn string_configuration_get<T: ConfigurationValue<ValkeyString> + 'static>(

@zackcam
Copy link
Contributor

zackcam commented Feb 21, 2025

Hi,
I took a look at recreating this and wasn't able to find a memory leak. Let me know if I compiled Valkey in a different way than you
Commmand used to compile the valkey server:
make -j4 SANITIZER=address SERVER_CFLAGS='-Werror' BUILD_TLS=module
I then ran this to load the module:
./src/valkey-server --loadmodule ../valkey-bloom/target/release/libvalkey_bloom.so
Finally i did step 3 and 4 above:

Image
I don't see any memory leak messages let me know if I should try and recreate this in a different way

@dgershko
Copy link
Author

Hi Zack!
Please try the following:

make distclean
make -j4 SANITIZER=address
./src/valkey-server --loadmodule ../valkey-bloom/target/release/libvalkey_bloom.so

here's the exact error I got when compiling like this and calling the CONFIG GET:

20781:M 23 Feb 2025 12:00:58.571 # Valkey is now ready to exit, bye bye...

=================================================================
==20781==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x7f87501e657d in malloc (/lib64/libasan.so.4+0xd857d)
    #1 0x5947a6 in ztrymalloc_usable_internal /home/dgershko/workplace/valkey/src/zmalloc.c:155
    #2 0x5947a6 in zmalloc_usable /home/dgershko/workplace/valkey/src/zmalloc.c:199
    #3 0x76a130 in createEmbeddedStringObjectWithKeyAndExpire.constprop.226 /home/dgershko/workplace/valkey/src/object.c:167
    #4 0x55e873 in VM_CreateStringFromString /home/dgershko/workplace/valkey/src/module.c:2794
    #5 0x7f874557bf07 in _$LT$valkey_module..redismodule..ValkeyString$u20$as$u20$core..clone..Clone$GT$::clone::hf82e8ed8ededebd7 (../valkey-bloom/target/release/libvalkey_bloom.so+0x2df07)
    #6 0x70dce0 in configGetCommand /home/dgershko/workplace/valkey/src/config.c:994
    #7 0x53e634 in call /home/dgershko/workplace/valkey/src/server.c:3735
    #8 0x54062c in processCommand /home/dgershko/workplace/valkey/src/server.c:4358
    #9 0x61b667 in processCommandAndResetClient /home/dgershko/workplace/valkey/src/networking.c:3044
    #10 0x61b667 in processInputBuffer /home/dgershko/workplace/valkey/src/networking.c:3172
    #11 0x62363a in readQueryFromClient /home/dgershko/workplace/valkey/src/networking.c:3277
    #12 0x634615 in callHandler /home/dgershko/workplace/valkey/src/connhelpers.h:79
    #13 0x634615 in connSocketEventHandler.lto_priv.378 /home/dgershko/workplace/valkey/src/socket.c:300
    #14 0x47a833 in aeProcessEvents /home/dgershko/workplace/valkey/src/ae.c:486
    #15 0x47b5c4 in aeMain /home/dgershko/workplace/valkey/src/ae.c:543
    #16 0x452960 in main /home/dgershko/workplace/valkey/src/server.c:7211
    #17 0x7f874f418139 in __libc_start_main (/lib64/libc.so.6+0x21139)

SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).

@zackcam
Copy link
Contributor

zackcam commented Feb 24, 2025

Thanks for giving the commands you used! Trying that returned the memory leak you showed I will now take a look at fixing it.

@KarthikSubbarao
Copy link
Member

We found the source of the leak in the SDK's config interface and Cameron is working on the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants