Fix Undefined Behavior by replacing illegal overlap test with memmove()

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;
+    }
+
+    /* destination object must be a string but not a string reference */
+    if ((object->type & (cJSON_String | cJSON_IsReference)) != cJSON_String)
     {
         return NULL;
     }
-    /* return NULL if the object is corrupted or valuestring is NULL */
-    if (object->valuestring == NULL || valuestring == NULL)
+
+    /* destination object must not be corrupted */
+    if (object->valuestring == NULL)
     {
         return NULL;
     }
@@ -421,23 +429,15 @@ 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)
     {
         return NULL;
     }
-    if (object->valuestring != NULL)
-    {
-        cJSON_free(object->valuestring);
-    }
+
+    cJSON_free(object->valuestring);
     object->valuestring = copy;
 
     return copy;

tests/misc_tests.c

@@ -488,24 +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;
-    char* str;
-    char* str2;
-
-    obj =  cJSON_Parse("\"foo0z\"");
-
-    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);
-    cJSON_Delete(obj);
-}
-
 static void *CJSON_CDECL failing_realloc(void *pointer, size_t size)
 {
     (void)size;
@@ -801,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);