glibc: add NIX_LD_LIBRARY_PATH #31263

lukateras · 2017-11-04T23:08:01Z

Motivation for this change

Will eventually lead to fix of #21109. Related: #31182 (comment).

To test this, replace glibc with path to glibc built with this patch and paste this into terminal (provided that you have nix-shell and patchelf on path):

glibc=/nix/store/...-glibc/
cd /tmp
curl -LO https://github.com/NixOS/nixpkgs/files/1443636/gtk3-progress-bar.tar.gz
tar zxf gtk3-progress-bar.tar.gz 
cd gtk3-progress-bar
nix-shell --run make
export NIX_LD_LIBRARY_PATH=$(patchelf --print-rpath gtk3-progress-bar)
patchelf --remove-rpath gtk3-progress-bar
patchelf --set-interpreter $glibc/lib/ld-linux-x86-64.so.2 gtk3-progress-bar
./gtk3-progress-bar

If you see a progress bar, then it works. It should also correctly handle case where some libraries are on LD_LIBRARY_PATH and others are on NIX_LD_LIBRARY_PATH, and case when LD_LIBRARY_PATH is empty but NIX_LD_LIBRARY_PATH is not.

/cc @abbradar

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

7c6f434c · 2017-11-04T23:11:53Z

I think this should target the staging branch.

lukateras · 2017-11-04T23:19:05Z

@7c6f434c Yeah, it will result in a total rebuild of everything. This PR should have no conflicts with staging branch, should I leave it as is or close this and open a new one?

7c6f434c · 2017-11-04T23:22:22Z

I have zero idea about how GitHub PR UI works this week, but in general it would be a good idea if this change was targeted towards `staging`. I guess closing/re-opening is the simplest way (given there is no substantial discussion in this PR yet)

lukateras · 2017-11-04T23:25:05Z

Figured it out: https://github.com/blog/2224-change-the-base-branch-of-a-pull-request

ghost · 2017-11-05T05:44:06Z

@yegortimoshenko Perhaps #29064 can be dealt in similar way?

dezgeg · 2017-11-05T10:24:41Z

pkgs/development/libraries/glibc/NIX_LD_LIBRARY_PATH.patch

+   DL_SYSDEP_OSCHECK (_dl_fatal_printf);
+ #endif
+
+  char *nix_library_path = getenv ("NIX_LD_LIBRARY_PATH");


I doubt that you can call getenv() here without causing a security vulnerability in setuid programs but rather secure_getenv or something.

@dezgeg Fixed, see unsecvars.h patch.

lukateras · 2017-11-05T12:16:04Z

@gnidorah Yeah, we can definitely do the same thing with LD_PRELOAD_PATH and NIX_LD_PRELOAD_PATH. Just need to append NIX_LD_PRELOAD_PATH to preloadlist around line 1470 of elf/rtld.c.

abbradar

Heh, I thought of doing it in this way (concat strings) when I looked at this myself but decided that it would look too hacky. It seems not so when I look at it now ;)

abbradar · 2017-11-05T12:35:03Z

pkgs/development/libraries/glibc/NIX_LD_LIBRARY_PATH.patch

+    strcpy (combined_library_path, library_path);
+
+    if (strlen (library_path) > 0)
+      strcat (combined_library_path, ":");


Maybe better to just set a character here to avoid O(n)? Save strlen(library_path) and do combined_library_path[len] = ':'.

combined_library_path should end with NUL.

Of course but you could do [len + 1] = '\0'. My point is that we want to avoid as much overhead as possible because this code executes with /any/ new process in the entire OS. However I'm not adamant on those -- if others think it's negligible let's merge, I see no other problems with that.

abbradar · 2017-11-05T12:35:18Z

pkgs/development/libraries/glibc/NIX_LD_LIBRARY_PATH.patch

+    if (strlen (library_path) > 0)
+      strcat (combined_library_path, ":");
+
+    strcat (combined_library_path, nix_library_path);


strcpy(combined_library_path + len + (conditional) 1, nix_library_path) would be more effective.

I'd prefer this code to be as straightforward as possible :-)

abbradar · 2017-11-05T12:36:37Z

pkgs/development/libraries/glibc/NIX_LD_LIBRARY_PATH.patch

+  char *combined_library_path;
+
+  if (nix_library_path) {
+    combined_library_path = malloc (strlen (library_path) + strlen (nix_library_path) + 2);


Compute library_path length once.

Definitely, good catch!

globin · 2017-11-05T16:22:55Z

cc @edolstra @Ericson2314

vcunat · 2017-11-05T17:41:27Z

I expect tomorrow I'll be combining staging with glibc updates (fixing vulnerabilities). We might catch those together, if this PR seems OK-ish by the time I do it.

lukateras · 2017-11-05T18:13:58Z

@vcunat Thanks! I've just made and tested change proposed by @abbradar, now it doesn't compute strlen(library_path) twice and also handles separator better. This should be ready to merge.

Ericson2314

Seems fine to me. NIX_SSL_CERT_FILE is the precedent of course. I wish we could have the proper return status, but I'll take your word that it would take a big ugly patch to do so.

vcunat · 2017-11-06T10:35:52Z

If I understand glibc and the patch correctly, this applies for shared glibc but not for static one (_dl_non_dynamic_init also calling _dl_init_paths). To include that case, the new code could be moved to _dl_init_paths directly, but I can't see any use case for such a combination, so I'm inclined to keep the current approach.

edolstra · 2017-11-06T10:47:56Z

I don't really understand the motivation for this. If the problem is to allow some proprietary games to find libGL, then can't you do that by mounting /usr/lib/libGL.so in the mount namespace?

Adding a lot of ad hoc NIX_* environment variables seems ugly to me (and I regret adding NIX_SSL_CERT_FILE for creating a precedent).

vcunat · 2017-11-06T11:26:17Z

Such additions are a bit risky. I personally currently can't see a better way around the use cases when you have a mix of binaries from nix and others (e.g. running on a non-nixos host). There it's common to have interferences due to one of those two groups wanting to override libs (like nixpkgs for opengl) and negatively affecting the others.

Another thing is, that on 2.26 #28622 this gets us:

/build/glibc-2.26/stdlib/getenv.c:84: undefined reference to `strncmp'

(not counting a trivial merge conflict in sysdeps/generic/unsecvars.h)

I also tried to address some other nitpicks in https://github.com/vcunat/nixpkgs/commits/p/glibc-NIX_LD but not this linking error.

Well, let me leave this open for now.

lukateras · 2017-11-06T12:14:37Z

@vcunat Do you plan to apply this patch to glibc 2.26? If so, I can rebase it on glibc 2.26 and resolve this.

I also had patched _dl_non_dynamic_init but dropped because I didn't see it as necessary. However, come to think of it, if we are to switch to NIX_LD_LIBRARY_PATH for OpenGL system-wide, this indeed should be moved to _dl_init_paths so that we support statically linked OpenGL applications.

@edolstra This is not only for games, or proprietary software; see #31136 (comment): avrdude is GPL'd. Quite a few games on Steam are open-source, too. This is essentially to make Nix composable with other package managers (PlatformIO, Steam): some of the software they distribute presumes that LD_LIBRARY_PATH is empty (or at least non-essential) and rewrites it.

What is really ugly here is mutability of environment variables. If LD_LIBRARY_PATH was to be append-only, this would not be an issue. Perhaps we should patch setenv instead? (not joking)

vcunat · 2017-11-06T12:18:10Z

@yegortimoshenko: glibc-2.26 is in staging now. There are some security fixes in there, so I don't want to delay it. You can utilize my branch (it used merge instead of rebase).

lukateras · 2017-11-06T13:45:24Z

@vcunat By the way, can this resolve #9415?

vcunat · 2017-11-06T14:37:04Z

@yegortimoshenko: no, there's a much more complex situation. Maybe the libcapsule approach at the end, if combined with libglvnd and maybe some extra hacks.

abbradar · 2018-03-14T20:44:18Z

You can patch libglvnd to look into /run/opengl-driver first, but that wouldn't be that different from just setting LD_LIBRARY_PATH, really. Just a bit cleaner.

Yeah, we already do just that -- override libglvnd (and other similar libraries like vulkan-loader) to look there. I agree that's similar but avoids problems with LD_LIBRARY_PATH and friends.

non-Nix binaries loaded with their ld-linux on NixOS

Yeah, that's definitely a case which we don't want to support.

non-Nix binaries loaded with our ld-linux on NixOS: NIX_LD_LATE_RUNPATH and/or ld.so.cache provide paths to the common libs your app doesn't explicitly override,

Note: this is a non-existent case outside of FHS environments, otherwise agreed.

Nix binaries on non-NixOS: LD_LIBRARY_PATH-wrapper like nixGL and/or libglvnd for libGL emulate NixOS environment, nix binaries work as if they are run under NixOS completely ignoring the host environment (unless you override it explicitly yourself),

If by "environment" you mean installed stuff in /usr (sans explicit overrides) then yeah.

Overall that's pretty much what I believed we are going to do. I have half-finished branch moving us to libglvnd which I abandoned at that time because Mesa was still believed to be unstable with it.

guibou · 2018-03-14T21:52:38Z

@oxij Please steal everything you want from nixGL (most of its current state is anyway stolen from @deepfire's nix-install-vendor-gl. But please add me to the future discussions / pull requests ;)

oxij · 2018-03-15T08:13:56Z

@guibou Ok! @abbradar I implemented it, it works :) See https://github.com/oxij/nixpkgs/commits/glibc/ld-fallback I decided to name the variable `LD_FALLBACK_PATH` because it's a generic thing. The only issue I currently see is that `ldd` doesn't recognize the effect of `LD_FALLBACK_PATH` and reports `libGL.so` missing in my tests (everything else works, however). But maybe this should be considered a feature and not a bug (because those libraries are missing in some sense). Not yet sure.

abbradar · 2018-03-15T13:05:09Z

@oxij Cool! ldd should show them -- it's not supposed to have different behavior from actual ld.so search codepaths. Overall this may even be upstreamable -- before just adding it to NixOS it would be a good idea to post it to glibc's mailing list and see what they have to say.

mmahut · 2019-08-19T11:28:47Z

Are there any updates on this pull request, please?

drewboardman · 2020-01-31T17:27:40Z

Is this still in progress? I'm seeing this error:

"can't load .so/.DLL for: /nix/store/681354n3k44r8z90m35hm8945vsp95h1-glibc-2.27/lib/libpthread.so (/nix/store/681354n3k44r8z90m35hm8945vsp95h1-glibc-2.27/lib/libpthread.so.0: undefined symbol: __libc_vfork, version GLIBC_PRIVATE)"

It's the same error mentioned here

#31846

and here

#29064

lukateras · 2020-02-29T18:49:15Z

I'm going to take a stab at rebasing this patch against the latest glibc.

oxij · 2020-02-29T20:20:58Z

My `glibc/ld-fallback` branch mentioned above works without any changes (just cherry-pick that commit, it still applies cleanly) and without the caveats of this implementation has, as discussed above, though. So, if we are to merge something like this without upstreaming the patch, then I think my implementation should be chosen over this, if only for the fact that it is not Nix-specific and can, in principle, be upstreamed (given somebody puts a bunch of effort to make `ldd` respect `LD_FALLBACK` variable; which, btw, IIRC is also a caveat here).

domenkozar · 2020-03-02T09:12:43Z

@oxij can you create a PR?

danielfullmer · 2020-04-29T00:00:45Z

Just adding a note here that I'm fairly certain this would solve issues we're having with SteamVR+Proton: #71554

DavHau · 2020-06-08T07:32:07Z

Why create an entirely new fallback mechanism if we can just use the one which is already built into ld-linux? I think this is what #59595 tries to do.

7c6f434c · 2020-06-12T09:56:17Z

@DavHau I guess for every upstream mechanism there is some package making assumptions about it in a way that is inconvenient for Nixpkgs…

stale · 2020-12-09T21:49:37Z

I marked this as stale due to inactivity. → More info

Mic92 · 2021-01-07T20:47:02Z

Fun fact in https://github.com/Mic92/nix-ld I also introduced NIX_LD_LIBRARY_PATH.

stale · 2021-07-08T00:22:12Z

I marked this as stale due to inactivity. → More info

oxij · 2023-08-11T14:54:55Z

I PRed my glibc patch at #248547.

lukateras mentioned this pull request Nov 4, 2017

chroot-user: rewrite in C, drop CHROOTENV_EXTRA_BINDS #31182

Merged

8 tasks

lukateras changed the base branch from master to staging November 4, 2017 23:23

GrahamcOfBorg added 10.rebuild-darwin: 1-10 10.rebuild-linux: 501+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild labels Nov 4, 2017

7c6f434c approved these changes Nov 5, 2017

View reviewed changes

dezgeg reviewed Nov 5, 2017

View reviewed changes

lukateras force-pushed the glibc/NIX_LD_LIBRARY_PATH branch from d4b552c to c269b3e Compare November 5, 2017 11:21

lukateras mentioned this pull request Nov 5, 2017

platformio fails to upload #31136

Closed

abbradar requested changes Nov 5, 2017

View reviewed changes

lukateras force-pushed the glibc/NIX_LD_LIBRARY_PATH branch from c269b3e to c55304c Compare November 5, 2017 18:07

Ericson2314 approved these changes Nov 5, 2017

View reviewed changes

vcunat self-assigned this Nov 6, 2017

vcunat removed their assignment Nov 6, 2017

veprbl added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 28, 2018

oxij mentioned this pull request Apr 17, 2019

[WIP] binutils, build-fhs-userenv: make use of ld fallback #59595

Draft

10 tasks

lukateras mentioned this pull request May 18, 2019

steam / steam-run: make it possible / document how to override steamrt dependencies #32881

Closed

danielfullmer mentioned this pull request May 4, 2020

Build failure SimulaVR/Simula#108

Closed

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 9, 2020

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 7, 2021

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 8, 2021

Artturin added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label May 4, 2022

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 4, 2022

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 2, 2022

Artturin marked this pull request as draft February 2, 2023 18:49

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Feb 2, 2023

AndersonTorres closed this Jun 25, 2023

oxij mentioned this pull request Aug 11, 2023

glibc: implement LD_FALLBACK_PATH environment variable #248547

Open

glibc: add NIX_LD_LIBRARY_PATH #31263

glibc: add NIX_LD_LIBRARY_PATH #31263

Conversation

lukateras commented Nov 4, 2017 • edited Loading

Motivation for this change

Things done

7c6f434c commented Nov 4, 2017

lukateras commented Nov 4, 2017

7c6f434c commented Nov 4, 2017 via email

lukateras commented Nov 4, 2017

ghost commented Nov 5, 2017

dezgeg Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukateras commented Nov 5, 2017

abbradar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

globin commented Nov 5, 2017

vcunat commented Nov 5, 2017

lukateras commented Nov 5, 2017 • edited Loading

Ericson2314 left a comment • edited Loading

Choose a reason for hiding this comment

vcunat commented Nov 6, 2017

edolstra commented Nov 6, 2017

vcunat commented Nov 6, 2017

lukateras commented Nov 6, 2017 • edited Loading

vcunat commented Nov 6, 2017

lukateras commented Nov 6, 2017

vcunat commented Nov 6, 2017

abbradar commented Mar 14, 2018 • edited Loading

guibou commented Mar 14, 2018

oxij commented Mar 15, 2018 via email

abbradar commented Mar 15, 2018

mmahut commented Aug 19, 2019

drewboardman commented Jan 31, 2020

lukateras commented Feb 29, 2020

oxij commented Feb 29, 2020 via email

domenkozar commented Mar 2, 2020

danielfullmer commented Apr 29, 2020

DavHau commented Jun 8, 2020

7c6f434c commented Jun 12, 2020

stale bot commented Dec 9, 2020

Mic92 commented Jan 7, 2021

stale bot commented Jul 8, 2021

oxij commented Aug 11, 2023

lukateras commented Nov 4, 2017 •

edited

Loading

dezgeg Nov 5, 2017 •

edited

Loading

lukateras commented Nov 5, 2017 •

edited

Loading

Ericson2314 left a comment •

edited

Loading

lukateras commented Nov 6, 2017 •

edited

Loading

abbradar commented Mar 14, 2018 •

edited

Loading