Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Silently get numbers into smaller types #288

Closed
vladimirgamalyan opened this issue Aug 4, 2016 · 8 comments
Closed

Silently get numbers into smaller types #288

vladimirgamalyan opened this issue Aug 4, 2016 · 8 comments
Labels
confirmed solution: invalid the issue is not related to the library

Comments

@vladimirgamalyan
Copy link

vladimirgamalyan commented Aug 4, 2016

This code:

    json j = { { "foo", 65536 } };
    int16_t v = j["foo"].get<int16_t>();
    std::cout << v << "\n";

prints

0

Probably, it would be better, if get thrown some exception (maybe std::out_of_range).

@whackashoe
Copy link
Contributor

If you are grabbing smaller widths then you must know that the numbers will be small.

@vladimirgamalyan
Copy link
Author

vladimirgamalyan commented Aug 4, 2016

@whackashoe yes, but the same rule can be apply to get number from a string, nevertheless we have an exception in that case. And, in most cases, json comes from untrusted sources, so we should check all the values from it. For numbers it can be:

        json j = { { "foo", 65536 } };
        json v = j["foo"];
        if (!v.is_number_integer())
            throw;
        int64_t a = v;
        if ((a < std::numeric_limits<int16_t>::min()) || (a > std::numeric_limits<int16_t>::max()))
            throw;
        int16_t b = static_cast<int16_t>(a);

@whackashoe
Copy link
Contributor

whackashoe commented Aug 4, 2016

If the data you are getting is untrusted, then you must pepper with checks anyways. May as well just read it into the largest width size number you have then check if you can cast downwards from there. Something expected to be a number but is actually "0234hnnnng111111" is I guess exceptional - it isn't a number at all- whereas rest of C++ historically thinks overflow isn't exceptional. I don't understand why you'd cast untrusted data to small size and assume a library will throw an exception if that doesn't fit.

Imo would be smarter to make a nice wrapper like you have and keep that for untrusted data casts but it doesn't need to be in the main lib itself- could be a nice addition to a supplementary/utils addon tho!

@vladimirgamalyan
Copy link
Author

vladimirgamalyan commented Aug 4, 2016

@whackashoe I understood the idea. Maybe my experience with another libs, with separate methods (get for strict extraction and to for best try conversion) misled me (I expected same behavior from int a = j["foo"].get<int>() and int a = j["foo"] accordingly).

@whackashoe
Copy link
Contributor

It's just my opinion on it, very subjective 👯

@nlohmann
Copy link
Owner

nlohmann commented Aug 4, 2016

RFC 7159, Section 6 states:

Since software that implements IEEE 754-2008 binary64 (double precision) numbers [IEEE754] is generally available and widely used, good interoperability can be achieved by implementations that expect no more precision or range than these provide, in the sense that implementations will approximate JSON numbers within the expected precision.

and

Note that when such software is used, numbers that are integers and are in the range [-(2^53)+1, (2^53)-1] are interoperable in the sense that implementations will agree exactly on their numeric values.

That said, JSON does not require a library to support a specific range for numbers, but proposes to use doubles for floating-point numbers and some type that is able to cope with integers in the range [-(2^53)+1, (2^53)-1].

The library follows this proposal closely, as documented for integers, unsigned integers, and floating-point numbers.

Casting a JSON value number to a type that can only cover a subrange is dangerous, and preventing harm is out of scope of a JSON library. It would also contradict the “not pay for what you don't use” approach of C++. If you need to find the smallest integer type for the number stored in the JSON value, you should implement your own conversion function.


By the way, instead of writing

int16_t v = j["foo"].get<int16_t>();

you can also write

int16_t v = j["foo"];

@nlohmann nlohmann added the solution: invalid the issue is not related to the library label Aug 4, 2016
@nlohmann nlohmann closed this as completed Aug 4, 2016
@vladimirgamalyan
Copy link
Author

@nlohmann so there is no difference between

int16_t v = j["foo"].get<int16_t>();

and

int16_t v = j["foo"];

right ?

And if we have to always test if number (from untrusted sources) is not float, than get them into in64_t, check their range, and cast to smaller type, probably it can be subject to have an additional method for strict cast? (and still have non strict conversion to follow “not pay for what you don't use” principle).

For example, well known c++ json libraries https://github.com/open-source-parsers/jsoncpp and https://github.com/miloyip/rapidjson do size checking.

@nlohmann
Copy link
Owner

nlohmann commented Aug 5, 2016

Yes, these lines are equivalent, because operator T() calls get<T>(). Alternatively, you could write auto v = j["foo"].get<int16_t>; if you do not want to repeat the type.


Numbers are parsed such that the "best" fitting type is used in the following order

  • unsigned integer (uint64_t)
  • signed integer (in64_t)
  • floating-point number (double)

If casting to double would result in NaN, a null value is created which follows the standard. That said, the library does perform size checking in the sense that "interoperable" numbers are stored correctly.

If you use the types above, you pay nothing, because no range check is necessary and no cast is executed. If you are not sure about the number type, the functions is_number_float(), is_number_integer(), and is_number_unsigned() can be used to determine the type. Alternatively, you can call type().

If you need to cast from a JSON number to a smaller range, you need to write a conversion function that detects overflow. With templates and std::numeric_limits, this should be simple, but out of scope for a JSON library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed solution: invalid the issue is not related to the library
Projects
None yet
Development

No branches or pull requests

3 participants