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.

Sign up for GitHub

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

Jump to bottom

make-wrapper: prevent duplicate path entries #104665

Draft
wants to merge 1 commit into
base: staging
Choose a base branch
from
Draft

make-wrapper: prevent duplicate path entries #104665

wants to merge 1 commit into from

Conversation

bjpbakker
Copy link
Contributor

Motivation for this change

Dependencies such as GTK are widely used and set by both WMs and X
applications. Due to cascading effects, these paths can get quite long with many
duplicates, and exceed PATH_MAX. This is turn causes some applications to
break (e.g. firejail with recent security patches).

This patch makes the wrapper script test whether the entry is already in the
path, and extends the path only for new entries.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@bjpbakker bjpbakker changed the title make-wrakker: prevent duplicate path entries make-wrapper: prevent duplicate path entries Nov 23, 2020
@bjpbakker
make-wrapper: prevent duplicate path entries
41f320e 
Dependencies such as GTK are widely used and set by both WMs and X
applications. Due to cascading effects, these paths can get quite long with many
duplicates, and exceed `PATH_MAX`. This is turn causes some applications to
break (e.g. `firejail` with recent security patches).

This patch makes the wrapper script test whether the entry is already in the
path, and extends the path only for new entries.
@bjpbakker bjpbakker force-pushed the wrapper-prevent-duplicate-path-entries branch from a1297a8 to 41f320e Compare November 23, 2020 13:02
@xaverdh
Copy link
Contributor

xaverdh commented Nov 23, 2020

So will this have quadratic runtime in the length of PATH then?

@bjpbakker
Copy link
Contributor Author

So will this have quadratic runtime in the length of PATH then?

All this patch does is prevent duplicate entries on any of the path variables (PATH, XDG_DATA_DIRS, etc) used with the wrapper. It doesn't strictly limit the length of any of these paths, but at least in my testing getting rid of the duplicates fixes the length issues I've experienced (mainly with firejail).

Also it's quite harmless to remove duplicate entries from the path.

@FRidh FRidh added this to the 21.03 milestone Nov 23, 2020
@FRidh FRidh changed the base branch from master to staging December 8, 2020 04:34
@FRidh FRidh requested a review from jtojnar December 8, 2020 04:34
@FRidh FRidh added this to WIP in Staging via automation Dec 8, 2020
@jtojnar
Copy link
Contributor

jtojnar commented Dec 8, 2020

If I parse this correctly, this will make --prefix no-op too, when the value is already in the variable.

And, thanks to bash, the code is even less legible than before 😿

@Ericson2314
Copy link
Member

Ericson2314 commented Dec 8, 2020 •
edited

I thought this would rewrap by choosing a new name for the underlying thing so as to not clobber the old one. Oh, we already have that.

@bjpbakker
Copy link
Contributor Author

bjpbakker commented Dec 8, 2020 •
edited

If I parse this correctly, this will make --prefix no-op too, when the value is already in the variable.

Yes that's correct.

I don't see any good way to solve that automatically. However I could preserve the existing behavior of prefixing when the value is already in the variable. Albeit at the cost of more crazy bash in the wrappers, as we'd then have to rewrite the variables completely.

And, thanks to bash, the code is even less legible than before crying_cat_face

Sadly so

@stale
Copy link

stale bot commented Jun 6, 2021

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

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 6, 2021
@doronbehar
Copy link
Contributor

Related: NixOS/rfcs#75

If I parse this correctly, this will make --prefix no-op too, when the value is already in the variable.

Yes that's correct.

Perhaps what should happen when --prefix is used with an env var value already present in the wrapper, is to move that value to the beginning, and delete the presence of it from the remaining value. For example if without --prefix the env var would be:

PATH="path1:path2:my-path"

With --prefix PATH my-path it would come out as:

PATH="my-path:path1:path2"

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

stale bot commented Apr 19, 2022

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

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 19, 2022
@Artturin Artturin modified the milestones: 21.05, 23.05 Dec 31, 2022
@stale stale bot removed 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md labels Dec 31, 2022
@RaitoBezarius RaitoBezarius modified the milestones: 23.05, 23.11 May 31, 2023
@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank marked this pull request as draft March 25, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314 Ericson2314 is a code owner

@jtojnar jtojnar Awaiting requested review from jtojnar

Assignees
No one assigned
Labels
2.status: merge conflict 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
Staging
  
WIP
Milestone
23.11
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@bjpbakker @xaverdh @jtojnar @Ericson2314 @doronbehar @RaitoBezarius @FRidh @wegank @Artturin