-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
builtins.substring: fix int overflow #7222
base: master
Are you sure you want to change the base?
Conversation
cc @puckipedia |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Care to add a little testcase for it? (here is probably the best place to do it)
Looks good otherwise 👍
src/libexpr/primops.cc
Outdated
@@ -3424,7 +3424,7 @@ static void prim_substring(EvalState & state, const PosIdx pos, Value * * args, | |||
.errPos = state.positions[pos] | |||
})); | |||
|
|||
v.mkString((unsigned int) start >= s->size() ? "" : s->substr(start, len), context); | |||
v.mkString((unsigned NixInt) start >= s->size() ? "" : s->substr(start, len), context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes a compilation failure on macOS. Wouldn't it make more sense to cast it to size_t
, since we're comparing to s->size()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's annoying, a cast to size_t would overflow on 32-bit systems. I've now added a forceIntChecked function that uses boost::numeric_cast
to do a checked cast to size_t where applicable in the primops.
repro: builtins.substring 4294967296 1 "umu"
0fa546c
to
07e1702
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good overall, but it feels like the error messages are slightly worse than what they used to be (when evaluating something like builtins.genList (x: x) (-1) for example
).
It might be nicer to make forceIntChecked
return an optional or equivalent rather than throwing, and keeping the error handling on the client side where we can get some better messages
I spent a while converting to a std::optional, but:
I've added a commit to separate overflows from underflows. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-03-03-nix-team-meeting-minutes-37/25998/1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additions to the error messages are sensible, very good. Only added suggestions on phrasing, but this is cherry on top.
src/libexpr/primops.cc
Outdated
state.error("integer %1% is too low", result) | ||
.withTrace(pos, errorCtx) | ||
.debugThrow<TypeError>(); | ||
} else { | ||
state.error("integer %1% is too high", result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's integer values that can be high or low, but integers themselves can be large or small.
That's the only thing I see that could be improved immediately. Ideally though we would determine the bound and do the fully explicit thing saying something like: integer %1 too small, must be at least %2
Or, depending on your taste, and I don't have an opinion on this: The value %1 is too low, it must be at least %2
Using "value" and avoiding "integer" allows reuse of that procedure for other numeric type.
We may also use the well-known phrase "out of bounds" so it can be immediately recognised for that class of error: out of bounds: the value %1 is too low, it must be at least %2
Pick what feels right to you.
https://www.boost.org/doc/libs/1_34_1/libs/numeric/conversion/doc/bounds.html
On the topic of error messages, I'd just like to point out a marginal regression wrt what we have on master (I don't have a strong opinion as to whether that should be a blocker or not, just want to make sure that's not an overlook): $ # On master
$ nix eval --expr 'builtins.genList (x: x) (-1)'
error:
… while calling the 'genList' builtin
at «string»:1:1:
1| builtins.genList (x: x) (-1)
| ^
error: cannot create list of size -1
$ # On this branch
$ nix run github:yorickvP/nix/fix-substring-int -- eval --expr 'builtins.genList (x: x) (-1)'
error:
… while calling the 'genList' builtin
at «string»:1:1:
1| builtins.genList (x: x) (-1)
| ^
… while evaluating the second argument passed to builtins.genList
at «none»:0: (source not available)
error: integer -1 is too low |
Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com>
repro:
builtins.substring 4294967296 1 "umu"