Handle RPATH in user program #48
Conversation
|
With heaptrack with this patch: The other question I had was could we also track |
There was a problem hiding this comment.
hey, this is cool but can you please document this better?
at the very least give an explanation and summary in your commit message. then ideally also add comments to the new functionality in the code.
finally, is there a way to test this? can we create a test binary that would fail in a similar way like julia when we don#t do whatever is done here?
| Dl_serinfo serinfo_size; | ||
| void *module; | ||
|
|
||
| module = original(module_path, RTLD_LAZY); |
There was a problem hiding this comment.
auto module = original(...);
| void *module; | ||
|
|
||
| module = original(module_path, RTLD_LAZY); | ||
| if (module == NULL) { |
|
|
||
| int res = dlinfo(module, RTLD_DI_SERINFOSIZE, &serinfo_size); | ||
| if (res) { | ||
| dlclose(module); |
| *serinfo = serinfo_size; | ||
| res = dlinfo(module, RTLD_DI_SERINFO, serinfo); | ||
| if (res) { | ||
| ::free(serinfo); |
| static void* hook(const char* filename, int flag) noexcept | ||
| { | ||
| auto ret = original(filename, flag); | ||
| void *ret = NULL; |
There was a problem hiding this comment.
please extract this code into a separate function with a descriptive name
| char file_path[PATH_MAX]; | ||
| for (unsigned int i = 0; i < serinfo->dls_cnt; i++) { | ||
| file_path[0] = 0; | ||
| strcat(file_path, serinfo->dls_serpath[i].dls_name); |
There was a problem hiding this comment.
strcat is unsafe and can overflow, please use strncat and track buffer size, or can we just use std::string instead?
| @@ -176,9 +176,80 @@ struct dlopen | |||
| static constexpr auto name = "dlopen"; | |||
| static constexpr auto original = &::dlopen; | |||
|
|
|||
| static Dl_serinfo* load_serinfo(const char* module_path) noexcept | |||
There was a problem hiding this comment.
instead of returning an memory allocated serinfo, can't you take that Dl_serinfo * as an out-param, and construct it on the stack of the caller and return bool instead?
| if (!res) { | ||
| fallback = true; | ||
| } else { | ||
| Dl_serinfo *serinfo; |
There was a problem hiding this comment.
see above: here just do Dl_serinfo serinfo; and then pass &serinfo to the load_serinfo call below?
| if (res) { | ||
| continue; | ||
| } | ||
| ::free(serinfo); |
There was a problem hiding this comment.
potentially leaked, has to be freed outside - better use std::unique_ptr if the heap allocation is needed, but best use the stack
| } else { | ||
| struct stat file_stat; | ||
| char file_path[PATH_MAX]; | ||
| for (unsigned int i = 0; i < serinfo->dls_cnt; i++) { |
There was a problem hiding this comment.
add documentation to explain what this loop does - it seems to stat some files and build some path, can you give some examples for how the path is build?
|
Sorry for the long silence, I currently don't have time to work on this and put it up in a bit of haste during a hackathon. The general observation is that the dynamic linker resolves Maybe I should have opened an issue first, before opening a PR ;) |
|
OK, but why does this only apply the injection mechanism and not the IIUC then a MWE would simply call |
|
Yes, as long as the library you are dlopening has an rpath and is linking against another library, the behavior should be triggered. The LD_PRELOAD mechanism simply failed for me when I tried it with Julia so I hadn't dug in if a similar change might be needed there. |
Running
heaptrack --use-inject juliafails due tothe dlopen injection breaks rpath/runpath in the user binary.
I encountered this previously in openucx/ucx#4001
and I took inspiration from their solution.
With this patch I can successfully trace malloc use in the Julia runtime.