Conversation
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.