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

Use DEEPBIND flag when loading external modules using dlopen #1703

Merged
merged 4 commits into from
Feb 10, 2025

Conversation

rjd15372
Copy link
Member

@rjd15372 rjd15372 commented Feb 10, 2025

The current flags used in dlopen to load external modules don't allow modules to define symbols that are already defined by the valkey server binary because the symbol resolution first looks in the server memory and only if it does not find anything, it looks in the module (shared library) memory.

This might become a problem if, for instance, we try to implement a new scripting engine based on a newer version of Lua. The Lua interpreter library shares many symbol names with the Lua interpreter included in the Valkey server binary.

To fix the above problem, this PR adds the flag RTLD_DEEPBIND to the flags used in dlopen on systems that support it, which changes the symbol resolution strategy to look for the symbol in the module memory first, if the code executing is from the module.

The current flags used in `dlopen` to load external modules don't allow
modules to define symbols that are already defined by the valkey server
binary because the symbol resolution first looks in the server memory
and only if it does not find anything, it looks in the module (shared
library) memory.

This might become a problem if, for instance, we try to implement a new
scripting engine based on a newer version of Lua. The Lua interpreter
library shares many symbol names with the Lua interpreter included in
the Valkey server binary.

To fix the above problem, this PR adds the flag `RTLD_DEEPBIND` to the
flags used in `dlopen`, which changes the symbol resolution strategy to
look for the symbol in the module memory first, if the code executing is
from the module.

Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 71.11%. Comparing base (3828936) to head (a221102).
Report is 5 commits behind head on unstable.

Files with missing lines Patch % Lines
src/module.c 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           unstable    #1703   +/-   ##
=========================================
  Coverage     71.10%   71.11%           
=========================================
  Files           123      123           
  Lines         65523    65532    +9     
=========================================
+ Hits          46590    46600   +10     
+ Misses        18933    18932    -1     
Files with missing lines Coverage Δ
src/module.c 9.61% <0.00%> (+<0.01%) ⬆️

... and 12 files with indirect coverage changes

Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
@zuiderkwast zuiderkwast merged commit 244c570 into valkey-io:unstable Feb 10, 2025
50 checks passed
@zuiderkwast zuiderkwast added the release-notes This issue should get a line item in the release notes label Feb 10, 2025
@zuiderkwast
Copy link
Contributor

@rjd15372 Daily job failed:

https://github.com/valkey-io/valkey/actions/runs/13253032412/job/36994775025

==89691==You are trying to dlopen a /home/runner/work/valkey/valkey/tests/modules/reply.so shared library with RTLD_DEEPBIND flag which is incompatible with sanitizer runtime (see google/sanitizers#611 for details). If you want to run /home/runner/work/valkey/valkey/tests/modules/reply.so library under sanitizers please remove RTLD_DEEPBIND from dlopen flags.

@enjoy-binbin
Copy link
Member

I see we did skip it, is there anything missing in the condition?

    int dlopen_flags = RTLD_NOW | RTLD_LOCAL;
#if (defined(__linux__) || defined(__FreeBSD__)) && !defined(__SANITIZE_ADDRESS__)
    /* RTLD_DEEPBIND, which is required for loading modules that contains the
     * same symbols, does not work with ASAN. Therefore, we exclude
     * RTLD_DEEPBIND when doing test builds with ASAN.
     * See https://github.com/google/sanitizers/issues/611 for more details.
     *
     * This flag is also currently only available in Linux and FreeBSD. */
    dlopen_flags |= RTLD_DEEPBIND;
#endif

@zuiderkwast
Copy link
Contributor

@enjoy-binbin see this: #1707

zuiderkwast added a commit that referenced this pull request Feb 11, 2025
Fixes the failure trying to use dlopen with DEEPBIND with ASAN when
compiling with Clang:

==90510==You are trying to dlopen a
/home/runner/work/valkey/valkey/tests/modules/keyspecs.so shared library
with RTLD_DEEPBIND flag which is incompatible with sanitizer runtime
(see google/sanitizers#611 for details). If
you want to run
/home/runner/work/valkey/valkey/tests/modules/keyspecs.so library under
sanitizers please remove RTLD_DEEPBIND from dlopen flags.


https://github.com/valkey-io/valkey/actions/runs/13261241213/job/37018133361

The previous check only covers GCC.

Additionally, don't require GLIBC when FreeBSD is used. FreeBSD has it's
own libc which supports DEEPBIND according to its docs.

Follow-up of #1703, #1707

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
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
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants