lib: fix overflowing fromHexString tests and example

[emilazy]

fromHexString is backed by builtins.fromTOML. Per the TOML v1.0.0 specification:

Arbitrary 64-bit signed integers (from −2^63 to 2^63−1) should be
accepted and handled losslessly. If an integer cannot be represented
losslessly, an error must be thrown.

The saturating behaviour of the toml11 version currently used by Nix is not lossless, and is therefore a violation of the TOML specification. We should not be relying on it. This blocks the update of toml11, as it became stricter about reporting this condition.

This, yes, is arguably an evaluation compatibility break. However, integer overflow was recently explicitly defined as an error by both Nix and Lix, as opposed to the C++ undefined behaviour it was previously implemented as:

• https://nix.dev/manual/nix/stable/release-notes/rl-2.25
• https://docs.lix.systems/manual/lix/stable/release-notes/rl-2.91.html#fixes

This included changing builtins.fromJSON to explicitly reject overflowing integer literals. I believe that the case for builtins.fromTOML is comparable, and that we are effectively testing undefined behaviour in TOML and the Nix language here, in the same way that we would have been if we had tests relying on overflowing integer arithmetic. I am not aware of any use of this behaviour outside of these tests; the reverted toml11 bump in Nix did not break the 23.11 evaluation regression test, for example.

C++ undefined behaviour is not involved here, as toml11 used the C++ formatted input functions that are specified to saturate on invalid values. But it’s still a violation of the TOML specification caused by insufficient error checking in the old version of the library, and inconsistent with the handling of overflowing literals in the rest of Nix.

Let’s fix this so that Nix implementations can correctly flag up this error and we can unblock the toml11 update.

Things done

• Built on platform:
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • NixOS tests in nixos/tests.
  • Package tests at passthru.tests.
  • Tests in lib/tests or pkgs/test for functions and "core" functionality.
• Ran nixpkgs-review on this PR. See nixpkgs-review usage.
• Tested basic functionality of all binary files, usually in ./result/bin/.
• Nixpkgs Release Notes
  • Package update: when the change is major or breaking.
• NixOS Release Notes
  • Module addition: when adding a new NixOS module.
  • Module update: when the change is significant.
• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[lf-]

oh yeah that is ..... a nasty thing to throw into a test in nixpkgs tbh. thanks for fixing it. lossful encoding is not ideal.

[flokli]

CI disagrees:

Error:        error: while parsing TOML: [error] toml::parse_hexadecimal_integer
        --> fromTOML
          |
        1 | v=0x-8000000000000000
          |   ^--- the next token is not an integer
Error: Process completed with exit code 1.

[emilazy]

Yeah it turns out that this doesn’t handle negative literals at all. Will fix soon.

[emilazy]

I’ve fixed that and added some more tests for some really bad behaviour of this function.

@woojiq @Janik-Haag I think it is unfortunate that this was merged as part of #318712, despite the PR it was cherry‐picked from having a blocking review from @infinisil from half a year prior pointing out serious problems with this function. If those had been addressed this wouldn’t be blocking correctness fixes in Nix implementations and an update of our toml11 package, and we wouldn’t have shipped behaviour like lib.fromHexString "1\nwhoops = {}" == 1 as public API in two Nixpkgs releases. Not sure what to do now. I lean towards deprecating the function for eventual removal; its name is inconsistent with other functions like lib.toIntBase10, we would have to make breaking changes anyway to give it reasonable behaviour, and its only in‐tree user restricts the format much more heavily and could have this implementation inlined at no real cost.

[samos667]

Maybe I'm off the mark, but why does fromHexString use builtins.fromTOML?
And what is this new lib.network? It doesn't seem to be used anywhere in Nixpkgs, and it's the only thing that depends on fromHexString.
I would like to see network support in Nixpkgs, but it seems to me to be more of a POC than anything else.
A more complete implementation that I personally use is: https://github.com/oddlama/nixos-extra-modules/blob/main/lib/net.nix
based on https://github.com/oddlama/nixos-extra-modules/blob/main/lib/netu.nix for IP parsing.

[woojiq]

And what is this new lib.network?

Network library that was planned to be implemented as part of the "google summer of code" project. But after implementation I didn't see much activity in pull-requests (no reviews, suggestions, etc), and probably no one really needed it, so it wasn't fully merged and finished.

This happened because of some "political" nixpkgs stuff (that I haven't tried to understand), after which my mentor left the nixpkgs community.

P.S. My bad for not fixing blocking threads in cherry-picked commit. I'll take this as a lesson.

[emilazy]

And what is this new lib.network? It doesn't seem to be used anywhere in Nixpkgs, and it's the only thing that depends on fromHexString.

It seems like it’s used in the Akkoma module and in a few out‐of‐tree repos, and I don’t see any immediate issues with its interface or implementation. So I think there’s no reason to drop it – it’s providing value to people and not causing any harm.

fromHexString itself is surprisingly popular already. So deprecation/removal is probably not the way. It seems like the callers stick to a sensible (0x)?[0-9A-Fa-f]+ format, so I think that we can follow up after this PR to add a warning for such inputs, and turn them into a throw after a release cycle or so. I won’t put that in this PR, since it wouldn’t be suitable for backport.

Network library that was planned to be implemented as part of the "google summer of code" project. But after implementation I didn't see much activity in pull-requests (no reviews, suggestions, etc), and probably no one really needed it, so it wasn't fully merged and finished.

This happened because of some "political" nixpkgs stuff (that I haven't tried to understand), after which my mentor left the nixpkgs community.

P.S. My bad for not fixing blocking threads in cherry-picked commit. I'll take this as a lesson.

Sorry if you didn’t have a great GSoC experience, and sorry if I came off as a little cranky here! It’s a failure of review processes on our side more than anything; just a matter of figuring out how we can ensure fromHexString behaves sensibly now that people are relying on it. Thankfully I think we can restrict it to a sensible input format and it shouldn’t be too bad a compatibility break. Thanks for your contributions, and it does seem like there are people benefiting from the IPv6 interface you added, including in NixOS itself.

[emilazy]

BTW, it seems like Snix and Tvix already reject this test due to using a more compliant TOML parser:

note: while evaluating this Nix code
 --> /snixbolt:1:1
  |
1 | (builtins.fromTOML "v=0x${builtins.hashString "sha256" "test"}").v
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: while evaluating this as native code (force)
 --> /snixbolt:1:2
  |
1 | (builtins.fromTOML "v=0x${builtins.hashString "sha256" "test"}").v
  |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error[E035]: Error converting TOML to a Nix value: error in TOML serialization: TOML parse error at line 1, column 3
  |
1 | v=0x9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08
  |   ^
number too large to fit in target type

 --> /snixbolt:1:2
  |
1 | (builtins.fromTOML "v=0x${builtins.hashString "sha256" "test"}").v
  |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

and funnily enough, so did 2.3:

nix-repl> (builtins.fromTOML "v=0x${builtins.hashString "sha256" "test"}").v
error: while parsing a TOML string at (string):1:2: Malformed number (out of range: stoll: out of range) at line 1

[emilazy]

Forgot to reply to this:

Maybe I'm off the mark, but why does fromHexString use builtins.fromTOML?

Because Nix has no native integer parsing functions so we just use TOML and JSON 🫠

[hsjobeki]

Seems lgtm to me.

Negative numbers such as 0x-8 would need support in nix ideally.

A possible workaround until then could be to detect and strip the negative sign, then apply it afterwards to the result.

We could handle this, now and remove it once it is fixed in nix. I'll leave this up to you, since this is not really related to the fix done in this PR.
Otherwise i'd merge as is.

[emilazy]

We could easily support them in this function, but considering its input format is so unintentionally permissive to begin with, I’d rather focus on locking it down for now than expanding it. Not sure what Nix‐side fix you’re referring to?

[nixpkgs-ci]

Successfully created backport PR for release-25.05:

• [Backport release-25.05] lib: fix overflowing fromHexString tests and example #434217

[emilazy]

Opened #434218 to follow up on restricting the format here.