Potential regression in behavior of builtins.fromJSON . builtins.readFile

[elitak]

I have a nix file that scrapes some json into the nix store, then tries to fromJSON (readFile thatJsonFileInTheStore), but gets an error in nix 1.12, when it did not in nix 1.11. Here's how to reproduce the problem:

# 1.11 works okay:
echo 'builtins.fromJSON ( builtins.readFile ( builtins.fetchurl https://jsonplaceholder.typicode.com/posts/1 ) )' | /nix/store/ram9pqdjx3l0f2ya8l0nn25bf986nhnw-nix-1.11.4/bin/nix-build -
# 1.12 does not:
echo 'builtins.fromJSON ( builtins.readFile ( builtins.fetchurl https://jsonplaceholder.typicode.com/posts/1 ) )' | /nix/store/9ax4mjhsys56dwhvlvl7l8jrlwr4rr2x-nix-1.12pre4911_b30d1e7/bin/nix-build -
error: the string ‘{
  "userId": 1,
  "id": 1,
  "title": "sunt aut facere repellat provident occaecati excepturi optio reprehenderit",
  "body": "quia et suscipit\nsuscipit recusandae consequuntur expedita et cum\nreprehenderit molestiae ut ut quas totam\nnostrum rerum est autem sunt rem eveniet architecto"
}’ is not allowed to refer to a store path (such as ‘/nix/store/ssvrwp9kh5fs4fhizbs1l8qcr1087caf-1’), at (string):1:1

In this case, I used fetchurl to create a sample json file in the store, but this will happen for any variable substituted there that is a derivation.

I think this probably is caused by recent changes around:

• src/libexpr/eval.cc: EvalState::forceStringNoCtx
• src/libexpr/primops.cc: prim_readFile
• src/libexpr/primops.cc: prim_fromJSON

I ran git blame around those areas, but couldn't work out what the cause was, since I'm not familiar with the way "context" works in this code.

My current workaround is to run nix-build twice and pass the store path back into the same file as an "contextless" --argstr, but that is really not optimal. Was this change intended? If so, what is the reasoning behind not allowing references to fromJSON/readFile?

[vcunat]

IIRC "string context" is precisely a list of nix store paths that the string "might refer to somewhere inside".

EDIT: long ago I didn't first see why such a thing is necessary, but then I realized how we commonly embed various store paths in the (multi-line) strings, and we use string operations to combine and transform them...

[elitak]

The json data strings make no such references. I don't know what step in the chain of calls is adding the potential references or why, but the behavior's definitely changed since 1.11.

[vcunat]

I didn't mean to point at the root of the problem (I can't see that); I just wanted to explain what I know about the contexts.

[elitak]

Okay. I've just never encountered the message when it was appropriately printed, so I can't say with certainty that it's incorrect in this case, but it sure seems like it.

Maybe someone who's very familiar with the codebase like @shlevy can have a look?

[vcunat]

Hehe, I tried with 1.12pre4911_b30d1e7 and got a different error:

error: imported archive of ‘/nix/store/ssvrwp9kh5fs4fhizbs1l8qcr1087caf-1’ lacks a signature

[elitak]

That seems like it could be a configuration issue with forcing everything in /nix/store to be signed. That's not something I'm familiar with. Try this instead: echo 'let pkgs = import <nixpkgs> {}; in builtins.fromJSON ( builtins.readFile "${pkgs.rustPlatform.rustRegistry}/config.json" )' | /nix/store/9ax4mjhsys56dwhvlvl7l8jrlwr4rr2x-nix-1.12pre4911_b30d1e7/bin/nix-build -

[shlevy]

This is caused by f7f0116, which as far as I can tell is just bogus, as I thought it might be in the original PR #834 @edolstra why did you merge?

[vcunat]

The change for that test case seems OK to me, so I don't think this is as easy as simply reverting that commit.

[edolstra]

@shlevy Probably because you 👍'd it :-)

A possible fix is to include the references of the file, rather than the file itself, in the context returned by readFile. This would be appropriate because you cannot use the contents of a file to get the path of the file (unless it contains a self-reference). Since for builtins.fetchurl, the references would be empty, builtins.fromJSON would work again.

[shlevy]

Ah, right 😁 Yes, the references seems like the right thing to do here.

[abbradar]

So I did something the wrong way after all D: While I seem to understand the problem won't the fix, as @edolstra correctly noticed, miss self-references? I'm not sure if it is an issue though -- I don't see any way self-reference should be handled somehow in Nix, so it's probably harmless to lose that information.

[shlevy]

No, self-references are caught with this.

[abbradar]

Oh, I have misunderstood then. I'm sorry but I'm unsure where should I get proper context from in the code -- can someone else show me this part? If you want to fix it by yourself it's fine too (please cc me for learning purposes).

[shlevy]

Pushed the fix, but I don't think it fixes this issue.

[elitak]

451c223 fixes

    echo 'builtins.fromJSON ( builtins.readFile ( builtins.fetchurl https://jsonplaceholder.typicode.com/posts/1 ) )' | nix-build -

but not

    echo 'let pkgs = import <nixpkgs> {}; in builtins.fromJSON ( builtins.readFile "${pkgs.rustPlatform.rustRegistry}/config.json" )' | nix-build -

[shlevy]

The latter will have all the context from pkgs.rustPlatform.rustRegistry, including references in files besides config.json. You'll need to use builtins.unsafeDiscardStringContext

[elitak]

That makes sense. I only needed the former to work, anyway.

I'm still not sure if everything is now functioning as intended. Should I close this issue?

[shlevy]

I don't think we can do any better than this without regressing on #833, so yeah I think so.