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 dlmopen for modules to clearly separate symbol namespace #6125

Open
wants to merge 2 commits into
base: 5.0
Choose a base branch
from

Conversation

jaromil
Copy link

@jaromil jaromil commented May 25, 2019

A module using same symbols as redis will end up calling functions in redis before its own.

I believe this is a bug and that isolation of namespaces is necessary, allowing for instance to embed Lua inside a module without interfering with the interpreter already present in Redis (see Zenroom's module)

To fix I suggest to use dlmopen(3) on _GNU_SOURCE capable systems for complete isolation of runtime symbol namespace resolution. Attached is a very small patch based on redis 5.0.5

The problem addressed is also debated here and mainly affects Linux based systems (GNU libc)

this way the dl library calls stop resolving to symbols in redis,
completely isolating namespaces. Useful for instance when embedding
own Lua without interfering with Redis.
@JohnSully
Copy link
Contributor

Have you had a chance to test this with any modules? My main concern is this could potentially result in subtle bugs.

@jaromil
Copy link
Author

jaromil commented May 27, 2019

I have tested it with two versions of my own module, one using the old API and one using the threaded and blocking one. Calls to RedisModule_* functions are succesful.

@JohnSully
Copy link
Contributor

I guess my main worry is if anything was using this behavior to do polyfills. For KeyDB at least I'll need to test it with a few other major modules.

@jaromil
Copy link
Author

jaromil commented Jun 3, 2019

I don't understand what you mean. I will see if the PR can be improved with detection of GNU libc at build time.

@JohnSully
Copy link
Contributor

I was thinking more for crazier modules like modssl: https://github.com/JohnSully/modssl That use internal Redis functionality outside of the public API. If they run on a version without the necessary internal function they could fallback to an internal one.

Its not something I particularly want to support (modssl is just a demo) but I am curious if any modules do call private APIs - and what the implications of this for those are. I'm going to do a test with some of the more popular ones fairly soon.

@jaromil
Copy link
Author

jaromil commented Jun 3, 2019

Now I get it. Looking forward to your tests. I am not sure if this change means total isolation or just a change in namespace priority. It just gives me the behaviour I would expect intuitively: call my module's symbols first, rather than silently crawl through redis' internal symbols, which is also relatively hard to debug btw.

this way does not break musl builds
@jaromil
Copy link
Author

jaromil commented Jun 13, 2019

Just improved a bit to avoid breakage on non-GNUlibc based builds (tested with musl).
Will squash in a single commit before merge.

@jaromil
Copy link
Author

jaromil commented Jan 27, 2020

The flag I propose will lead to complete isolation, so, yes, any module using host's symbols will fail loading as the mode it is not "lazy". But please consider keeping the ambiguity seriously hinders the development of advanced modules.
There are 3 scenarios possible I see (and wish redis also would consider):

  1. have a module metadata file parsed before loading the module, including a setting about the symbol scoping / isolation
  2. use dlmopen non-lazy and parse errors, fallback to dlopen if modules are missing
  3. use dlopen and include a metadata function in modules drfining scoping / isolation, then close abd reopen with dlmopen if needed / possible

@yossigo
Copy link
Member

yossigo commented Nov 24, 2020

Hi @jaromil, I'm trying to organize all module related open issues and bumped in this one. Problem is real indeed, but I don't like the use of the non-standard dlmopen which could lead to modules working well on glibc and failing on other platforms (which we see more of, with things like Alpine-based docker images).

In the past we ended up using -Wl,-Bsymbolic in the past to solve this, which should not lead to inconsistent behavior. Also, unless I'm mistaken this also makes it possible to control symbol scope at the compilation unit level, so parts of the module can still poke at Redis directly.

@jaromil
Copy link
Author

jaromil commented Nov 25, 2020

Hi! thanks for the heads up, I have tested the build flags you mention and they do not solve the case I'm describing, which in brief consists of a module shipping its own Lua interpreter.

@yossigo
Copy link
Member

yossigo commented Nov 25, 2020

@jaromil Thanks for checking. Do you have a small sample you can share? While I don't like the dlmopen solution for the reasons stated above, I think it is an important problem to have solutions for.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

4 participants