fix insufficient check in cJSON_DetachItemViaPointer

[hopper-vul]

cJSON_DetachItemViaPointer() will crash if the detached item has field prev is null. The common suitation scenario is the detached item is created by cJSON_Create* APIs and directly pass it to cJSON_DetachItemViaPointer(object, item) call without adding item to object previously. Then the cJSON_DetachItemViaPointer() will crash because it does not check whether the passed item has valid prev field.

As detach a non-existent item is an undesirable behavior, instead of raising an uneasy core dump, this commit adds the NULL check of item->prev in cJSON_DetachItemViaPointer and return NULL to inform user such unexpect behavior (as user will routinely free/handle the detached resources later).

Signed-off-by: hopper-vul hopper.vul@gmail.com

[hopper-vul]

The smallest case is

#include "cJSON.h"
  
int main(){
        cJSON *object = cJSON_CreateObject();
        cJSON *item = cJSON_CreateObject();
        cJSON *deatch = cJSON_DetachItemViaPointer(object, item); //core dump!
        if (deatch == NULL)
                cJSON_Delete(item);
        cJSON_Delete(object);
        return 0;
}

[hopper-vul]

@Alanscut @FSMaxB Hi, could you please review this PR? I'm looking forward to your feedback.

Thanks.

[Alanscut]

I think the null check of item-prev should be at the place it's visited.

Just think about this scenario:
When item == parent->child, it means item is the first child of parent. In this case, the item->prev is NULL cause it's the first item. In your implementation this will return NULL and it's not what we are expected.

[Alanscut]

I prefer adding this null check to the place where item->prev is used instead of here.
I mean:

    if (item != parent->child)
    {
        if (item->prev == NULL)
        {
            return NULL;
        }
        /* not the first element */
        item->prev->next = item->next;
    }

[MSQFIGHTING]

I think the null check of item-prev should be at the place it's visited.

Just think about this scenario: When item == parent->child, it means item is the first child of parent. In this case, the item->prev is NULL cause it's the first item. In your implementation this will return NULL and it's not what we are expected.
Hi, when i see the readme , i found that if "item == parent->child", the item->prev will be the last object of his parent object and the item->prev->next will be NULL