json icon indicating copy to clipboard operation
json copied to clipboard

Check we don't overflow when casting down integers during parsing

Open ArnaudBienner opened this issue 10 months ago • 4 comments

Hi,

I've faced the following issue when using this library: I stored some numbers in small integers (because I expect to use values in a small interval in practice) but the static_cast made during parsing lead to invalid values because of the overflow.

I believe it would be nice to have this runtime check to notice possible overflows at runtime.

ArnaudBienner avatar Apr 19 '24 15:04 ArnaudBienner

I am a bit concerned that adding overflow exceptions (though correct) are changing the behavior of the library in a breaking (and maybe also surprising) way.

nlohmann avatar Apr 19 '24 17:04 nlohmann

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @ArnaudBienner Please read and follow the Contribution Guidelines.

github-actions[bot] avatar Apr 19 '24 17:04 github-actions[bot]

Coverage Status

coverage: 100.0%. remained the same when pulling da5800cdccebdd531a09399c89df13ea6e57de21 on ArnaudBienner:develop into 8c391e04fe4195d8be862c97f38cfe10e2a3472e on nlohmann:develop.

coveralls avatar Apr 19 '24 18:04 coveralls

Thank you all for your feedback :)

Turns out the check was (by chance) working in the sample case I wrote for unit testing :( I should have been more careful and more exhaustive in my testing, sorry about that. I've added a few more unit tests.

In addition to fixing the check, I also change the code to:

  • Not perform the check if the source type is smaller than the destination type: as discussed, this is not needed in this case. I didn't managed to use enable_if to have two different version of that function. In my PR I'm using if constexpr to achieve a similar effect, but this is a C++17 feature, even if I know this project is C++11: this was just to make a point. I can either drop it completely, or if you have a way to define macros to support multiple C++/compilers versions and features, we can do this as well.
  • Always perform the check in case the source number is floating points: in this case the sizeof check is not relevant
  • Ignore the check when dealing with infinity numbers: I realized when running unit tests this was already handled specifically in the code elsewhere

ArnaudBienner avatar Apr 20 '24 17:04 ArnaudBienner