Skip to content
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

Lookup libraries in libjulia-* before jl_exe_handle #50162

Merged
merged 4 commits into from
Jun 20, 2023

Conversation

topolarity
Copy link
Member

During bootstrapping, many internal symbols (e.g. jl_fl_parse) are found in the global EXE namespace leading to an ambiguous lookup in Julia-in-Julia scenarios.

With this search order change, no sysimage symbols are resolved to jl_exe_handle:

$ cat objdump_after.txt | grep libjulia_internal_handle | wc
   1131   14703  145899
$ cat objdump_after.txt | grep jl_exe_handle | wc
      0       0       0

versus before:

$ cat objdump_before.txt | grep libjulia_internal_handle | wc
    577    7501   74433
$ cat objdump_before.txt | grep jl_exe_handle | wc
    554    7202   63710

We do not use `dlvsym` to separate the symbols between multiple copies
of libjulia, instead preferring to resolve symbols directly against the
appropriate internal library handle.

During bootstrapping, many internal symbols (e.g. `jl_fl_parse`) are
available in the global EXE namespace, so we need to adapt our search
order to resolve symbols in internal libraries first.

With this fix, no sysimage symbols are resolved to `jl_exe_handle`
(which is generally broken in Julia-in-Julia scenarios):
```
$ cat objdump_after.txt | grep libjulia_internal_handle | wc
   1131   14703  145899
$ cat objdump_after.txt | grep jl_exe_handle | wc
      0       0       0
```

versus before:
```
$ cat objdump_before.txt | grep libjulia_internal_handle | wc
    577    7501   74433
$ cat objdump_before.txt | grep jl_exe_handle | wc
    554    7202   63710
```
This flag is needed to ensure that `libclang_rt.asan-*.so` appears
explicitly in the DT_NEEDED entries of libjulia-*. Without this entry,
e.g. `dlsym(libjulia_internal_handle)` can end up finding symbols
directly in libc.so.6, effectively bypassing the ASAN interceptors.
@topolarity topolarity force-pushed the dlfind-search-order branch from 85ba015 to 1af6648 Compare June 14, 2023 15:32
@topolarity topolarity added the DO NOT MERGE Do not merge this PR! label Jun 14, 2023
@DilumAluthge DilumAluthge marked this pull request as draft June 20, 2023 14:47
@topolarity topolarity removed the DO NOT MERGE Do not merge this PR! label Jun 20, 2023
@topolarity
Copy link
Member Author

In order for dlsym to find the libasan interposers when we do, e.g. dlsym(jl_libjulia_internal_handle, "malloc"), this PR changes libasan (libclang_rt.asan-<arch> for Clang, libasan for GCC) to be installed as a shared library.

Due to #50170, I'm not able to test installation/relocatability with the new Makefile changes, but otherwise I think this is approximately the right approach. It works for me with contrib/asan/build.sh both locally and on deepsea

@topolarity topolarity requested a review from staticfloat June 20, 2023 15:07
@topolarity topolarity marked this pull request as ready for review June 20, 2023 15:08
@topolarity topolarity requested a review from vtjnash June 20, 2023 15:09
Copy link
Member

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right to me, but I haven't built it locally. As long as it works with the contrib scripts to build the ASAN Julia, I think we're good to go.

@vtjnash vtjnash merged commit a40cb96 into JuliaLang:master Jun 20, 2023
@ararslan
Copy link
Member

ararslan commented Aug 8, 2023

The reordering in jl_dlfind broke FreeBSD: the libdl and binary platform tests now fail. On Julia 1.9 I get 42 entries from dllist() and on 1.10 and master I get 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants