misc_tests fails on 1.7.18 under Windows

[blmaier]

The Meson WrapDB project runs cJSON tests with Windows and VisualStudio. On release 1.7.18, misc_tests crashes with SIGinvalid only on the Windows target (https://github.com/mesonbuild/wrapdb/pull/1520). I would guess it's related to this commit that adds an intentional use-after-free.

[Alanscut]

Similar failure was reported with valgrind on linux. Did you turn on any memory check like valgrind or sanitizers?

[dirkmueller]

This happens without valgrind as well, for any libc implementation that is poisoning the memory that has been freed to make exploitation of use-after-free infeasible.

[AdamMajer]

Use-after-free is implementation specific behaviour and should not be relied upon. How well this works will depend on star alignment and mood of the magic smoke in the CPU. (ie. where in the allocated chunk the ptr actually exists and what malloc/free actually does internally)

I don't believe it's feasible to salvage it. And you can't just use realloc() on invalid pointer either.

The test should be reverted.

FWIW, the test is failing in a VM on openSUSE Leap 15.5 but not with Leap 15.5 in a container with kernel from Tumbleweed. Why? Because undefined behaviour.

Finally, this setting of pointer internally to 0 inside of a deleted struct to protect against double-free is questionable because you pass a pointer (not a pointer to pointer) in the delete function and the original one cannot be set to 0. This is actually what probably matters most. Modifying the test a little to delete all allocations and removing the use-after-free bits,

diff --git a/tests/misc_tests.c b/tests/misc_tests.c
index 94dd91a..68d89c6 100644
--- a/tests/misc_tests.c
+++ b/tests/misc_tests.c
@@ -741,11 +741,10 @@ static void deallocated_pointers_should_be_set_to_null(void)
     cJSON *root = cJSON_CreateObject();

     cJSON_Delete(string);
-    free(string->valuestring);

     cJSON_AddObjectToObject(root, "object");
     cJSON_Delete(root->child);
-    free(root->child->string);
+    cJSON_Delete(root);
 #endif
 }

is a double-free and crash.

If the API was cJSON_Delete(&root->child), then we could avoid the double free.

[Alanscut]

I think it would be a better choice to revert this test.