nix icon indicating copy to clipboard operation
nix copied to clipboard

Fix readfile from special files

Open thufschmitt opened this issue 1 year ago • 8 comments

Allow builtins.readFile to work on non-standard files (like /dev/stdin, pipes, etc..).

Fix #9330

Thanks @ThinkChaos for the bisect and the implementation

Keeping as draft for now because

  1. It doesn't fully work (builtins.readFile "/dev/stdin" from a tty works, but echo "foo" | nix eval --impure --expr 'builtins.readFile "/dev/stdin"' fails with error: opening file '/proc/3978327/fd/pipe:[15074562]': No such file or directory)
  2. I'm not sure about all the implications – I imagine that this allows reading from a pipe in pure eval mode, which isn't exactly pure, so we might need further restrictions
  3. ~~The change introducing the breakage (https://github.com/NixOS/nix/commit/57db3be9e448042814d386def2d8af16a2a4c4b9) made the read more streaming, and this undoes that. We might want to only fallback to a non-streaming read when we can't avoid it~~ EDIT: Fixed now

thufschmitt avatar Feb 15 '24 09:02 thufschmitt

I'm not sure that builtins.readFile should support special files. Our notion of FSOs doesn't include them after all, and they're not very pure/reproducible. What's the use case for this? If it's passing in data from special files on the CLI, it would probably be better to have a --arg-from-file foo /dev/stdin or something like that.

edolstra avatar Feb 15 '24 12:02 edolstra

I agree that supporting special files might be a misfeature which is part of why I didn't make a PR myself and wanted more feedback.

My personal use case is doing something similar to what's described in Secure, Declarative Key Management with NixOps, Pass, and nix-plugins: using the extra-builtins plugin to read something that's private but not secret. I.E. stuff I don't mind in my Nix store (and thus can be available during evaluation for simplicity), but don't want in my repo such as my physical address, phone number, etc.

--arg-from-file should be enough to achieve that:
--expr 'builtins.readFile "/dev/stdin"' would become --arg-from-file foo /dev/stdin --expr foo

To set expectations, I'm willing to personally do either of the following:

  • finish this minimal patch to get the stdin readFile pattern working again
  • only address "2. this allows reading from a pipe in pure eval mode" leaving readFile not working with non-regular files

If your preferred option is --arg-from-file, then I I'll get rid of my need for this pattern. Not because I'm against that solution, but simply because I already have plans for getting rid of this pattern.

ThinkChaos avatar Feb 15 '24 14:02 ThinkChaos

My use case is that I have some Nix Expressions that I want to run as regular binaries that read from stdin and write to stdout.

For example, something like this to get the version of a package:

#!/nix/var/nix/profiles/default/bin/nix eval --show-trace --json -f
let
  package = builtins.replaceStrings [ "\r" "\n" ] [ "" "" ] ( builtins.readFile "/dev/stdin" );
in
  ( import <nixpkgs> { } ).${ package }.version

Then you would run it like this:

% echo 'git' | ./main.nix
"2.43.0"

It is worth noting that this still works on macOS, only Linux was affected by this regression. So it would be wise to either restore the old behaviour or drop it completely in both OSs.

t0rr3sp3dr0 avatar Feb 15 '24 20:02 t0rr3sp3dr0

@t0rr3sp3dr0 that's an interesting pattern, but my first reaction is you'd be better off doing this:

#!/usr/bin/env -S nix-instantiate --eval
{ package ? null }:
( import <nixpkgs> { } ).${ package }.version

And run it using ./main.nix --argstr package git.
For better UX you could even write a wrapper script that transforms args into the --argstr form, and use that as the shebang interpreter.

ThinkChaos avatar Feb 15 '24 21:02 ThinkChaos

That's just an example, the real expression I have is more complex. We are indexing package versions by commit hash in a way that allows you to tell Nix to install a specific version of some package or to pin a major or minor version without having to manually look for the commit hashes in https://lazamar.co.uk/nix-versions and then use the old channel revision to install that package.

To enable this, we have some jobs that run daily to fetch new package versions, do some testing, and update the indices. These jobs are a mix of Bash Scripts and Nix Expressions that read JSON files and write JSON files.

We could change our scripts to get the input from a variable, but it's more convenient to have it read from stdin and write to stdout. The change would make us run something like this: ./main.nix --argstr input "$(cat input.json)".

t0rr3sp3dr0 avatar Feb 16 '24 01:02 t0rr3sp3dr0

I'm not sure that builtins.readFile should support special files. Our notion of FSOs doesn't include them after all, and they're not very pure/reproducible. What's the use case for this? If it's passing in data from special files on the CLI, it would probably be better to have a --arg-from-file foo /dev/stdin or something like that.

I don't know whether we should have supported them at all from the beginning, but we used to support them (and we still do on macOS), so we shouldn't break that. We should obviously make sure that it doesn't work in pure eval mode, but that seems to be already pretty well handled.

FWIW, my use-case which made me notice this was using Nix as a jq-like scripting layer (something like echo '{"foo": 1}' | nix eval --impure --expr '(builtins.fromJSON (builtins.readFile "/dev/stdin")).foo')

thufschmitt avatar Feb 16 '24 08:02 thufschmitt

We should obviously make sure that it doesn't work in pure eval mode, but that seems to be already pretty well handled.

I disagree, IMO the list of cursed paths (i.e. not blessed) should be replaced with a fstat + IS_REG. That's orthogonal to this fix though.

ThinkChaos avatar Feb 16 '24 16:02 ThinkChaos

but we used to support them (and we still do on macOS), so we shouldn't break that

I don't know if we have to guarantee perpetual bug-compatibility. FWIW, reading from special(-ish) files has not worked for most of Nix's history, e.g. on Nix 2.3:

# nix-instantiate --eval-only -E 'builtins.readFile "/proc/meminfo"'
""

The S_ISREG test is insufficient on Linux because files like /proc/meminfo pretend to be regular files.

edolstra avatar Feb 18 '24 16:02 edolstra

Discussed during the Nix maintainers meeting on 2024-02-28. Rejected. Could accept an --arg-file argument to have that feature, but not within the language itself.

  • @edolstra: Ugly, not sure we want the language to have that
  • Explicit CLI flag is better, --arg-file
  • Close

thufschmitt avatar Feb 29 '24 06:02 thufschmitt

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

https://discourse.nixos.org/t/2024-02-28-nix-team-meeting-129/40499/1

nixos-discourse avatar Feb 29 '24 07:02 nixos-discourse