`cJSON_CreateNumber` is truncating numbers outside the bounds of `INT_MIN` - `INT_MAX`

[raphael-bmec-co]

Issue

Calls to the following may result in truncated values:
void JSONVar::operator=(unsigned int i)
void JSONVar::operator=(long l)
void JSONVar::operator=(unsigned long ul)

All these methods wrap a call to CJSON_PUBLIC(cJSON *) cJSON_CreateNumber(double num) shown below which sets valuedouble correctly but truncates valueint for values outside the bounds of INT_MIN - INT_MAX.

CJSON_PUBLIC(cJSON *) cJSON_CreateNumber(double num)
{
    cJSON *item = cJSON_New_Item(&global_hooks);
    if(item)
    {
        item->type = cJSON_Number;
        item->valuedouble = num;

        /* use saturation in case of overflow */
        if (num >= INT_MAX)
        {
            item->valueint = INT_MAX;
        }
        else if (num <= (double)INT_MIN)
        {
            item->valueint = INT_MIN;
        }
        else
        {
            item->valueint = (int)num;
        }
    }

    return item;
}

The corresponding getters all access the valueint.

JSONVar::operator unsigned int () const
{
    return cJSON_IsNumber (_json) ? _json->valueint : 0;
}

JSONVar::operator long () const
{
    return cJSON_IsNumber (_json) ? _json->valueint : 0;
}

JSONVar::operator unsigned long () const
{
    return cJSON_IsNumber (_json) ? _json->valueint : 0;
}

Steps to reproduce

Sample code:

#include <Arduino_JSON.h>

void setup() {

  Serial.begin(115200);

  JSONVar jsonVar;

  jsonVar["unsignedInt"] = UINT_MAX;
  jsonVar["longMax"] = LONG_MAX;
  jsonVar["longMin"] = LONG_MIN;
  jsonVar["unsignedLong"] = ULONG_MAX;

  Serial.println("Expected: " + String((double) jsonVar["unsignedInt"]) + " got: " + String((unsigned int) jsonVar["unsignedInt"]));
  Serial.println("Expected: " + String((double) jsonVar["longMax"]) + " got: " + String((long) jsonVar["longMax"]));
  Serial.println("Expected: " + String((double) jsonVar["longMin"]) + " got: " + String((long) jsonVar["longMin"]));
  Serial.println("Expected: " + String((double) jsonVar["unsignedLong"]) + " got: " + String((unsigned long) jsonVar["unsignedLong"]));

}

void loop() {
}

Output:

Expected: 4294967295.00 got: 2147483647
Expected: 2147483647.00 got: 2147483647
Expected: -2147483648.00 got: -k
Expected: 4294967295.00 got: 2147483647

Potential fixes

1. Fix the implementation of cJSON_CreateNumber (unlikely see Clean up the cJSON struct DaveGamble/cJSON#63) OR
2. Use valuedouble and static_cast to the correct type.

JSONVar::operator unsigned int () const
{
    return cJSON_IsNumber (_json) ? static_cast<unsigned int>_json->valuedouble: 0;
}

JSONVar::operator long () const
{
    return cJSON_IsNumber (_json) ? static_cast<long>_json->valuedouble: 0;
}

JSONVar::operator unsigned long () const
{
    return cJSON_IsNumber (_json) ? static_cast<unsigned long>_json->valuedouble: 0;
}

Workaround:
When reading values of the types listed above use cast to double to get the valuedouble and then cast to the desired type.

e.g. static_cast((double) jsonVar["unsignedLong"])

[tommie]

Returning a casted double breaks the expected contract for integers (that they are exactly represented,) but is of course in line with what JS would do.

IMHO, it might be better if the getters aborted if the integer is out of bounds.

Either way: saturation here is very unexpected.

Edit However, see also #54 on support for 64-bit integers. Saturating at 31 bits sounds really odd since MAX_SAFE_INTEGER is 53 bits.

[raphael-bmec-co]

Returning a casted double breaks the expected contract for integers (that they are exactly represented,)

Fair point. Probably not an ideal fix.

Either way: saturation here is very unexpected.

This is not so because unsigned int, long and unsigned long are all specifically handled types so it happens all the time. See https://github.com/arduino-libraries/Arduino_JSON/blob/master/src/JSONVar.h#L32-L45.

[tommie]

My angle is that saturated arithmetics is not the common (expected) behavior in either C++ or JavaScript. In C++ I'd expect wrap-around or abort and in JavaScript I'd expect an inaccurate result. This being a JSON library, I would have expected one of those behaviors, not saturation at some arbitrary point (in the context of JSON's data model.)

[raphael-bmec-co]

Right, following and agreed - we had this exact issue with this "failing" silently and taking ages to pick up the bug.