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

ArnaudBienner commented 2 months ago

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.

nlohmann commented 2 months ago

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.

github-actions[bot] commented 2 months ago

🔴 Amalgamation check failed! 🔴

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

coveralls commented 2 months ago

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

ArnaudBienner commented 2 months ago

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

nlohmann / json

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

🔴 Amalgamation check failed! 🔴