Integer-overflow (OSS-Fuzz issue 267)

[nlohmann]

The library is continuously fuzz tested by Google's OSS-Fuzz. Today, an error was reported:

Detailed report: https://clusterfuzz-external.appspot.com/testcase?key=6062909058711552

Project: json
Fuzzer: libFuzzer_json_parse_fuzzer
Fuzz target binary: parse_fuzzer
Job Type: libfuzzer_ubsan_json
Platform Id: linux

Crash Type: Integer-overflow
Crash Address: 
Crash State:
  nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<cha
  nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<cha
  nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<cha
  

Minimized Testcase (0.34 Kb): https://clusterfuzz-external.appspot.com/download/AMIfv95dNmUALTp5hmbUCvNgnX8D8PnyrgGS7MUFE5Ag4XZV5PfVwXKLx3R-w6MapVK4_oLtah9R6cfasZ3oDG0zpihRP4LjUzOGgyCWsph28ZF4cHoZlRPwQsfXQT-CHwEbfEqhWte9hSB_nv8k6tAq-BwzeUJ87y8lGiAfeRphECPodTdkfMQ?testcase_id=6062909058711552

Issue filed automatically.

See https://github.com/google/oss-fuzz/blob/master/docs/reproducing.md for more information.

This bug is subject to a 90 day disclosure deadline. If 90 days elapse
without an upstream patch, then the bug report will automatically
become visible to the public.

The linked report contains this input file: fuzz-2-json_parse_fuzzer.zip

Furthermore, this error message describes the error.

src/json.hpp:9217:49: runtime error: negation of -9223372036854775808 cannot be represented in type number_integer_t (aka long); cast to an unsigned type to negate this value to itself

This is the line in question:

result.m_value.number_integer = -static_cast<number_integer_t>(value);

[nlohmann]

Some notes:

• Could be related to Bug in overflow detection when parsing integers #380. Edit: No, the warning still exists with the code from Bug in overflow detection when parsing integers #380.
• Possibly fixed with locale-independent str-to-num #379.
• The exact number -9223372036854775808 is already used in a roundtrip test.

[TurpentineDistillery]

This is a false-positive; in the context of the code this is not a problem even though static_cast overflows.

The value is 2^63; all math checks out.

[qwename]

I think the point is that whatever happens after the overflow is implementation-defined.

According to C++11 standard section 4.7 (Integral conversions) point 3:

If the destination type is signed, the value is unchanged if it can be
represented in the destination type (and bit-field width); otherwise,
the value is implementation-defined.

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf

[nlohmann]

There is a discussion on the number -9223372036854775808 on Stack Overflow, but I am not sure what to take away from there.

[mikea]

I think this is a valid bug: -INT64_MIN is not defined (=INT64_MAX + 1). The result of this operation could depend on compiler, platform and compilation flags.

[nlohmann]

One naive idea:

if (value == max) // edit: before I had 9223372036854775808 instead of max
  result.m_value.number_integer = INT64_MIN;
else
  result.m_value.number_integer = -static_cast<number_integer_t>(value);

[nlohmann]

With 79fa8b2 the warning is gone and all tests still succeed. If Travis and AppVeyor succeed, I shall trigger a re-test at OSS-Fuzz.

[mikea]

It will re-test automatically once new fuzzer is built and used by cluster-fuzz (~1 day). Let's give it a chance to work automatically.

[nlohmann]

@mikea Thanks for the hint! And thanks for taking care about this!

[TurpentineDistillery]

I think this is a valid bug: -INT64_MIN is not defined (=INT64_MAX + 1). The result of this operation could depend on compiler, platform and compilation flags.

I was a bit surprised that the standard would leave this detail implementation-defined and did some googling.

Turns out that is so in order to facilitate compatibility with hardware that employs one's-complement representation of integral types, just in case someone decides to implement a c++11 compiler for a UNIVAC-1100.

[mikea]

@TurpentineDistillery I agree that the reason it is undefined is very outdated. The unspecified behaviour is still in the standard and compilers might take opportunities related to it and such bugs are worth fixing. Take a look at Chandler's talk from last CppCon where he touches on similar issues: https://www.youtube.com/watch?v=yG1OZ69H_-o

[nlohmann]

The error has been reported as fixed:

ClusterFuzz has detected this issue as fixed in range 201612121340:201612121438.

Detailed report: https://clusterfuzz-external.appspot.com/testcase?key=6062909058711552

Project: json
Fuzzer: libFuzzer_json_parse_fuzzer
Fuzz target binary: parse_fuzzer
Job Type: libfuzzer_ubsan_json
Platform Id: linux

Crash Type: Integer-overflow
Crash Address:
Crash State:
nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<cha
nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<cha
nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<cha

Fixed: https://clusterfuzz-external.appspot.com/revisions?job=libfuzzer_ubsan_json&range=201612121340:201612121438

Minimized Testcase (0.34 Kb): https://clusterfuzz-external.appspot.com/download/AMIfv95dNmUALTp5hmbUCvNgnX8D8PnyrgGS7MUFE5Ag4XZV5PfVwXKLx3R-w6MapVK4_oLtah9R6cfasZ3oDG0zpihRP4LjUzOGgyCWsph28ZF4cHoZlRPwQsfXQT-CHwEbfEqhWte9hSB_nv8k6tAq-BwzeUJ87y8lGiAfeRphECPodTdkfMQ?testcase_id=6062909058711552

See https://github.com/google/oss-fuzz/blob/master/docs/reproducing.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

Thanks for the cooperation!

[marton78]

oh cool, you do fuzz testing! do you know this one? it's fantastic: http://lcamtuf.coredump.cx/afl/

[nlohmann]

Yes, there is a makefile target make fuzz_testing using AFL. See https://github.com/nlohmann/json/tree/develop/test/reports/2016-10-02-fuzz for the latest report.