Skip to content

Fix potential vulnerable cloned function #2253

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

Merged
merged 2 commits into from
May 26, 2025

Conversation

npt-1707
Copy link
Contributor

@npt-1707 npt-1707 commented May 21, 2025

Hi Development Team,

I identified a potential vulnerability in a clone function sdsMakeRoomFor()
in src/shell/sds/sds.c sourced from redis/redis.
This issue, originally reported in CVE-2021-41099,
was resolved in the repository via this commit redis/redis@c6ad876.

This PR applies the corresponding patch to fix the vulnerability in this codebase.

Please review at your convenience. Thank you!

acelyc111
acelyc111 previously approved these changes May 22, 2025
Copy link
Member

@acelyc111 acelyc111 left a comment

Choose a reason for hiding this comment

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

@npt-1707 Thanks for the contribution!

@empiredan
Copy link
Contributor

empiredan commented May 22, 2025

Compilation failed with the following error:

[ 95%] Building C object src/shell/CMakeFiles/pegasus_shell.dir/sds/sds.c.o
/__w/incubator-pegasus/incubator-pegasus/src/shell/sds/sds.c: In function 'sdsMakeRoomFor':
/__w/incubator-pegasus/incubator-pegasus/src/shell/sds/sds.c:200:25: error: variable 'reqlen' set but not used [-Werror=unused-but-set-variable]
  200 |     size_t len, newlen, reqlen;
      |                         ^~~~~~
/__w/incubator-pegasus/incubator-pegasus/src/shell/sds/sds.c: At top level:
cc1: note: unrecognized command-line option '-Wno-inconsistent-missing-override' may have been intended to silence earlier diagnostics
cc1: all warnings being treated as errors
make[2]: *** [src/shell/CMakeFiles/pegasus_shell.dir/build.make:303: src/shell/CMakeFiles/pegasus_shell.dir/sds/sds.c.o] Error 1
make[2]: *** Waiting for unfinished jobs....
[ 95%] Building CXX object src/server/test/CMakeFiles/pegasus_unit_test.dir/__/pegasus_server_impl.cpp.o
[ 95%] Building CXX object src/server/CMakeFiles/pegasus_server.dir/rocksdb_wrapper.cpp.o
make[1]: *** [CMakeFiles/Makefile2:3916: src/shell/CMakeFiles/pegasus_shell.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

The root cause is that the CI builds in release mode (see lint_and_test_cpp.yaml#L119), rather than in debug mode. In release mode, the statement assert(hdrlen + newlen + 1 > reqlen); gets optimized away, leading to the compiler warning that reqlen is set but not used.

In fact, the same purpose can be achieved without using the reqlen variable by assert(hdrlen + newlen + 1 > len + addlen);. However, doing so would introduce a significant difference from the changes in redis/redis@c6ad876, which may lead to more conflicts when merging code in the future. Therefore, we can use (void) to silence the unused variable warning for reqlen to solve this problem at the lowest cost.

@acelyc111 acelyc111 merged commit 656d849 into apache:master May 26, 2025
81 of 82 checks passed
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

Successfully merging this pull request may close these issues.

3 participants