Repository navigation
Conversation
`StringViewStream::get()` returned a signed `char` as an `int`. On platforms where `char` is signed, the byte 0xFF became -1, which is `EOF`, so `parseString` threw "unterminated string in derivation". Nix strings are bytes, and `builtins.readFile` puts arbitrary bytes into a derivation's env, args, builder or system. Nix writes these derivations, but since 69b449f it cannot read them back. One such `.drv` in the store makes `nix store gc` fail for the whole store. Assisted-by: Claude Code (claude-opus-5)
| */ | ||
| struct StringViewStream | ||
| { | ||
| std::string_view remaining; |
There was a problem hiding this comment.
Could this be a std::span<const uint8_t>, and then both methods return uint8_t? Then we don't need any casts here.
There was a problem hiding this comment.
We still need the -1 for EOF
There was a problem hiding this comment.
That would be quite a bit of churn for not so apparent reason tbh... ATerm is stringly after all and we'd be a bit too annoyed casting between spans and string_view. Let's go with this fix for now.
Assisted-by: Claude Code (claude-opus-5)
|
@Ericson2314 can we just remove JSON representation of derivations already please? It's not possible to represent all aterm in that at all. |
| TEST_F(DerivationTest, NonUtf8Bytes) | ||
| { | ||
| const std::string aterm = | ||
| "Derive([(\"out\",\"/nix/store/7mjal1rkzwzrgppbvidx6gf2bls2hvb5-u6\",\"\",\"\")],[],[],\"a\xff" | ||
| "b\x80\",\"/bin/sh\",[],[(\"builder\",\"/bin/sh\"),(\"name\",\"u6\")," | ||
| "(\"out\",\"/nix/store/7mjal1rkzwzrgppbvidx6gf2bls2hvb5-u6\"),(\"system\",\"a\xff" | ||
| "b\x80\")])"; | ||
| auto drv = | ||
| derivation::parse(*store, std::string{aterm}, "u6", derivation::defaultSupportWindowsStoreDir, mockXpSettings); | ||
| ASSERT_EQ( | ||
| drv.platform, | ||
| "a\xff" | ||
| "b\x80"); | ||
| ASSERT_EQ(derivation::unparse(drv, *store, derivation::defaultSupportWindowsStoreDir), aterm); | ||
| } |
There was a problem hiding this comment.
Would be nicer to have characterisation tests but current infrastucture for them sucks and incorrectly assumes that JSON format is good. I think we need aterm-only characterisation tests.
There was a problem hiding this comment.
We can have have ATerm only tests?
There was a problem hiding this comment.
Right, let's do that. Also we can probably avoid describing the structure by hand and do "roundtrip"-style ATerm tests. How does that sound?
|
@xokdvium well I am still tempted to support As a practical matter while many other JSON implementations will not support this, people probably also don't really need non-unicode strings here for real-world use-cases, so we still get the benefits of JSON for easy interop. |
A new op, `drv FILE`, answers `ok DRVPATH SHA256` or `error`: the derivation read and written again, named by its file. Its 310 cases: - the six real .drv files that broke `nix store gc`, now with answers; - bytes/: each of the 256 bytes in an env value, and all of them in an env name, an argument, the builder and the system; - shape/: outputs of each kind, fixed outputs with each method and algorithm at the path evaluation gives, inputs, xp-dyn-drv, refusals; - upstream/: the .drv test data of Nix 2.34.8. Every accepted case comes back byte for byte, so SHA256 is the hash of the file, and a file named by its store path gives that path. lib.adapterFor builds the reference adapter from another Nix. An oracle that does not compile against its API is left out and its op skipped. Through it, Nix 2.36pre (nixVersions.git, 203f85b2) fails 11 drv cases: exactly those with a 0xFF byte, of the 256 single bytes only env-value-ff. That is the regression of 69b449fe5, fixed by NixOS/nix#16580. Assisted-by: Claude Code (claude-opus-5)
|
woops. sorry for the noise. what implication does this have for |
Because
builtins.readFilereturns a string with no validation/conversion, Nix strings can contain arbitrary bytes. Which can then bleed in a derivation's env, args, builder or system.Nix writes these derivations, but since 69b449f it cannot read them back. I had one such
.drvin the store makingnix store gcfail for the whole store.Assisted-by: Claude Code (claude-opus-5)
Motivation
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.