Repository navigation
Fixes for malloc-related bugs - #228
Conversation
| } | ||
| else { | ||
| malloc_sig_t app_malloc = get_libc_malloc(); | ||
| malloc_sig_t app_malloc = get_app_malloc(); |
There was a problem hiding this comment.
This function, patch_on_linkactivity, is called on each library open. And get_app_malloc() doesn't return a value until we hit CONSISTENT, which is after all initial library loads are done. So won't this lead to names not being patched in the initial libraries? Which will break debuggers.
I think we need to fall back to other allocators if app_malloc isn't ready. I'm investigating which malloc routines ld-linux uses, and we probably need to match that.
There was a problem hiding this comment.
This will lead to names not being patched in the initial libraries, if the original name requires more storage than the relocated name. So long as the original name fits in the existing storage for l_name, it doesn't require malloc.
The cases are:
- original name fits in l_name: names patched both before and after this PR
- original name too long, libc not in link map yet: name not patched either before or after this PR
- original name too long, libc (or, after PR, any library defining malloc) already in link map: crashes before PR, name not patched after PR
There was a problem hiding this comment.
Understood that it's not introduced by this patch, but I think we still need to fix this. Otherwise debuggers are going to break.
From my claude-assisted investigating, it'll be safe to use spindle's malloc to allocate a patched name in the case where we haven't reached CONSISTENT.
Libraries that are dlopen'd after CONSISTENT normally have strings and structures allocated with the application malloc, so they do need this code so that they don't fault when deallocated under dlclose.
But, libraries loaded before CONSISTENT are allocated with the initial rtld heap, which doesn't support being free'd and thus aren't free'd. So it should be safe to allocate a string from spindle's heap.
There was a problem hiding this comment.
I've changed it to fall back to using Spindle's heap for l_name before we get LA_ACT_CONSISTENT
| @@ -0,0 +1,6 @@ | |||
| int origin_target(); | |||
There was a problem hiding this comment.
Can we expand this testing to dlopen and dlclose the libraries with long paths? I believe the dlclose is going to cause free calls, which are going to validate that we are calling the right mallocs.
There was a problem hiding this comment.
I've expanded the test. Now that we are changing l_name consistently, the test checks the link maps to verify that l_name has the original name, and dlopens and dlcloses a library via a long path which also depends on a library via a long path. The test runs with the default malloc, plus tagmalloc, so this exercises the case where we do allocate, and the tagmalloc variant verifies that the correct malloc/free pair was used
test dlopen/dlclose with long path, alternate malloc
|
Thanks. I'm good with merging this once CI finishes. |
This PR fixes three issues found in testing with the E4S testsuite, all related to Spindle's use of the application's
malloc:readlinkinside of the firstmallocin order to read a configuration file. Thereadlinkwas then intercepted by Spindle, which was then itself trying tomallocwhile initializing a thread-local variable. This is fixed by changing the TLS model toinitial-exec.realpath), the memory would be allocated with themallocfromlibceven if the application used an alternate malloc. When the application then tried tofreethis memory, it would use its alternate implementation offree. This is fixed by, instead of always usinglibc, findingmallocby iterating over the link maps to find a DSO that exports a definedmallocsymbol.patch_on_linkactivityattempts to patchl_nameback to the original path rather than Spindle's relocated path. If the original path is longer than Spindle's relocated path, it must allocate memory in which to store the original path, which it does with the application'smalloc. However, this could occur early, beforeLA_ACT_CONSISTENT, in which case it would be unsafe to use the obtainedmalloc. This is fixed by checking whetherLA_ACT_CONSISTENThas already been seen, and falling back to not replacingl_namewith the original path if it hasn't and Spindle can't safely use the application'smallocyet.New tests check for interoperability with alternate malloc implementations.
tagmalloc.cwraps libc malloc, attaching a tag to allocated memory so that we can verify on free that it came from this wrapper.realpath_free_testandrealpath_free_lib_testinclude the wrapper directly in the executable or link against it as a shared library, respectively, and verify that Spindle'sreadlinkreturns memory allocated through the wrapper.long_needed_path_test,long_needed_app_malloc_test, andlong_needed_lib_malloc_testgenerate an executable which links against a library via a long, slash-padded path, forcing the attempt to allocate storage forl_name, and verifies that this does not crash with libc malloc or an alternate malloc.I have rerun the E4S testsuite on this PR. The
chapelandadios2tests, which previously failed, pass with this PR, and no new failures are introduced. After this PR, thestctest is the only remaining E4S test failure.Closes #225. Closes #226. Closes #227.