no NULL pointer check in cJSON_DetachItemViaPointer

[tregua87]

I just noticed that the function cJSON_DetachItemViaPointer does not perform a proper null-check for item->prev for the second argument. Library commit 3249730.

Let's take this simple example:

#include <cjson/cJSON.h>

#include <stdlib.h>
#include <stdint.h>

int main(int argc, char** argv) {

    cJSON *a, *b;

    a =  cJSON_ParseWithOpts("\"foo\"", nullptr, 0);
    b =  cJSON_ParseWithOpts("\"bar\"", nullptr, 0);

    cJSON_DetachItemViaPointer(b, a);

    return 0;
}

item argument is like:

p *item
$1 = {
  next = 0x0,
  prev = 0x0,
  child = 0x0,
  type = 0x10,
  valuestring = 0x602000000010 "ciao",
  valueint = 0x0,
  valuedouble = 0,
  string = 0x0
}

but there is no check for item->prev:

if (item != parent->child) {
    /* not the first element */
    item->prev->next = item->next; // At line 2215, cJSON.c
}

I can write a PR but I do not know how it is the intended behavior of the library. Where is the best place to put the NULL check?

[PeterAlfredLee]

Given item being the first element of parent->child, the item->prev would also be null, and it is correctly handled.

Besides this, cjson is designed to believe the input arguments are correct, which means the parent should be the actual parent of item. With a null check of item->prev, there will be another null pointer here:

    else if (item->next == NULL)
    {
        /* last element */
        parent->child->prev = item->prev;
    }

As parent->child is null here. And there will be some more errors if parent is not the actual parent of item. Calling cJSON_ParseWithOpts with error arguments will create corrupted items.

We can valid the input arguments, expensively. We can iterate the parent->child and valid if item is the actual child of parent. But this would cost a lot of time. cjson does not do checks like this, instead it intends to believe the input arguments are correct. This is often seen in cjson.

Let's get back to your case. We and add a null check for item->prev here, but I do no think it helps much.

[tregua87]

Calling cJSON_ParseWithOpts with error arguments will create corrupted items.

This sentence makes me think of a possible solution. Can we modify the struct cJSON and add a field to the root JSON object?

This will automatically tell if two nodes belong to the same tree. This field should be updated if you detach.

[tregua87]

Btw, I disagree with a statement of yours.

In the snippet below, parent->child points to NULL (0x0). Therefore, parent->child->prev is a NULL pointer dereference, which is a real bug.

IMO, I would avoid any NULL pointer dereference as much as possible. This may lead to a crash of the process.

else if (item->next == NULL)
    {
        /* last element */
        parent->child->prev = item->prev;
    }

[PeterAlfredLee]

This sentence makes me think of a possible solution. Can we modify the struct cJSON and add a field to the root JSON object?

Even with a root reference, a caller can easily forge a corrupted item like this:

int main(int argc, char** argv) {

    cJSON *a, *b;

    a =  cJSON_ParseWithOpts("\"foo\"", nullptr, 0);
    b =  cJSON_ParseWithOpts("\"bar\"", nullptr, 0);

        // bypass the root valid
        b->root = a->root;
    cJSON_DetachItemViaPointer(b, a);

    return 0;
}

[PeterAlfredLee]

IMO, I would avoid any NULL pointer dereference as much as possible.

Personally I do agree with you. But cjson is not implemented in that way. cjson is released long day before and it is widely used. Just my two cents.