fix: add safe integer conversion for bitwise operations#1217
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Unable to sign the CLA:
I will try later. |
715e678 to
42e89a4
Compare
|
CLA signed. |
johnbartholomew
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
It would be good to have some basic test coverage of this.
|
I also wonder if we need to do something extra with left-shifts. With the current code, 2^53 (or some other large value) can be left-shifted beyond the safe integer range. In fact this does work correctly up to a certain point because left shifting naturally gives a value which is a multiple of 2^x, and those values can be exactly represented even though adjacent integers cannot. However, since the operation is performed via a signed 64-bit integer, left shifting can easily result in undefined behaviour (pre C++20) or defined but confusing behaviour in C++20 and beyond. See, for example, https://stackoverflow.com/a/60712725/52251 And this correct (perhaps) but rather confusing result: |
af364c5 to
3544891
Compare
|
Good point! I added explicit checks before left-shifting to ensure the value won’t overflow a signed 64‑bit integer. If the shift would exceed 63 bits, we throw an error. This prevents undefined or confusing behavior and keeps left‑shifts within the safe range. Tests are in and pass at least locally. Feedback welcome. |
johnbartholomew
left a comment
There was a problem hiding this comment.
Thanks for continuing to work on this :-)
There is some inconsistency in the comments and code about whether the maximum safe value is 2^53 or (2^53)-1. It would be good to make sure this is correct and consistent in the comments and the actual logic.
2^53 can be represented exactly, so I would set that as the maximum (and correspondingly -2^53 as the minimum). This also matches the Jsonnet documentation.
(You can also verify it yourself by iterating through adjacent double-precision values, e.g., using nextafter, or by manually working out what the double precision representation is for 2^53-1 (ok), 2^53 (ok), and 2^53+1 (not representable!))
|
Thanks @johnbartholomew for the feedback! I guess I misunderstood the previous comment, but I've now aligned comments, code and tests to match what's in the Jsonnet spec. |
johnbartholomew
left a comment
There was a problem hiding this comment.
Looks good! Would you mind rebasing onto master? Or I can do it when I merge.
85fb4ed to
e7d6e86
Compare
Implements proper safety checks for all bitwise operations (<<, >>, &, ^, |) when converting double values to int64_t. This prevents undefined behavior and potential security issues from integer overflow or invalid values. The implementation: - Adds safeDoubleToInt64 function with proper range validation - Applies this function to all bitwise operations - Limits values to the safe integer range (±(2^53-1)) - Provides clear error messages with source location context The safe integer range limitation is necessary because IEEE 754 doubles can only precisely represent integers up to 2^53-1. Beyond this range, precision is lost, which would lead to unpredictable results in bitwise operations that depend on exact bit patterns. This change aligns with the Jsonnet specification which requires bitwise operations to convert operands to signed 64-bit integers before performing operations, while ensuring mathematical correctness. Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
Add safety checks to prevent undefined behavior in bitwise left shift operations. These checks detect potential overflow conditions before they occur and throw appropriate error messages to help users identify problematic code. The error messages are consistent between runtime and static error handling. Added tests. Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
IEEE 754 double-precision floating-point numbers can precisely represent all integers in the range [-2^53, 2^53]. The previous implementation incorrectly used the range [-(2^53 - 1), 2^53 - 1] when checking if a double could be safely converted to an int64 for bitwise operations. This commit updates the `safeDoubleToInt64` function and its associated comments and tests to use the correct safe integer range, aligning with the IEEE 754 standard and Jsonnet documentation. Test cases have been adjusted to verify the new boundaries and fix related assertions. Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
Adds the `safe_integer_conversion.jsonnet.fmt.golden` file containing the expected output from `jsonnetfmt`. This resolves the formatting test failure for this file in `run_fmt_tests.sh` by providing an explicit golden output for comparison, accounting for minor spacing differences introduced by the formatter. Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
e7d6e86 to
eff455a
Compare
|
Awesome! Rebase done |
|
Missed that max integer value was reverted to
The problem here is that "safe" integer is the one that has defined successor and predecessor For example:
Only overflow with larger integer can be detected now |
Fixes google#858. This is intended to match the restriction added in C++ Jsonnet in google/jsonnet#1217 and updated in google/jsonnet#1240
Fixes google#858. This is intended to match the restriction added in C++ Jsonnet in google/jsonnet#1217 and updated in google/jsonnet#1240
Implements proper safety checks for all bitwise operations (
<<,>>,&,^,|,~) when converting double values toint64_t. This prevents undefined behavior and potential security issues that could arise from integer overflow or the use of non-finite double values (NaN/Infinity) in bitwise contexts.The implementation:
safeDoubleToInt64helper function with range validation.[-2^53, 2^53]before conversion toint64_t.<<) operations from causingint64_toverflow.The safe integer range
[-2^53, 2^53]is used because IEEE 754 double-precision floating-point numbers, as used by Jsonnet, can precisely represent all integers within this range. Using doubles outside this range for bitwise operations could lead to unpredictable results due to precision loss.This change aligns with the Jsonnet specification, which requires bitwise operations to convert operands to signed 64-bit integers, while ensuring the conversion process is safe and mathematically sound within the limits of double precision representation. New test cases have been added to cover boundary conditions and potential errors.
NOTE: This was originally reported through the Google OSS VRP. Conclusion was that this can be reported publicly.