Fixes #904 : Fixes sort_list function to maintain previous node data after running mergesort

[nathanjjohnson7]

Fixes #904

Previously, the sort_list() function, which is called by cJSONUtils_SortObject, wasn't managing the prev data point of each node, while going through the different steps of mergesort. This caused the ->prev value of the first item in the list, to point to NULL instead of pointing to the last item in the list, leading to errors when you try to add new elements to the list.

This commit fixes that by tracking and correctly assigning the prev value at each step.

Below is a script that tests the sort_list functionality. It creates an object, sorts the object, shows the next and prev values of each element and attempts to add new elements and sort again:

#include <stdio.h>
#include "cJSON.h"
#include "cJSON_Utils.h"

int main() {
    printf("Creating Object ... \n\n");
    cJSON *object = cJSON_Parse("{\"v1\": 1, \"a2\": 2}");

    char *object_str = cJSON_Print(object);
    printf("object=\n%s\n", object_str);
    free(object_str);

    printf("Connections: \n");
    for(cJSON *node = object->child;node!=NULL;node = node->next){
        printf(
            "node: %s, node->prev: %s, node->next: %s\n",
            node->string,
            node->prev ? node->prev->string : "null",
            node->next ? node->next->string : "null"
        );    
    }
    printf("\n");

    cJSONUtils_SortObject(object);

    printf("\n");
    printf("sorted Object: \n");
    object_str = cJSON_Print(object);
    printf("object=\n%s\n", object_str);
    free(object_str);
    printf("\n");

    printf("Connections: \n");
    for(cJSON *node = object->child;node!=NULL;node = node->next){
        printf(
            "node: %s, node->prev: %s, node->next: %s\n",
            node->string,
            node->prev ? node->prev->string : "null",
            node->next ? node->next->string : "null"
        );    
    }
    printf("\n");

    printf("Adding V3: \n");
    cJSON *v3_value = cJSON_CreateNumber(3);
    cJSON_AddItemToObject(object, "v3", v3_value);

    object_str = cJSON_Print(object);
    printf("object=\n%s\n", object_str);
    free(object_str);

    printf("Connections: \n");
    for(cJSON *node = object->child;node!=NULL;node = node->next){
        printf(
            "node: %s, node->prev: %s, node->next: %s\n",
            node->string,
            node->prev ? node->prev->string : "null",
            node->next ? node->next->string : "null"
        );    
    }
    printf("\n");

    printf("Adding d5, a0, z7, k2: \n");
    cJSON *d5_value = cJSON_CreateNumber(5);
    cJSON_AddItemToObject(object, "d5", d5_value);
    cJSON *a0_value = cJSON_CreateNumber(0);
    cJSON_AddItemToObject(object, "a0", a0_value);
    cJSON *z7_value = cJSON_CreateNumber(7);
    cJSON_AddItemToObject(object, "z7", z7_value);
    cJSON *k2_value = cJSON_CreateNumber(2);
    cJSON_AddItemToObject(object, "k2", k2_value);


    object_str = cJSON_Print(object);
    printf("object=\n%s\n", object_str);
    free(object_str);

    printf("Connections: \n");
    for(cJSON *node = object->child;node!=NULL;node = node->next){
        printf(
            "node: %s, node->prev: %s, node->next: %s\n",
            node->string,
            node->prev ? node->prev->string : "null",
            node->next ? node->next->string : "null"
        );    
    }
    printf("\n");

    cJSONUtils_SortObject(object);

    printf("\n");
    printf("sorted Object: \n");
    object_str = cJSON_Print(object);
    printf("object=\n%s\n", object_str);
    free(object_str);
    printf("\n");

    printf("Connections: \n");
    for(cJSON *node = object->child;node!=NULL;node = node->next){
        printf(
            "node: %s, node->prev: %s, node->next: %s\n",
            node->string,
            node->prev ? node->prev->string : "null",
            node->next ? node->next->string : "null"
        );    
    }
    printf("\n");

    return 0;
}

The result of running the above script using the previous implementation of sort_list() is the following:

Creating Object ...

object=
{
        "v1":   1,
        "a2":   2
}
Connections:
node: v1, node->prev: a2, node->next: a2
node: a2, node->prev: v1, node->next: null


sorted Object:
object=
{
        "a2":   2,
        "v1":   1
}

Connections:
node: a2, node->prev: null, node->next: v1
node: v1, node->prev: a2, node->next: null

Adding V3:
object=
{
        "a2":   2,
        "v1":   1
}
Connections:
node: a2, node->prev: null, node->next: v1
node: v1, node->prev: a2, node->next: null

Adding d5, a0, z7, k2:
object=
{
        "a2":   2,
        "v1":   1
}
Connections:
node: a2, node->prev: null, node->next: v1
node: v1, node->prev: a2, node->next: null


sorted Object:
object=
{
        "a2":   2,
        "v1":   1
}

Connections:
node: a2, node->prev: null, node->next: v1
node: v1, node->prev: a2, node->next: null

As shown above, once the object is sorted, the ->prev value of the first element in the list points to NULL instead of the last element of the list. node->prev of a2 points to NULL instead of v1. Once this error is made, users can no longer add to the list, as shown above. Every successive call to cJSON_AddItemToObject results in no change at all.

Below is the result of running the script with the modifications of sort_list() proposed in this commit:

Creating Object ...

object=
{
        "v1":   1,
        "a2":   2
}
Connections:
node: v1, node->prev: a2, node->next: a2
node: a2, node->prev: v1, node->next: null


sorted Object:
object=
{
        "a2":   2,
        "v1":   1
}

Connections:
node: a2, node->prev: v1, node->next: v1
node: v1, node->prev: a2, node->next: null

Adding V3:
object=
{
        "a2":   2,
        "v1":   1,
        "v3":   3
}
Connections:
node: a2, node->prev: v3, node->next: v1
node: v1, node->prev: a2, node->next: v3
node: v3, node->prev: v1, node->next: null

Adding d5, a0, z7, k2:
object=
{
        "a2":   2,
        "v1":   1,
        "v3":   3,
        "d5":   5,
        "a0":   0,
        "z7":   7,
        "k2":   2
}
Connections:
node: a2, node->prev: k2, node->next: v1
node: v1, node->prev: a2, node->next: v3
node: v3, node->prev: v1, node->next: d5
node: d5, node->prev: v3, node->next: a0
node: a0, node->prev: d5, node->next: z7
node: z7, node->prev: a0, node->next: k2
node: k2, node->prev: z7, node->next: null


sorted Object:
object=
{
        "a0":   0,
        "a2":   2,
        "d5":   5,
        "k2":   2,
        "v1":   1,
        "v3":   3,
        "z7":   7
}

Connections:
node: a0, node->prev: z7, node->next: a2
node: a2, node->prev: a0, node->next: d5
node: d5, node->prev: a2, node->next: k2
node: k2, node->prev: d5, node->next: v1
node: v1, node->prev: k2, node->next: v3
node: v3, node->prev: v1, node->next: z7
node: z7, node->prev: v3, node->next: null

As seen above, the new sort_list() ensures that the node->prev value is maintained after sorting. Furthermore, now cJSON_AddItemToObject works as expected.

[liamHowatt]

I've encountered it too. @DaveGamble objects cannot be added to after being sorted.

[nathanjjohnson7]

@liamHowatt, does my code fix the issue for you?

[kamnxt]

Just experienced the same issue while trying to use cJSONUtils_GenerateMergePatch, which also uses sorting.

@nathanjjohnson7 Your code fixed the issue, but it didn't compile at first:

cJSON/cJSON_Utils.c:513:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
  513 |     cJSON *last_valid_current_item = list;
      |     ^~~~~

After moving that a few lines up, things seem to work correctly.

I've also written a minimal test:

diff --git a/cJSON_Utils.c b/cJSON_Utils.c
index 8592255..095eab9 100644
--- a/cJSON_Utils.c
+++ b/cJSON_Utils.c
@@ -488,6 +488,7 @@ static cJSON *sort_list(cJSON *list, const cJSON_bool case_sensitive)
     cJSON *current_item = list;
     cJSON *result = list;
     cJSON *result_tail = NULL;
+    cJSON *last_valid_current_item = list;
 
     if ((list == NULL) || (list->next == NULL))
     {
@@ -510,7 +511,6 @@ static cJSON *sort_list(cJSON *list, const cJSON_bool case_sensitive)
     current_item = list;
 
     /*Used to store the last item in list, before current_item becomes null*/
-    cJSON *last_valid_current_item = list;
     while (current_item != NULL)
     {
         /* Walk two pointers to find the middle. */
@@ -577,7 +577,7 @@ static cJSON *sort_list(cJSON *list, const cJSON_bool case_sensitive)
             smaller_prev_storage = smaller->prev;
         }
         else
-        {   
+        {
             /*save smaller->prev to storage before its changed*/
             smaller_prev_storage = smaller->prev;
 
@@ -1530,4 +1530,4 @@ CJSON_PUBLIC(cJSON *) cJSONUtils_GenerateMergePatch(cJSON * const from, cJSON *
 CJSON_PUBLIC(cJSON *) cJSONUtils_GenerateMergePatchCaseSensitive(cJSON * const from, cJSON * const to)
 {
     return generate_merge_patch(from, to, true);
-}
\ No newline at end of file
+}
diff --git a/tests/misc_utils_tests.c b/tests/misc_utils_tests.c
index 7d300bc..2e3e068 100644
--- a/tests/misc_utils_tests.c
+++ b/tests/misc_utils_tests.c
@@ -70,11 +70,27 @@ static void cjson_utils_functions_shouldnt_crash_with_null_pointers(void)
     cJSON_Delete(item);
 }
 
+static void cjson_utils_sort_shouldnt_break_adding(void)
+{
+    cJSON *obj;
+    cJSON *a;
+    obj = cJSON_Parse("{\"c\":\"1\",\"b\":\"1\"}");
+    cJSONUtils_SortObjectCaseSensitive(obj);
+    cJSON_AddStringToObject(obj, "a", "1");
+
+    a = cJSON_GetObjectItemCaseSensitive(obj, "a");
+    TEST_ASSERT_NOT_NULL(a);
+    TEST_ASSERT_EQUAL_STRING("1", a->valuestring);
+}
+
+
+
 int main(void)
 {
     UNITY_BEGIN();
 
     RUN_TEST(cjson_utils_functions_shouldnt_crash_with_null_pointers);
+    RUN_TEST(cjson_utils_sort_shouldnt_break_adding);
 
     return UNITY_END();
 }