NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
18.03k stars 14.03k forks source link

Kitty leaks packages into system environment #171972

Open Infinidoge opened 2 years ago

Infinidoge commented 2 years ago

Describe the bug

When installing Kitty (the terminal emulator), several paths are leaked into the system environment, as opposed to staying within the wrapped program. In particular, the nix store path to kitty, imagemagick, xsel, and ncurses.dev are added to PATH

Steps To Reproduce

Steps to reproduce the behavior:

  1. Install kitty (not reproducible in nix-shell, I'm using environment.systemPackages)
  2. printenv PATH (or tr ':' '\n' <<< "$PATH" since it is easier to read)
  3. Observe the packages specified in kitty's wrapProgram (here) included in PATH as Nix store paths

Expected behavior

The nix store paths should not be present in PATH, since the wrapper should use them internally.

Screenshots

image

Additional context

A similar problem happens with Qtile, where a number of its dependencies are leaked into PATH as opposed to just being wrapped, including Python, which prevents me from setting up my own Python-with-packages in my system environment because it comes before the systemPackages in PATH. However, I know that this is caused by the Qtile derivation using python3.withPackages as opposed to a wrapper, and will be investigating that next.

Notify maintainers

@tex @rvolosatovs @Luflosi

Metadata

 - system: `"x86_64-linux"`
 - host os: `Linux 5.17.5, NixOS, 22.05 (Quokka), 22.05.20220503.cbe587c`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.9.0pre20220428_4bb111c`
 - channels(root): `"nixos"`
 - nixpkgs: `/nix/store/a7vqg16mrzq49vb6j7i3ry0i49i24rvf-source`

Since I'm running a Flakes setup, my nixpkgs revision is cbe587c735b734405f56803e267820ee1559e6c1

Infinidoge commented 2 years ago

You know... I think I may know the problem for Kitty specifically. Since I'm accessing the internal inside kitty, it may inherit its own path. Still seems like weird behavior, though.

jtojnar commented 2 years ago

That is expected, environment variables are inherited by child processes by default (see exec(3p)). Kitty would have to clean the variables passed to it in the wrapper from the environment the child processes are run – but it has no way to distinguish those from variables from ambient environment. (Or it would have to run the child processes from a different, unwrapped process using some kind of systemd magic like GNOME Terminal does.)

It is one of the main downside of wrappers and I would argue that we might want to choose alternative methods like hardcoding the paths in source code to avoid this behaviour.

Similar issue exists for window managers and other shells, unless they also do the systemd trickery.

Infinidoge commented 2 years ago

Hmm, yeah I should have probably remembered that.

(Or it would have to run the child processes from a different, unwrapped process using some kind of systemd magic like GNOME Terminal does.)

I may look into how the GNOME Terminal does this, since this seems like an overall cleaner option.

... like hardcoding the paths in source code to avoid this behaviour.

Also may look into this, though I'll need to check how Kitty uses it.

Similar issue exists for window managers and other shells...

Definitely explains why I was seeing stuff from Qtile as well, since combined those are my terminal and WM.

jtojnar commented 2 years ago

(Or it would have to run the child processes from a different, unwrapped process using some kind of systemd magic like GNOME Terminal does.)

I may look into how the GNOME Terminal does this, since this seems like an overall cleaner option.

I might have misremembered how it worked. There was some environment weirdness in the past but it does not seem to use systemd-run I was thinking of.

Infinidoge commented 2 years ago

Hmm, systemd-run seems interesting and could work, however trying to recreate a proper user environment inside that seems... painful.

Infinidoge commented 2 years ago

It's unfortunate that building a form of transient path isn't really possible with a var like TRANSIENT_PATH, otherwise that would be highly convenient.

Though, I've had a thought: When using a wrapper, would it be possible to format the additions to PATH in such a way that it is easily removed through something in a shell's init script (or if you want to clean it before running a process, using some sort of helper script), like with an ignored keyword that is included in that part of the PATH.

I'm not sure how practical it would be, if at all, but something like /nix/store/XXX-thing/bin -> /nix/store/wrapped-XXX-thing/bin, and since the wrapped- prefix is special, it would be filtered out. Then a script could remove the wrapped paths.

jtojnar commented 2 years ago

I am afraid it would make already complex system even more complex. And there are some cases where you want to preserve the environment (e.g. when program re-execs itself). Hardcoding paths using a patch will be a lot simpler and cleaner.

nixos-discourse commented 2 years ago

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/overlay-with-new-version/20076/8

tejing1 commented 1 year ago

If hardcoding paths to avoid the need to wrap isn't feasible, another option would be to patch the location where kitty starts the shell or other program it's supposed to run inside the terminal, and run it with an "unwrapper" script. Since the "unwrapper" script would be part of the kitty package, it would know, character for character, what was done by the wrapper, so undoing it cleanly shouldn't be hard.

See also: #186243

tejing1 commented 1 year ago

See also: #47290

This problem is everywhere.

Infinidoge commented 1 year ago

I am surprised that this GitHub issue has seen activity again, but yeah the problem really is everywhere. I honestly didn't think about the possibility of an 'unwrapper', that sounds like a surprisingly good solution, even if admittedly weird-feeling. As much as it feels like there should be some fundamentally better solution, an unwrapper would at least work.

misumisumi commented 1 year ago

I am from #186243. After a day of trial and error, I got a method that works for me and I share it with you. It is simple though, just append

PATH=$(echo "$PATH" | sed 's/\/nix\/store\/[a-zA-Z._0-9-]\+\/bin:\?//g' | sed 's/:$//')
export PATH="$PATH"

to your .zshrc or .bashrc. This is in response to the fact that the only PATH included in the wrapper is /nix/store, It is only valid within a shell session, so side effects may be minimal.

tejing1 commented 1 year ago

Hmm, that will interfere with nix shell and nix-shell functionality, but if that's not an issue for you, it's a decent solution.

misumisumi commented 1 year ago

Ah, indeed. I had only checked nix-direnv. I modified it a bit so that nix shell and nix-shell are not affected. (Split by shell depth, need to set environment variable if startx is used)

 if [ -n "$DESKTOP_SESSION" ] && [ "$SHLVL" -eq 2 ] || [ "$SHLVL" -eq 1 ]; then
    PATH=$(echo "$PATH" | sed 's/\/nix\/store\/[a-zA-Z._0-9-]\+\/bin:\?//g' | sed 's/:$//')
    export PATH="$PATH"
fi
jcollie commented 1 year ago

I worked around it by adding this line to my kitty.conf, which resets the PATH to the default.

env PATH=/run/wrappers/bin:${homeDirectory}/.nix-profile/bin:/etc/profiles/per-user/${username}/bin:/nix/var/nix/profiles/default/bin:/run/current-system/sw/bin
tejing1 commented 1 year ago

Repeating this here from the terminator issue, since most of the discussion of workarounds has been here:

if [ "$(basename "$(readlink "/proc/$PPID/exe")")" -eq ".kitty-wrapped" ]; then
    PATH=$(echo "$PATH" | sed 's/\/nix\/store\/[a-zA-Z._0-9-]\+\/bin:\?//g' | sed 's/:$//')
fi

Should be a more accurate condition than @misumisumi's above. It's still not perfect, though. If you start a kitty instance from inside a nix-shell, for example, it will remove things it shouldn't. A more nuanced sed command might be able to help with that, based on the predictability of what kitty's wrapper adds to the PATH.

musjj commented 1 year ago

I think, the imagemagick one can be solved by doing (not tested):

postPatch = ''
  substituteInPlace tools/utils/images/loading.go \
    --replace \
      'return utils.FindExe("magick")' \
      'return "${lib.getExe imagemagick}"' \
'';

But I'm not sure where the ncurses binaries comes into the picture. Anyone have any ideas?

nixos-discourse commented 10 months ago

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-can-i-find-out-why-imagemagick-is-in-my-path/36646/2

nixos-discourse commented 9 months ago

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-is-xdg-data-dirs-set-for-some-apps/38432/4