-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
glibc: implement LD_FALLBACK_PATH
environment variable
#248547
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really nice
By the way, I created a similar PR #248777 , then I just realized you already created this. |
Will you try to submit this patch to the upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you thrown this against upstream glibc already? Even though ldd(1)
seems to have issues with that? Asking because every glibc patch causes an additional maintenance burden and I'd really like to know if there's a realistic chance of this ever getting upstreamed.
If upstream changes that block of code that was copy-pasted, are we going to notice and change our copy? What if they fix an LPE there? Could we have this patch controlled by an off-by-default flag, build an extra copy |
@amjoseph-nixpkgs
Yes, I think I fixed that locally similarly to your suggestion above, re-building to check now.
Sure, a separate |
I agree, and think this is pretty important. The glibc maintainers will have a much better idea of what kind of complications/benefits such a change could cause. |
👍
Meh, I already carry a (large) patch that simply disables
That is a pretty esoteric use case. Wouldn't it make more sense for I get really nervous about fiddling around with glibc's dynamic loader, nixpkgs-wide. There is a lot of voodoo in there, and a long history of exploits when environment variables are consulted during the dynamic loading process. I think this is a cool feature but for it to be on-by-default everywhere... I'm not sure about the cost/benefit there. |
OTOH @oxij may not get an entirely unbiased hearing there, since this feature is useful almost exclusively for closed-source software. I dunno. I would support merging this as off-by-default (i.e. an extra |
Currently the default python3 from nixpkgs does not support Python packages
in manylinux ABI. I think it’s really bad. This PR can solve the problem.
…On Tue, Aug 15, 2023 at 7:31 PM Adam Joseph ***@***.***> wrote:
Sure, a separate glibc will solve Steam issues.
👍
But doing this to the default glibc also allows to cheaply replace
libpulse with https://github.com/oxij/libcardiacarrest at build-time
Meh, I already carry a (large) patch that simply disables libpulse
throughout nixpkgs. It's not that hard. I just don't want to fight the
people who seem religiously opposed to merging it; it isn't that important
to me.
and then replace back with libpulse on per-app basis at runtime, for
instance.
That is a pretty esoteric use case.
Wouldn't it make more sense for libcardiacarrest to have an environment
variable that causes it to dlopen() libpuslse and forward all calls to it?
I get really nervous about fiddling around with glibc's dynamic loader,
nixpkgs-wide. There is a lot of voodoo in there, and a long history of
exploits when environment variables are consulted during the dynamic
loading process. I think this is a cool feature but for it to be
on-by-default everywhere... I'm not sure about the cost/benefit there.
—
Reply to this email directly, view it on GitHub
<#248547 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAES3OW26Y2DL7EQKUKPLUDXVQWHFANCNFSM6AAAAAA3NA4XXU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Even though I'm mostly opposed to adding this fallback on something so core/critical, this^ approch sounds fair to me. I know getting a review for something major like glibc is probably going to take forever and have a lot of churn, so it feels bad to block what might be a quick fix for the python many Linux abi and Steam. I'm going to have to defer to others though in terms of whether this adds too much complexity to the already massive nixpkgs. @Atry If you have any more examples, or can find examples of packages that can be fixed with this change, I think that would make for a really strong argument for merging this PR. (basically showing this is a general technique/hack instead of just a hack that should be done inside of steam and python) |
You're doing a pretty good job removing my reservations about this PR 😁 @Atry if you've got a fork or a patch file for the python change, I'll test it out with some of my projects to support/validate it. |
I pushed 6960b056594bbf413d076ea60e0c08573b1efe91 into here. It, I think, fixes all the nitpicks. It also seems to be working for the last 12 days on my system so, it's probably ok. |
> Personally, I think a better solution is to add pkgsGlibcLdFallback like I did in the new patch
Just to make sure we're on the same page: I suppose this would mean spending compute on the bootstrap again, just for the sake of some steam/cuda?
Yes, otherwise, in the worst case, you'd have to manually recursively deep-override `glibc` for each dependency of the package where you want to use `LD_FALLBACK_PATH`.
To me that seems much harder than just re-bootstrapping.
|
Slightly off-topic, but posting it here because it seems to target the same use-case. Just encountered Flox over at https://github.com/flox/flox and https://flox.dev/. Which has something called |
So this isn't directly related to the current issue, which is that "nixpkgs with
Nice, thanks! Is this the same approach as described in https://hpc.guix.info/blog/2020/05/faster-relocatable-packs-with-fakechroot/? |
From there:
So, yes, that is indeed the same mechanism :). EDIT: the use-case is slightly different though. The Guix folks use it to make closures relocatable by rewriting all search paths from |
Yes, we use LD_AUDIT as an alternative and less commonly used mechanism to guide dynamic loading. This also helps avoid invalidating the cache. Let me see if there are some portions of the approach and lessons learned along the way that might be helpful. |
If you can find the time to do this: I would be very interested in this :). I'm pretty sure other people following this PR would also find it interesting! |
Nixpkgs uses glibc 2.37 with enough patches from 2.38 for this to apply anyway.
Rebased to fix conflicts. I also left Since people appear to cherry-picking these commits to their own branches, maybe consider just merging this? It is, after all, completely optional and does nothing by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is far from the first time I'm reading this so my vision may have got blurry. I still hold that
- there's a benefit to persisting a variant of this patch - as a piece of knowledge - in Nixpkgs git history, and to making the patch more available for experimentation;
- there's no reason to hold the patch back. In particular:
- The change has no effect on the default Nixpkgs instantiation, and so has no security implications for users unless they explicit opt-in into the experimental functionality.
- We do have a deprecation mechanism for evalModules' options, so we will be able to phase the patch out if it becomes stale
IMO neither this patch nor the LD_AUDIT-based approaches seem to solve the LD_LIBRARY_PATH/DT_RUNPATH priority issue in its entirety: we should pursue and explore both.
EDIT(2024-06-02 23:03 UTC), in reaction to Ma27's latest message: I think we've already shared all opinions, and I suppose it's time we acknowledged we can't reach a consensus. The change won't go through without a consensus. Reaching out with the upstream is the next available action
My stance on this is still the same, to reiterate: this alters the way how libraries are resolved at a very central place and I still see no intentions in upstreaming this. Happy to be proven wrong though. I'd like to see feedback from upstream developers, then we'll see how to proceed. Beforehand, this can of course be maintained in its own repo though. |
As far as I can tell, for linux, it seems like the LD_AUDIT approach gives total control over loading order, including fallback methods. For example, it could re-implement the lookup logic (runpath/rpath/LD_library/system etc) but just with a patch on the fallback case. Is there an edgecase where this isn't sufficient @SomeoneSerge ? My PR Review / ThoughtsIgnoring feasibility for a moment, I would prefer the core problem be fixed with a separate package, rather than adding an option to GCC. For example, given a shell.nix, add one package (an LD audit shim), and set LD_AUDIT to that shim. This would avoid rebuilds, cache busting, and (important to me) not add complexity to GCC in nixpkgs. Again, OP did everything right for this PR, IMO. That said, Nixpkgs maintaince is a huge burden, a lot of which comes from the philosophy of "well it works right now, and people are using it, so let's tack on one more patch/option". At some point there's too many tacked-on options. Going back to feasibility, AFAIK it should be possible to have the LD audit approach support the same fallback path ENV var as this PR. But in addition, in the future, the LD audit approach could add per-binary lookups (similar to modifying the runpath, but without actually modifying the binary). For example, if a steam game cannot modify its own binary, but also does not want to affect the LD fallback of child processes, it could add itself to a env var mapping (bin-path to fallback-path mapping) # using "/.." as a split-pattern
# newlines and comments added for clairty (would be removed in practice)
# "/../=" as the map-assignment
LD_TARGETED_FALLBACK="
$LD_TARGETED_FALLBACK
:/..:
/nix/store/1234-steamthing/bin/game1 # binary
:/../=:
/nix/store/1234-gcc-thing/lib:/nix/store/1234-libpng/lib
# ^ colon-separated LD fallback for this binary specifically
:/..: # next mapping
/nix/store/1234-steamthing/bin/game2 # binary
:/../=:
/nix/store/1234-gcc-thing/lib:/nix/store/1234-libpng/lib
" Packages in nixpkgs (like the steam game) could also, in the future, individually add the LD audit shim as a runtime dependency. Which would be a nice way to gradually introduce this change into nixpkgs. _ Since, at the moment, it seems to me like that is both more flexible and less intrusive, I'm in favor of that route over accepting this PR. |
@jeff-hykin I'd rather we moved the @oxij would you like to team up on preparing the email to upstream?.. |
I'm not sure what it has to do with bundlers. I only mention the LD audit approach as a reason for blocking this PR. I agree it should eventually have its own discussion.
Despite my stance on the PR, I do support you guys on emailing upstream. Let me know if you need more voices and I get some other people to chime in.
@SomeoneSerge I still think this is important to know for this PR and emailing upstream. |
Issue at hand (opinion): it is customary for other projects to break Nixpkgs software by abusing The fact that one could implement (and that Guix and flox have) an even more complex wrapper is not entirely orthogonal to this problem, but doesn't directly address it either. The ability to hijack ld.so's search mechanism in its entirety is very interesting, it's obvious a lot could be achieved by exploring that path (maybe we could even improve on the libc situation and create a wrapper that always chooses the "newest" libc/libstdc++), but I disagree that this is on-topic or that this is blocking. The only reason I see not to merge this is that this is likely to be removed or grow obsolete relatively fast - but I'm repeating myself and I promised to stop. Footnotes
|
Thanks that was helpful as now I understand your position. I'll join you in saying, I think my side has been heard. |
Description of changes
This adds LD_FALLBACK_PATH environment variable support to
glibc
. From the commit message:Why?
Basically,
LD_FALLBACK_PATH
provides a cheap ad-hoc alternative to generating app-specific chroots for making wrappers that want to change default libraries.I made it as a better alternative to #31263 in #31263 (comment), and kept it updated since. If you look through that issue, you'll see that neither the original problem, nor any of the related ones mentioned there are solved, even though most of the issues are closed as stale.
I've been using this thing since Spring 2018 without issues.
Pings
ping @domenkozar and @Ericson2314 who wanted me to PR this and @timokau who also reported happily using my patch in #59595 (comment)