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

Fix module / script call CLUSTER SLOTS / SHARDS fake client check crash #1063

Merged
merged 7 commits into from
Sep 25, 2024

Conversation

enjoy-binbin
Copy link
Member

@enjoy-binbin enjoy-binbin commented Sep 23, 2024

The reason is VM_Call will use a fake client without connection,
so we also need to check if c->conn is NULL.

This also affects scripts. If they are called in the script, the
server will crash. Injecting commands into AOF will also cause
startup failure.

Fixes #1054.

The reason is VM_Call will use a fake client without connection,
so we also need to check if c->conn is NULL.

Fixes valkey-io#1054.

Signed-off-by: Binbin <[email protected]>
@enjoy-binbin enjoy-binbin requested review from zuiderkwast and PingXie and removed request for zuiderkwast September 23, 2024 03:35
@enjoy-binbin enjoy-binbin added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Sep 23, 2024
Signed-off-by: Binbin <[email protected]>
Copy link

codecov bot commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.62%. Comparing base (af81174) to head (18e71a1).
Report is 2 commits behind head on unstable.

Files with missing lines Patch % Lines
src/module.c 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1063      +/-   ##
============================================
+ Coverage     70.46%   70.62%   +0.15%     
============================================
  Files           114      114              
  Lines         61688    61694       +6     
============================================
+ Hits          43467    43569     +102     
+ Misses        18221    18125      -96     
Files with missing lines Coverage Δ
src/aof.c 80.08% <100.00%> (+0.01%) ⬆️
src/eval.c 56.59% <100.00%> (+0.04%) ⬆️
src/functions.c 95.59% <100.00%> (+<0.01%) ⬆️
src/networking.c 87.99% <100.00%> (-0.43%) ⬇️
src/server.h 100.00% <ø> (ø)
src/module.c 9.64% <0.00%> (-0.01%) ⬇️

... and 10 files with indirect coverage changes

Signed-off-by: Binbin <[email protected]>
Copy link
Member

@PingXie PingXie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @enjoy-binbin!

src/networking.c Outdated Show resolved Hide resolved
Signed-off-by: Binbin <[email protected]>
Signed-off-by: Binbin <[email protected]>
@enjoy-binbin enjoy-binbin changed the title Fix module call CLUSTER SLOTS / SHARDS fake client check crash Fix module / script call CLUSTER SLOTS / SHARDS fake client check crash Sep 23, 2024
Copy link
Member

@PingXie PingXie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @enjoy-binbin!

src/networking.c Show resolved Hide resolved
@enjoy-binbin enjoy-binbin merged commit 80fcbd3 into valkey-io:unstable Sep 25, 2024
56 checks passed
@enjoy-binbin enjoy-binbin deleted the fix_module_call branch September 25, 2024 06:51
@enjoy-binbin enjoy-binbin added the release-notes This issue should get a line item in the release notes label Sep 25, 2024
madolson pushed a commit to madolson/valkey that referenced this pull request Sep 30, 2024
…sh (valkey-io#1063)

The reason is VM_Call will use a fake client without connection,
so we also need to check if c->conn is NULL.

This also affects scripts. If they are called in the script, the
server will crash. Injecting commands into AOF will also cause
startup failure.

Fixes valkey-io#1054.

Signed-off-by: Binbin <[email protected]>
naglera pushed a commit to naglera/placeholderkv that referenced this pull request Oct 10, 2024
…sh (valkey-io#1063)

The reason is VM_Call will use a fake client without connection,
so we also need to check if c->conn is NULL.

This also affects scripts. If they are called in the script, the
server will crash. Injecting commands into AOF will also cause
startup failure.

Fixes valkey-io#1054.

Signed-off-by: Binbin <[email protected]>
Signed-off-by: naglera <[email protected]>
enjoy-binbin added a commit to enjoy-binbin/valkey that referenced this pull request Oct 21, 2024
The fake client flag was introduced in valkey-io#1063, we want
this to replace all !conn fake client checks.

Signed-off-by: Binbin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes This issue should get a line item in the release notes run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[CRASH] When loading module-oss (RediSearch with Coordinator), Valkey Node crashes
3 participants