Fix nested dynamic library loading via RPATH #25694
Conversation
hoodmane
force-pushed
the
fix-nested-dylib-loading
branch
from
2560c87 to
78de8b2
Compare
There is a bug with dynamic library loading. Suppose liba.so is on LD_LIBRARY_PATH and it has an RPATH of `$ORIGIN/other_dir` and loads `libb.so` from other_dir. Then suppose `libb.so` has an RPATH of `$ORIGIN` and wants to load `libc.so` also from `other_dir`. Before this PR this doesn't work. The problem is that `flags.rpath.parentLibPath` is set to `libb.so` and we replace $ORIGIN with `PATH.dirname(parentLibName)` which is `.`. So unless `other_dir` is on the `LD_LIBRARY_PATH` or is the current working directory, loading will fail. The fix: if `findLibraryFS()` returns a value that is not `undefined`, replace `libName` with the returned value.
hoodmane
force-pushed
the
fix-nested-dylib-loading
branch
from
78de8b2 to
befeae4
Compare
See upstream PR: emscripten-core/emscripten#25694 This fixes shapely.
|
I think this is the same thing I wanted to fix in #24234 and which is already in Pyodide patches |
| 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.
I think these 4 lines can be replaced with:
cflags = ['-fPIC', '--pre-js', 'pre.js'] + settings + preloads
self.do_runf('main.c', cflags=cflags)
Also maybe just inline the settings into cflags rather than a separate var.
There was a problem hiding this comment.
Maybe add some expected output too?
| 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.
Does this test depend on file preloading? Or can you just use -sNODERAWFS instead?
| ''') | ||
| os.mkdir('libs') | ||
| os.mkdir('lib1') | ||
| self.emcc('side3.c', ['-fPIC', '-sSIDE_MODULE', '-olibs/libside3.so']) |
There was a problem hiding this comment.
You don't need -fPIC in addtion to MAIN_MODULE and/or SIDE_MODULE.
| 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.
I think you can just do ENV.LD_LIBRARY_PATH = "/lib1"; and you don't need to actually export ENV, just add it to DEFAULT_LIBRARY_FUNCS_TO_INCLUDE
| typedef void (*F)(void); | ||
|
|
||
| int main() { | ||
| void* handle = dlopen("libside1.so", RTLD_NOW); |
There was a problem hiding this comment.
assert(handle);
and then
assert(side1)
| dbg(`checking filesystem: ${libName}: ${f ? 'found' : 'not found'}`); | ||
| #endif | ||
| if (f) { | ||
| libName = f; |
There was a problem hiding this comment.
Add a comment here maybe? "Replace libName with its full path. Important for resolving other libs via $ORIGIN rpath"
| var resLibNameC = __emscripten_find_dylib(buf, rpathC, libNameC, bufSize); | ||
| return resLibNameC ? UTF8ToString(resLibNameC) : undefined; | ||
| }); | ||
| return FS.lookupPath(result).path; |
There was a problem hiding this comment.
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.
Should we have __emscripten_find_dylib return an abspath instead? Or is that more complex that this solution?
| os.mkdir('libs') | ||
| os.mkdir('lib1') | ||
| self.emcc('side3.c', ['-fPIC', '-sSIDE_MODULE', '-olibs/libside3.so']) | ||
| self.emcc('side2.c', ['-fPIC', '-sSIDE_MODULE', '-olibs/libside2.so', '-Wl,-rpath,$ORIGIN', 'libs/libside3.so']) |
There was a problem hiding this comment.
The self.emcc take an output_filename named param (instead of -o arg).
There was a problem hiding this comment.
Actually scratch that.. I'm gonna remove that argument :) #25699
| typedef void (*F)(void); | ||
|
|
||
| int main() { | ||
| void* handle = dlopen("libside1.so", RTLD_NOW); |
There was a problem hiding this comment.
Does this test depend on dlopen? Or can the bug be reproduced using normal runtime dynamic linking too?
3 participants
There is a bug with dynamic library loading. Suppose liba.so is on LD_LIBRARY_PATH and it has an RPATH of
$ORIGIN/other_dirand loadslibb.sofrom other_dir. Then supposelibb.sohas an RPATH of$ORIGINand wants to loadlibc.soalso fromother_dir.Before this PR this doesn't work. The problem is that
flags.rpath.parentLibPathis set tolibb.soand we replace $ORIGIN withPATH.dirname(parentLibName)which is.. So unlessother_diris on theLD_LIBRARY_PATHor is the current working directory, loading will fail.The fix: if
findLibraryFS()returns a value that is notundefined, replacelibNamewith the returned value.