From 03781448bf9b32d06a85999cf685554488f29093 Mon Sep 17 00:00:00 2001 From: Juuso Alasuutari Date: Sun, 19 Jan 2025 21:58:23 +0200 Subject: [PATCH 1/5] Revert "Free mem in cjson_set_valuestring_should_return_null_if_strings_overlap" This reverts commit 078c4e6c53f13dff15f0eaac1611abb6379e0206. --- tests/misc_tests.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/misc_tests.c b/tests/misc_tests.c index a96c2fdc..a878fe33 100644 --- a/tests/misc_tests.c +++ b/tests/misc_tests.c @@ -503,7 +503,6 @@ static void cjson_set_valuestring_should_return_null_if_strings_overlap(void) /* If it overlaps, the string will be messed up.*/ TEST_ASSERT_TRUE(strcmp(str, "bcde") == 0); TEST_ASSERT_NULL(str2); - cJSON_Delete(obj); } static void *CJSON_CDECL failing_realloc(void *pointer, size_t size) From dd51beca84e825d5908a16794a1ee8ca0ceafd4a Mon Sep 17 00:00:00 2001 From: Juuso Alasuutari Date: Sun, 19 Jan 2025 21:58:23 +0200 Subject: [PATCH 2/5] Revert "CJSON_SetValuestring: better test for overlapping string" This reverts commit 4f4d7f70c253927c4d8130771168c9fa7864d2d4. --- tests/misc_tests.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/tests/misc_tests.c b/tests/misc_tests.c index a878fe33..bfc3adf8 100644 --- a/tests/misc_tests.c +++ b/tests/misc_tests.c @@ -490,19 +490,15 @@ static void cjson_functions_should_not_crash_with_null_pointers(void) static void cjson_set_valuestring_should_return_null_if_strings_overlap(void) { - cJSON *obj; + cJSON *obj, *obj_dup; char* str; - char* str2; - obj = cJSON_Parse("\"foo0z\""); + obj = cJSON_Parse("\"fooz\""); + obj_dup = cJSON_Duplicate(obj, 1); - str = cJSON_SetValuestring(obj, "abcde"); - str += 1; - /* The string passed to strcpy overlap which is not allowed.*/ - str2 = cJSON_SetValuestring(obj, str); - /* If it overlaps, the string will be messed up.*/ - TEST_ASSERT_TRUE(strcmp(str, "bcde") == 0); - TEST_ASSERT_NULL(str2); + str = cJSON_SetValuestring(obj_dup, "beeez"); + cJSON_SetValuestring(obj_dup, str); + cJSON_SetValuestring(obj_dup, ++str); } static void *CJSON_CDECL failing_realloc(void *pointer, size_t size) From 4d41888f09fe0e078540f3c62313477b5b593d90 Mon Sep 17 00:00:00 2001 From: Juuso Alasuutari Date: Sun, 19 Jan 2025 21:58:23 +0200 Subject: [PATCH 3/5] Revert "CJSON_SetValuestring: add test for overlapping string" This reverts commit b47edc4750301a17bcc72bf20323d2f625a4ae05. --- tests/misc_tests.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/tests/misc_tests.c b/tests/misc_tests.c index bfc3adf8..2fd5ed75 100644 --- a/tests/misc_tests.c +++ b/tests/misc_tests.c @@ -488,19 +488,6 @@ static void cjson_functions_should_not_crash_with_null_pointers(void) cJSON_Delete(item); } -static void cjson_set_valuestring_should_return_null_if_strings_overlap(void) -{ - cJSON *obj, *obj_dup; - char* str; - - obj = cJSON_Parse("\"fooz\""); - obj_dup = cJSON_Duplicate(obj, 1); - - str = cJSON_SetValuestring(obj_dup, "beeez"); - cJSON_SetValuestring(obj_dup, str); - cJSON_SetValuestring(obj_dup, ++str); -} - static void *CJSON_CDECL failing_realloc(void *pointer, size_t size) { (void)size; @@ -796,7 +783,6 @@ int CJSON_CDECL main(void) RUN_TEST(cjson_replace_item_via_pointer_should_replace_items); RUN_TEST(cjson_replace_item_in_object_should_preserve_name); RUN_TEST(cjson_functions_should_not_crash_with_null_pointers); - RUN_TEST(cjson_set_valuestring_should_return_null_if_strings_overlap); RUN_TEST(ensure_should_fail_on_failed_realloc); RUN_TEST(skip_utf8_bom_should_skip_bom); RUN_TEST(skip_utf8_bom_should_not_skip_bom_if_not_at_beginning); From 6d6a650a5f07f9540874f6f29bdd26cc11513876 Mon Sep 17 00:00:00 2001 From: Juuso Alasuutari Date: Sun, 19 Jan 2025 22:13:15 +0200 Subject: [PATCH 4/5] Replace illegal overlapping strings check with memmove() According to the C standard it is Undefined Behavior to arithmetically compare addresses that don't point to within the same object. There is simply no way to implement this sort of check legally. The correct way to avoid problems from overlapping strings is to replace strcpy() with memmove(). --- cJSON.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/cJSON.c b/cJSON.c index d7c72363..8bb52e65 100644 --- a/cJSON.c +++ b/cJSON.c @@ -421,13 +421,7 @@ CJSON_PUBLIC(char*) cJSON_SetValuestring(cJSON *object, const char *valuestring) if (v1_len <= v2_len) { - /* strcpy does not handle overlapping string: [X1, X2] [Y1, Y2] => X2 < Y1 or Y2 < X1 */ - if (!( valuestring + v1_len < object->valuestring || object->valuestring + v2_len < valuestring )) - { - return NULL; - } - strcpy(object->valuestring, valuestring); - return object->valuestring; + return memmove(object->valuestring, valuestring, v1_len + 1); } copy = (char*) cJSON_strdup((const unsigned char*)valuestring, &global_hooks); if (copy == NULL) From 1a30ff038f4f43806e758777623a126f7277da28 Mon Sep 17 00:00:00 2001 From: Juuso Alasuutari Date: Sun, 19 Jan 2025 22:34:55 +0200 Subject: [PATCH 5/5] Optimize cJSON_SetValuestring() somewhat Minor optimizations to cJSON_SetValueString() program flow: - NULL check the input first before going into more detail. - Check the type flags with one bitwise AND instead of two. - Remove a redundant second object->valuestring NULL check. --- cJSON.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/cJSON.c b/cJSON.c index 8bb52e65..1be1a228 100644 --- a/cJSON.c +++ b/cJSON.c @@ -405,13 +405,21 @@ CJSON_PUBLIC(char*) cJSON_SetValuestring(cJSON *object, const char *valuestring) char *copy = NULL; size_t v1_len; size_t v2_len; - /* if object's type is not cJSON_String or is cJSON_IsReference, it should not set valuestring */ - if ((object == NULL) || !(object->type & cJSON_String) || (object->type & cJSON_IsReference)) + + /* input must not be NULL */ + if (valuestring == NULL || object == NULL) { return NULL; } - /* return NULL if the object is corrupted or valuestring is NULL */ - if (object->valuestring == NULL || valuestring == NULL) + + /* destination object must be a string but not a string reference */ + if ((object->type & (cJSON_String | cJSON_IsReference)) != cJSON_String) + { + return NULL; + } + + /* destination object must not be corrupted */ + if (object->valuestring == NULL) { return NULL; } @@ -428,10 +436,8 @@ CJSON_PUBLIC(char*) cJSON_SetValuestring(cJSON *object, const char *valuestring) { return NULL; } - if (object->valuestring != NULL) - { - cJSON_free(object->valuestring); - } + + cJSON_free(object->valuestring); object->valuestring = copy; return copy;