libexpr: fix UB in builtins.ceil and builtins.floor#13013
Conversation
tighten and fix specification of both builtins
| v.mkInt(ceilValue); | ||
| } else if (isInt) { | ||
| // a NixInt, e.g. INT64_MAX, can be rounded to -int_min due to the cast to NixFloat | ||
| state.error<EvalError>("Due to a bug (see https://github.com/NixOS/nix/issues/12899) the NixInt argument %1% caused undefined behavior in previous Nix versions.\n\tFuture Nix versions might implement the correct behavior.", args[0]->integer().value).atPos(pos).debugThrow(); |
There was a problem hiding this comment.
Maybe the error should point out that the integer is out of the allowed range? It's probably also documented in the issue, but it's probably a good idea to just point this out?
There was a problem hiding this comment.
The input is in range! A NixInt input cannot be out of range, if the function would have been implemented correctly. The rounding of the int64 to double cast causes it to go out of range, i.e. the error is the same as in the final check, but the final check cannot catch this (on x86_64 it would) because of the UB.
I don't know, what else should be explained here. The documentation mentions a safe range and this check applies to exactly 512 NixInt values only, i.e. all integers greater than INT64_MAX - 512, if the rounding mode is round to nearest even.
| auto ceilValue = ceil(value); | ||
| bool isInt = args[0]->type() == nInt; | ||
| constexpr NixFloat int_min = std::numeric_limits<NixInt::Inner>::min(); // power of 2, so that no rounding occurs | ||
| if (ceilValue >= int_min && ceilValue < -int_min) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Motivation
Currently the
builtins.ceilandbuiltins.floorfunctions can trigger undefined behavior and due to the implementation integers with a magnitude larger than2^53can be rounded into the wrong direction. This violates the current, very sparse documentation of the builtins. Thus an error is thrown is such cases, too, in order to not silently break backwards compatibility.The documentation is fixed and is more precise now.
Context
Throws an evaluation error in cases, where the issues of
builtins.ceilandbuiltins.floormentioned in #12899 are encountered.Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.