-
Notifications
You must be signed in to change notification settings - Fork 24.4k
Fix crash in lookupKey() when executing_client is NULL
#14415
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
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
|
Why can't I see the test crashes when i reverted the code in db.c? |
|
@sundb , it is reproduced easily on my machine. just by running the test with previous code:
|
sundb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moticless I updated the top comment. Please confirm whether the version of backport is correct.
|
@sundb , |
|
@moticless I can reproduce it now, but I need to use tricks to avoid the compiler optimizing this code or use |
|
Verified it is reproduced at 8.0 and 8.2. Thanks. |
This PR is based on:
valkey-io/valkey#2347
This was introduced in #13512
The server crashes with a null pointer dereference when lookupKey() is called from handleClientsBlockedOnKey(). The crash occurs because server.executing_client is NULL, but the code attempts to access server.executing_client->cmd->proc without checking.
Crash scenario:
Client 1 enables CLIENT NO-TOUCH
Client 2 blocks on BRPOP mylist 0
Client 1 executes RPUSH mylist elem
When unblocking Client 2, lookupKey() dereferences NULL server.executing_client → crash
Solution
Added proper null checks before dereferencing server.executing_client:
Check if LOOKUP_NOTOUCH flag is already set before attempting to modify it
Verify both server.current_client and server.executing_client are not NULL before accessing their members
Maintain the TOUCH command exception for scripts
Testing
Added regression test in tests/unit/type/list.tcl that reproduces and verifies the fix for this crash scenario.
This fix is based on valkey-io/valkey#2347
Co-authored-by: Uri Yagelnik uriy@amazon.com
Co-authored-by: Ran Shidlansik ranshid@amazon.com