Invalid union access for get_ref/get_ptr with unsigned integer

[cschreib-ibex]

Description

When a JSON value is stored as unsigned integer, it is possible to call get_ref<number_integer_t>() or get_ptr<number_integer_t>() without error, which accesses the signed integer member of the internal union instead of the unsigned member. This is undefined behaviour in C++ standard and should not be allowed (especially since trying access with other more clearly incompatible types, such as strings, is already checked and reported as an error).

I note that a corresponding unit test exists but has been commented out:

json/tests/src/unit-reference_access.cpp

Lines 218 to 219 in 6325839

	//CHECK_THROWS_WITH_AS(value.get_ref<json::number_integer_t&>(),
	// "[json.exception.type_error.303] incompatible ReferenceType for get_ref, actual type is number", json::type_error&);

The root of the problem is that is_number_integer() returns true for both signed and unsigned integer storage, yet is used to guard reference/pointer access using signed integers:

json/include/nlohmann/json.hpp

Lines 1464 to 1467 in 6325839

	number_integer_t* get_impl_ptr(number_integer_t* /*unused*/) noexcept
	{
	return is_number_integer() ? &m_data.m_value.number_integer : nullptr;
	}

I also note the docs say that "Writing data to the referee of the result yields an undefined state.", which isn't very clear. Does this mean we are not supposed to write a value to the object pointed to by the reference/pointer? The earlier wording (changed in 4e52277#diff-b56a00981d8f3b87e3ce49a7eb27d36f4586d9c54c3fb628a88cfc000aa5fed4L2632) was "The pointer becomes invalid if the underlying JSON object changes.", which made more sense.

Reproduction steps

See code example below.

Expected vs. actual results

Expected: type error exception.
Actual: no error, unsigned data is access as signed data (undefined behaviour).

Minimal code example

#include <nlohmann/json.hpp>
#include <iostream>

int main() {
    using namespace nlohmann;

    json j = json::number_unsigned_t{1u};
    assert(j.is_number_unsigned());

    j.get_ref<json::number_integer_t&>() = -1;
    assert(j.is_number_unsigned());
    
    std::cout << j.get<json::number_unsigned_t>() << std::endl;
    return 0;
}

Error messages

None.

Compiler and operating system

Linux x64, gcc 11

Library version

3.11.3

Validation

• The bug also occurs if the latest version from the develop branch is used.
• I can successfully compile and run the unit tests.

[nlohmann]

I can confirm the issue. Indeed, using is_number_integer() is wrong there. I'll check and also have a look at the wording of the warning.

[cschreib-ibex]

Thank you!

[Ericson2314]

(Coming from #4752)

I see the change log links the issue, but it is not yet clear to me what the intended workflow (post bug fix --- no disagreement it is a bug) is? For example, will a positive number always be parsed as an unsigned number?

It seems that in general if one wanted to work by-reference, they would have to try getting it both ways just to be sure? Probably it is better to try by value then?

[nlohmann]

I see the change log links the issue, but it is not yet clear to me what the intended workflow (post bug fix --- no disagreement it is a bug) is? For example, will a positive number always be parsed as an unsigned number?

Yes.

It seems that in general if one wanted to work by-reference, they would have to try getting it both ways just to be sure? Probably it is better to try by value then?

Maybe. In any case, you must know the type of the value to safely use get_ref. Alternatively, you could use get_ptr which returns nullptr on type mismatch.