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

Disable DL_LOAD_PATH prepending for @-paths on Darwin #42721

Merged
merged 3 commits into from
Oct 21, 2021

Conversation

staticfloat
Copy link
Member

Many thanks to Randy Rucker from Apple for pointing out that we were
technically relying on undefined behavior in dyld for loading things
via jl_dlopen("@loader_path/@rpath/libfoo.dylib"), due to the
composition of dlopen("@rpath/libfoo.dylib") in our Julia code, and
the DL_LOAD_PATH prepending we do in jl_load_dynamic_library().

This PR uses a slightly modified version of a patch emailed to me by
Randy, and should solve
JuliaLang/Downloads.jl#149 on macOS Monterey
where the undefined behavior in dyld has changed.

Many thanks to Randy Rucker from Apple for pointing out that we were
technically relying on undefined behavior in `dyld` for loading things
via `jl_dlopen("@loader_path/@rpath/libfoo.dylib")`, due to the
composition of `dlopen("@rpath/libfoo.dylib")` in our Julia code, and
the `DL_LOAD_PATH` prepending we do in `jl_load_dynamic_library()`.

This PR uses a slightly modified version of a patch emailed to me by
Randy, and should solve
JuliaLang/Downloads.jl#149 on macOS Monterey
where the undefined behavior in `dyld` has changed.
@staticfloat staticfloat added backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Oct 20, 2021
staticfloat and others added 2 commits October 20, 2021 14:25
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@staticfloat
Copy link
Member Author

Confirmed to fix JuliaLang/Downloads.jl#149

@staticfloat staticfloat merged commit 76c2431 into master Oct 21, 2021
@staticfloat staticfloat deleted the sf/apple_dlpath_atpaths branch October 21, 2021 16:37
@vtjnash
Copy link
Member

vtjnash commented Oct 21, 2021

Why is this code trying to handle full paths (with /) in them anyways?

Also, seems like we have special handling here for @executable_path that is now getting dropped on macOS?

@staticfloat
Copy link
Member Author

Also, seems like we have special handling here for @executable_path that is now getting dropped on macOS?

Ironically, the @executable_path stuff is actually for everything other than macOS, since macOS supports that natively. I agree it's confusing though; what do you suggest? The @executable_path stuff is to emulate that functionality on Windows and other platforms that don't have a way to load something with a relative path from the executable itself.

Why is this code trying to handle full paths (with /) in them anyways?

I don't quite understand what you're asking.

@vtjnash
Copy link
Member

vtjnash commented Oct 21, 2021

I don't understand why this code is combining a/b with Dl_LOAD_PATH, and not just when given b. It is common for the shell, for example, to only search if given only a name and not a path.

@staticfloat
Copy link
Member Author

Aha, I see what you mean. Yeah, that's not necessarily useful.

I think we can probably refactor this to be a three-step process:

  1. Translate @executable_path/-style modnames to an absolute path.
  2. Do abspath and is_atpath detection (the latter still macOS-only)
  3. Enter DL_LOAD_PATH loop.

This way, we'll have consistent handling across platforms as much as possible, and we cut down on unnecessary jl_load_dynamic_library() calls.

@vtjnash
Copy link
Member

vtjnash commented Oct 21, 2021

I agree

@staticfloat
Copy link
Member Author

Looking into this a bit deeper, I see that we are actually transforming the entries of DL_LOAD_PATH, not modname. E.g. we are allowing for DL_LOAD_PATH to contain @executable_path/../lib/, and then prepend it to all entries.

I had forgotten that that was how I designed this, and I'm not at all sure that this is precisely what we want. I don't think we're actively using this feature yet, so we can still change it if we want to. This was done to serve these changes from my JLL stdlibs PR, where I wanted to get Base able to dlopen("libpcre2-8") when that file lived in some very specific location. Looking back on it now, my gut says that the "right" way to do this would be for us to dlopen("@executable_path/../whatever"), but the reason we aren't doing that is because we just do ccall) without any explicit dlopen() on libpcre due to us needing libpcre so early on that we don't have access to niceties like joinpath(). We could, of course, hardcode a specific dlopen() instead. I'm undecided.

KristofferC pushed a commit that referenced this pull request Oct 22, 2021
* Disable `DL_LOAD_PATH` prepending for `@`-paths on Darwin

Many thanks to Randy Rucker from Apple for pointing out that we were
technically relying on undefined behavior in `dyld` for loading things
via `jl_dlopen("@loader_path/@rpath/libfoo.dylib")`, due to the
composition of `dlopen("@rpath/libfoo.dylib")` in our Julia code, and
the `DL_LOAD_PATH` prepending we do in `jl_load_dynamic_library()`.

This PR uses a slightly modified version of a patch emailed to me by
Randy, and should solve
JuliaLang/Downloads.jl#149 on macOS Monterey
where the undefined behavior in `dyld` has changed.

* Apply suggestions from code review

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Use `[]` instead of `*`

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
(cherry picked from commit 76c2431)
KristofferC pushed a commit that referenced this pull request Oct 29, 2021
* Disable `DL_LOAD_PATH` prepending for `@`-paths on Darwin

Many thanks to Randy Rucker from Apple for pointing out that we were
technically relying on undefined behavior in `dyld` for loading things
via `jl_dlopen("@loader_path/@rpath/libfoo.dylib")`, due to the
composition of `dlopen("@rpath/libfoo.dylib")` in our Julia code, and
the `DL_LOAD_PATH` prepending we do in `jl_load_dynamic_library()`.

This PR uses a slightly modified version of a patch emailed to me by
Randy, and should solve
JuliaLang/Downloads.jl#149 on macOS Monterey
where the undefined behavior in `dyld` has changed.

* Apply suggestions from code review

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Use `[]` instead of `*`

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
(cherry picked from commit 76c2431)
KristofferC pushed a commit that referenced this pull request Oct 29, 2021
* Disable `DL_LOAD_PATH` prepending for `@`-paths on Darwin

Many thanks to Randy Rucker from Apple for pointing out that we were
technically relying on undefined behavior in `dyld` for loading things
via `jl_dlopen("@loader_path/@rpath/libfoo.dylib")`, due to the
composition of `dlopen("@rpath/libfoo.dylib")` in our Julia code, and
the `DL_LOAD_PATH` prepending we do in `jl_load_dynamic_library()`.

This PR uses a slightly modified version of a patch emailed to me by
Randy, and should solve
JuliaLang/Downloads.jl#149 on macOS Monterey
where the undefined behavior in `dyld` has changed.

* Apply suggestions from code review

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Use `[]` instead of `*`

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
(cherry picked from commit 76c2431)
KristofferC pushed a commit that referenced this pull request Nov 11, 2021
* Disable `DL_LOAD_PATH` prepending for `@`-paths on Darwin

Many thanks to Randy Rucker from Apple for pointing out that we were
technically relying on undefined behavior in `dyld` for loading things
via `jl_dlopen("@loader_path/@rpath/libfoo.dylib")`, due to the
composition of `dlopen("@rpath/libfoo.dylib")` in our Julia code, and
the `DL_LOAD_PATH` prepending we do in `jl_load_dynamic_library()`.

This PR uses a slightly modified version of a patch emailed to me by
Randy, and should solve
JuliaLang/Downloads.jl#149 on macOS Monterey
where the undefined behavior in `dyld` has changed.

* Apply suggestions from code review

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Use `[]` instead of `*`

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
(cherry picked from commit 76c2431)
@KristofferC KristofferC removed backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Nov 13, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
…42721)

* Disable `DL_LOAD_PATH` prepending for `@`-paths on Darwin

Many thanks to Randy Rucker from Apple for pointing out that we were
technically relying on undefined behavior in `dyld` for loading things
via `jl_dlopen("@loader_path/@rpath/libfoo.dylib")`, due to the
composition of `dlopen("@rpath/libfoo.dylib")` in our Julia code, and
the `DL_LOAD_PATH` prepending we do in `jl_load_dynamic_library()`.

This PR uses a slightly modified version of a patch emailed to me by
Randy, and should solve
JuliaLang/Downloads.jl#149 on macOS Monterey
where the undefined behavior in `dyld` has changed.

* Apply suggestions from code review

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Use `[]` instead of `*`

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
…42721)

* Disable `DL_LOAD_PATH` prepending for `@`-paths on Darwin

Many thanks to Randy Rucker from Apple for pointing out that we were
technically relying on undefined behavior in `dyld` for loading things
via `jl_dlopen("@loader_path/@rpath/libfoo.dylib")`, due to the
composition of `dlopen("@rpath/libfoo.dylib")` in our Julia code, and
the `DL_LOAD_PATH` prepending we do in `jl_load_dynamic_library()`.

This PR uses a slightly modified version of a patch emailed to me by
Randy, and should solve
JuliaLang/Downloads.jl#149 on macOS Monterey
where the undefined behavior in `dyld` has changed.

* Apply suggestions from code review

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Use `[]` instead of `*`

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
staticfloat added a commit that referenced this pull request Dec 23, 2022
* Disable `DL_LOAD_PATH` prepending for `@`-paths on Darwin

Many thanks to Randy Rucker from Apple for pointing out that we were
technically relying on undefined behavior in `dyld` for loading things
via `jl_dlopen("@loader_path/@rpath/libfoo.dylib")`, due to the
composition of `dlopen("@rpath/libfoo.dylib")` in our Julia code, and
the `DL_LOAD_PATH` prepending we do in `jl_load_dynamic_library()`.

This PR uses a slightly modified version of a patch emailed to me by
Randy, and should solve
JuliaLang/Downloads.jl#149 on macOS Monterey
where the undefined behavior in `dyld` has changed.

* Apply suggestions from code review

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Use `[]` instead of `*`

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
(cherry picked from commit 76c2431)
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.

3 participants