Visual Studio static analyzer warnings

[bnason-nf]

Hi,

I just ran the VStudio static analyzer on cJSON, and it reported these issues (which generally look pretty minor):

cjson\cjson.c(327): warning C26451: Arithmetic overflow: Using operator '-' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '-' to avoid overflow (io.2).
cjson\cjson.c(349): warning C26451: Arithmetic overflow: Using operator '-' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '-' to avoid overflow (io.2).
cjson\cjson.c(1369): warning C6011: Dereferencing NULL pointer 'input_buffer'. See line 1346 for an earlier location where this can occur
cjson\cjson.c(1678): warning C26451: Arithmetic overflow: Using operator '+' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '+' to avoid overflow (io.2).
cjson\cjson.c(2303): warning C26451: Arithmetic overflow: Using operator '-' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '-' to avoid overflow (io.2).

I can put together a pull request to address these if you'd like.

Thanks,
Benbuck

[FSMaxB]

Thanks for taking the time to report this.

Is this for cJSON 1.7.8 ? It seems to me like maybe some of the warnings belong to different lines than what I looked at?

Let's take a look at the issues?

Arithmetic Overflow 1 & 2

cjson\cjson.c(327): warning C26451: Arithmetic overflow: Using operator '-' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '-' to avoid overflow (io.2).
cjson\cjson.c(349): warning C26451: Arithmetic overflow: Using operator '-' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '-' to avoid overflow (io.2).

cJSON/cJSON.c

Line 327 in 08103f0

else if (number <= INT_MIN)

cJSON/cJSON.c

Line 349 in 08103f0

else if (number <= INT_MIN)

This doesn't make any sense to me. Not only are there no 4 byte values in cJSON nor is there any cast to any 8 byte value or any subtractions involved.

Dereferencing NULL

cjson\cjson.c(1369): warning C6011: Dereferencing NULL pointer 'input_buffer'. See line 1346 for an earlier location where this can occur

cJSON/cJSON.c

Lines 1367 to 1371 in 08103f0

	if (cannot_access_at_index(input_buffer, 0))
	{
	input_buffer->offset--;
	goto fail;
	}

In isolation this could be a NULL pointer dereference since cannot_access_at_index(input_buffer, 0) will be true if input_buffer is NULL. This means avoiding dereferencing of NULL here comes down to never passing NULL into parse_array. If this ever happens this would be really bad because it is already dereferenced here:

cJSON/cJSON.c

Lines 1346 to 1349 in 08103f0

	if (input_buffer->depth >= CJSON_NESTING_LIMIT)
	{
	return false; /* to deeply nested */
	}

BUT there is only one place where parse_array is called:

cJSON/cJSON.c

Lines 1253 to 1256 in 08103f0

	if (can_access_at_index(input_buffer, 0) && (buffer_at_offset(input_buffer)[0] == '['))
	{
	return parse_array(item, input_buffer);
	}

And this ensures that input_buffer can never be NULL!

cJSON/cJSON.c

Line 260 in 08103f0

#define can_access_at_index(buffer, index) ((buffer != NULL) && (((buffer)->offset + index) < (buffer)->length))

Arithmetic Overflow 3

cjson\cjson.c(1678): warning C26451: Arithmetic overflow: Using operator '+' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '+' to avoid overflow (io.2).

cJSON/cJSON.c

Lines 1678 to 1679 in 08103f0

	length = (size_t) ((output_buffer->format ? 1 : 0) + (current_item->next ? 1 : 0));
	output_pointer = ensure(output_buffer, length + 1);

length has only 3 possible values here: 0, 1 and 2. This means that length + 1 in the next line can only be 1, 2 or 3. So definitely no overflow here and ensure contains checks for arithmetic overflow.

Arithmetic Overflow 4

cjson\cjson.c(2303): warning C26451: Arithmetic overflow: Using operator '-' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '-' to avoid overflow (io.2).

cJSON/cJSON.c

Line 2303 in 08103f0

else if (num <= INT_MIN)

?? Same as the first two. Doesn't really make sense to me.

[FSMaxB]

Is this static analysis option built in directly to Visual Studio?

[bnason-nf]

Hi Max,

Correct, this was using cJSON 1.7.8, and the lines match up with what I'm seeing.

This functionality is built into Visual Studio, more information can be found here:

https://docs.microsoft.com/en-us/visualstudio/code-quality/code-analysis-for-c-cpp-overview?view=vs-2017

For arithmetic overflow 1 and 2, number is a double (8 byte float), and INT_MIN is an int (4 byte signed integer. Casting number to an int before comparing avoids the warning, assuming that's the type conversion you want here.

For the null pointer dereference, I think the analyzer is not taking the holistic view that you are, it's only looking at that function in isolation. Imagine if a new call site was added that passed a null pointer for input_buffer, then the analyzer would have saved you a potential problem. That may not be a realistic scenario, but if you'd like to address it anyway, a simple null check and return would take care of it.

For arithmetic overflow 3, it's pretty silly since both sides of the + operator are either zero or one, so the result will be an int value of 0, 1, or 2, and any number of bytes could hold that result without any issues, regardless of the promotion to size_t (which is 8 bytes on Windows 64 for example). In any case, just distributing the cast can satisfy the analyzer:

For arithmetic overflow 4, it's the same as 1 and 2, casting num to int before comparing addresses it.

So the overall the patch for these reported issues might look like this:

diff --git a/cJSON.c b/cJSON.c
index 5da278e..3536535 100644
--- a/cJSON.c
+++ b/cJSON.c
@@ -324,7 +324,7 @@ loop_end:
     {
         item->valueint = INT_MAX;
     }
-    else if (number <= INT_MIN)
+    else if ((int)number <= INT_MIN)
     {
         item->valueint = INT_MIN;
     }
@@ -346,7 +346,7 @@ CJSON_PUBLIC(double) cJSON_SetNumberHelper(cJSON *object, double number)
     {
         object->valueint = INT_MAX;
     }
-    else if (number <= INT_MIN)
+    else if ((int)number <= INT_MIN)
     {
         object->valueint = INT_MIN;
     }
@@ -1343,6 +1343,9 @@ static cJSON_bool parse_array(cJSON * const item, parse_buffer * const input_buf
     cJSON *head = NULL; /* head of the linked list */
     cJSON *current_item = NULL;

+    if (!input_buffer)
+        return false;
+
     if (input_buffer->depth >= CJSON_NESTING_LIMIT)
     {
         return false; /* to deeply nested */
@@ -1675,7 +1678,7 @@ static cJSON_bool print_object(const cJSON * const item, printbuffer * const out
         update_offset(output_buffer);

         /* print comma if not last */
-        length = (size_t) ((output_buffer->format ? 1 : 0) + (current_item->next ? 1 : 0));
+        length =  ((size_t)(output_buffer->format ? 1 : 0) + (size_t)(current_item->next ? 1 : 0));
         output_pointer = ensure(output_buffer, length + 1);
         if (output_pointer == NULL)
         {
@@ -2300,7 +2303,7 @@ CJSON_PUBLIC(cJSON *) cJSON_CreateNumber(double num)
         {
             item->valueint = INT_MAX;
         }
-        else if (num <= INT_MIN)
+        else if ((int)num <= INT_MIN)
         {
             item->valueint = INT_MIN;
         }

If that works for you I can put together a pull request.

Thanks,
Benbuck

[FSMaxB]

For arithmetic overflow 1 and 2, number is a double (8 byte float), and INT_MIN is an int (4 byte signed integer. Casting number to an int before comparing avoids the warning, assuming that's the type conversion you want here.

Yesterday was too late ... of course int is int32_t on x86_64, I kind of read byte as bit ...
Actually casting number to int is exactly the opposite of what I want. The fix is to cast INT_MIN to double. The only reason for this comparison is to prevent undefined behavior on overflow when double is casted to int.

For the null pointer dereference, I think the analyzer is not taking the holistic view that you are, it's only looking at that function in isolation. Imagine if a new call site was added that passed a null pointer for input_buffer, then the analyzer would have saved you a potential problem. That may not be a realistic scenario, but if you'd like to address it anyway, a simple null check and return would take care of it.

I don't want to do anything about that at the moment. But I'll consider using assert in the future.

For arithmetic overflow 3, it's pretty silly since both sides of the + operator are either zero or one, so the result will be an int value of 0, 1, or 2, and any number of bytes could hold that result without any issues, regardless of the promotion to size_t (which is 8 bytes on Windows 64 for example). In any case, just distributing the cast can satisfy the analyzer:

Casting to size_t seems to be the way to go here.

I would be glad to accept a pull request!