-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix nested dynamic library loading via RPATH #25694
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1051,7 +1051,7 @@ var LibraryDylink = { | |
| #endif | ||
| } | ||
| var rpathResolved = (rpath?.paths || []).map((p) => replaceORIGIN(rpath?.parentLibPath, p)); | ||
| return withStackSave(() => { | ||
| var result = withStackSave(() => { | ||
| // In dylink.c we use: `char buf[2*NAME_MAX+2];` and NAME_MAX is 255. | ||
| // So we use the same size here. | ||
| var bufSize = 2*255 + 2; | ||
|
|
@@ -1061,6 +1061,7 @@ var LibraryDylink = { | |
| var resLibNameC = __emscripten_find_dylib(buf, rpathC, libNameC, bufSize); | ||
| return resLibNameC ? UTF8ToString(resLibNameC) : undefined; | ||
| }); | ||
| return FS.lookupPath(result).path; | ||
| }, | ||
| #endif // FILESYSTEM | ||
|
|
||
|
|
@@ -1168,6 +1169,7 @@ var LibraryDylink = { | |
| dbg(`checking filesystem: ${libName}: ${f ? 'found' : 'not found'}`); | ||
| #endif | ||
| if (f) { | ||
| libName = f; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comment here maybe? "Replace libName with its full path. Important for resolving other libs via $ORIGIN rpath" |
||
| var libData = FS.readFile(f, {encoding: 'binary'}); | ||
| return flags.loadAsync ? Promise.resolve(libData) : libData; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2437,6 +2437,67 @@ def get_runtime_paths(path): | |
| self.assertEqual(get_runtime_paths('libside1.so'), ['$ORIGIN']) | ||
| self.assertEqual(get_runtime_paths('a.out.wasm'), ['$ORIGIN']) | ||
|
|
||
| def test_dylink_dependencies_rpath_nested(self): | ||
| create_file('pre.js', r''' | ||
| Module.preRun.push(() => { | ||
| Module.ENV.LD_LIBRARY_PATH = "/lib1"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can just do |
||
| }); | ||
| ''') | ||
| create_file('side1.c', r''' | ||
| #include <stdio.h> | ||
|
|
||
| void side2(); | ||
|
|
||
| void side1() { | ||
| printf("side1\n"); | ||
| side2(); | ||
| } | ||
| ''') | ||
| create_file('side2.c', r''' | ||
| #include <stdio.h> | ||
| void side3(); | ||
|
|
||
| void side2() { | ||
| printf("side2\n"); | ||
| side3(); | ||
| } | ||
| ''') | ||
| create_file('side3.c', r''' | ||
| #include <stdio.h> | ||
|
|
||
| void side3() { | ||
| printf("side3\n"); | ||
| } | ||
| ''') | ||
| create_file('main.c', r''' | ||
| #include <dlfcn.h> | ||
| #include <stdio.h> | ||
|
|
||
| typedef void (*F)(void); | ||
|
|
||
| int main() { | ||
| void* handle = dlopen("libside1.so", RTLD_NOW); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assert(handle); and then assert(side1) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this test depend on dlopen? Or can the bug be reproduced using normal runtime dynamic linking too? |
||
| F side1 = (F)dlsym(handle, "side1"); | ||
|
|
||
| printf("main\n"); | ||
| side1(); | ||
| return 0; | ||
| } | ||
| ''') | ||
| os.mkdir('libs') | ||
| os.mkdir('lib1') | ||
| self.emcc('side3.c', ['-fPIC', '-sSIDE_MODULE', '-olibs/libside3.so']) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need |
||
| self.emcc('side2.c', ['-fPIC', '-sSIDE_MODULE', '-olibs/libside2.so', '-Wl,-rpath,$ORIGIN', 'libs/libside3.so']) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually scratch that.. I'm gonna remove that argument :) #25699 |
||
| self.emcc('side1.c', ['-fPIC', '-sSIDE_MODULE', '-Wl,-rpath,$ORIGIN/../libs', '-olib1/libside1.so', 'libs/libside2.so']) | ||
| settings = ['-sMAIN_MODULE=2', '-sDYLINK_DEBUG', "-sEXPORTED_FUNCTIONS=[_printf,_main]", "-sEXPORTED_RUNTIME_METHODS=ENV"] | ||
| preloads = [] | ||
| for file in ['lib1/libside1.so', 'libs/libside2.so', 'libs/libside3.so']: | ||
| preloads += ['--preload-file', file] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this test depend on file preloading? Or can you just use |
||
| cmd = [EMCC, 'main.c', '-fPIC', '--pre-js', 'pre.js'] + settings + preloads | ||
| self.run_process(cmd) | ||
|
|
||
| self.run_js('a.out.js') | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these 4 lines can be replaced with: Also maybe just inline the settings into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add some expected output too? |
||
|
|
||
| def test_dylink_LEGACY_GL_EMULATION(self): | ||
| # LEGACY_GL_EMULATION wraps JS library functions. This test ensure that when it does | ||
| # so it preserves the `.sig` attributes needed by dynamic linking. | ||
|
|
||
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.
Maybe either put this line inside the
withStackSave, or move the final line out (i.e. just return resLibNameC), since that one doesn't need to be inside there either.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.
Should we have
__emscripten_find_dylibreturn an abspath instead? Or is that more complex that this solution?