quickjs icon indicating copy to clipboard operation
quickjs copied to clipboard

Fuzzing quickjs with LibFuzzer

Open renatahodovan opened this issue 10 months ago • 13 comments

I'm experimenting with fuzzing quickjs with libFuzzer using the fuzz_eval target defined in oss-fuzz. The main concept of libFuzzer is to execute the API of the target application (encapsulated into the LLVMFuzzerTestOneInput function) in an infinite loop and track any non-expected behaviour, like crashes. In case of quickjs, the JS runtime and context objects are initialized in LLVMFuzzerTestOneInput only once as static global objects for performance reasons (I guess) and every test case provided as char arrays are evaled with JS_Eval in them. My question is related to this concept: is it possible that these static global JSRuntime or JSContext are being polluted during fuzzing from one test to the other?

This suspicion raised, since my fuzzer triggered several failures (assertion failures, memory corruptions) that originated from the source tree of quickjs. However, reproducing these failures solely with the latest test case saved by libFuzzer (crash-....) didn't work. Instead, I needed to execute all the previous test cases, too (hundreds of tests). So, is it possible that the execution of a previous test affects subsequent test executions?

I've attempted to address the pollution issue by creating and freeing JSContext and JSRuntime in each iteration (using JS_FreeContext and JSFreeRuntime). However, this approach resulted in a significant decrease in performance (speed) and I'm not even sure if this is the right approach, so I'm seeking guidance from the experts here :)

Note: I've reported this suspicion/question to oss-fuzz as well, but since I'm not sure that the maintainers of quickjs are actively tracking the related issues there, I reported it here, too.

renatahodovan avatar Mar 26 '24 21:03 renatahodovan

I've seen many issues reported by oss-fuzz to quickjs during the years, but I haven't found mirrored reports here for them or any sign of human reproduction. So were these issues reproducible? If they were not, then it might be the sign of the same problem.

renatahodovan avatar Mar 26 '24 21:03 renatahodovan

My question is related to this concept: is it possible that these static global JSRuntime or JSContext are being polluted during fuzzing from one test to the other?

Both the runtime and the context hold state, so this does seem to be an issue in that testing harness setup. (If the goal is to run each test truly independently.)

past-due avatar Mar 26 '24 23:03 past-due

The context keeps track of promise jobs for example, and the runtime keeps track of objects to be garbage collected.

Thus, I think memory corruption issues can certainly affect the subsequent tests and I'd run each of them off a fresh runtime and context.

saghul avatar Mar 27 '24 07:03 saghul

I've created a minimal input corpus with two test cases. One of them creates a variable and the second one tries to print its value. If the test cases were run independently, then the second test should not access the variable defined in the first one. But it still does...

test_1.js

console.log("test1 starts...") var a = "t1var"; console.log("test1 ends...");

test_2.js

console.log("test2 starts ..."); console.log(a); console.log("test2 ends ...");

Execute:

// -prefer_small=1 -shuffle=0 are needed to ensure the ordering of the example inputs

fuzz_eval test/ -runs=0 -prefer_small=1 -shuffle=0

Output:

INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3514420955
INFO: Loaded 1 modules   (31392 inline 8-bit counters): 31392 [0xa1f8d0, 0xa27370),
INFO: Loaded 1 PC tables (31392 PCs): 31392 [0x9325f8,0x9acff8),
INFO:        2 files found in test/
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: seed corpus: files: 2 min: 78b max: 80b total: 158b rss: 33Mb
test1 starts...
test1 ends...
test1 starts...
test1 ends...
test2 starts ...
t1var
test2 ends ...
#4	INITED cov: 569 ft: 732 corp: 2/158b exec/s: 0 rss: 35Mb
#4	DONE   cov: 569 ft: 732 corp: 2/158b lim: 80 exec/s: 0 rss: 35Mb
Done 4 runs in 0 second(s)

The output shows, that the value of a variable (with value t1var) defined in test_1.js is accessible in test_2.js as well.

renatahodovan avatar Mar 27 '24 08:03 renatahodovan

Thus, I think memory corruption issues can certainly affect the subsequent tests and I'd run each of them off a fresh runtime and context.

Is there any way to get a clear context and runtime for every test case which has a lower performance impact than creating and freeing them in every iteration (something like reset, forced gc or... )?

renatahodovan avatar Mar 27 '24 08:03 renatahodovan

I don't think it's possible to do that reliably since a bug might have corrupted the memory.

In general they should be very cheap to create and destroy...

saghul avatar Mar 27 '24 08:03 saghul

Releasing the contexts would also allow for builtin memory leak detection using DUMP_LEAKS.

chqrlie avatar Mar 27 '24 08:03 chqrlie

I don't think it's possible to do that reliably since a bug might have corrupted the memory.

I agree. Yet tests could be run in batches of 1000 and bissected into smaller batches if memory corruption is detected or some other fishy condition.

In general they should be very cheap to create and destroy...

We have tried to minimize this, but many objects still need to be constructed to the overhead is still significant for this kind of testing.

chqrlie avatar Mar 27 '24 08:03 chqrlie

I agree. Yet tests could be run in batches of 1000 and bissected into smaller batches if memory corruption is detected or some other fishy condition.

It could work in other circumstances but libFuzzer only stores a limited set of test cases that were interesting somehow (either reached previously uncovered code or caused a crash). So they cannot be batched here and hence the test cases must be executed truly independently.

I've created a patch (diff pasted below) to recreate and destroy the runtime and context in every iteration. As I mentioned above, it causes a serious performance decrease: ~20x slower test executions on average. But if this is the way to achieve independent test executions, then it's still better than having a target that produces non-reproducible failures only. However, before I upload this patch causing this slowdown to oss-fuzz, it would be nice if someone could verify that the issues reported earlier by oss-fuzz were not reproducible either (I've tried to reproduce a few earlier bugs without success, but I do not have access to the new ones).

60 seconds of fuzzing with one-time initialization:

Done 642948 runs in 61 second(s)
stat::number_of_executed_units: 642948
stat::average_exec_per_sec:     10540
stat::new_units_added:          6981
stat::slowest_unit_time_sec:    0
stat::peak_rss_mb:              844

60 seconds of fuzzing with recreates and destroys:

Done 31400 runs in 61 second(s)
stat::number_of_executed_units: 31400
stat::average_exec_per_sec:     514
stat::new_units_added:          1198
stat::slowest_unit_time_sec:    0
stat::peak_rss_mb:              456

Patch for creating and destroying runtime and context:

diff --git a/projects/quickjs/fuzz_eval.c b/projects/quickjs/fuzz_eval.c
index cef3f441a..b7bee198c 100644
--- a/projects/quickjs/fuzz_eval.c
+++ b/projects/quickjs/fuzz_eval.c
@@ -19,9 +19,6 @@
 #include <stdint.h>
 #include <stdio.h>

-static int initialized = 0;
-JSRuntime *rt;
-JSContext *ctx;
 static int nbinterrupts = 0;

 // handle timeouts from infinite loops
@@ -32,13 +29,15 @@ static int interrupt_handler(JSRuntime *rt, void *opaque)
 }

 int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
-    if (initialized == 0) {
-        rt = JS_NewRuntime();
+    if (Size == 0)
+      return 0;
+
+      JSRuntime *rt = JS_NewRuntime();
         // 64 Mo
         JS_SetMemoryLimit(rt, 0x4000000);
         // 64 Kb
         JS_SetMaxStackSize(rt, 0x10000);
-        ctx = JS_NewContextRaw(rt);
+        JSContext *ctx = JS_NewContextRaw(rt);
         JS_SetModuleLoaderFunc(rt, NULL, js_module_loader, NULL);
         JS_AddIntrinsicBaseObjects(ctx);
         JS_AddIntrinsicDate(ctx);
@@ -53,10 +52,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
         JS_AddIntrinsicBigInt(ctx);
         JS_SetInterruptHandler(JS_GetRuntime(ctx), interrupt_handler, NULL);
         js_std_add_helpers(ctx, 0, NULL);
-        initialized = 1;
-    }

-    if (Size > 0) {
         uint8_t *NullTerminatedData = (uint8_t *)malloc(Size + 1);
         memcpy(NullTerminatedData, Data, Size);
         NullTerminatedData[Size] = 0;
@@ -69,7 +65,8 @@ int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
             js_std_loop(ctx);
             JS_FreeValue(ctx, val);
         }
-    }
+    JS_FreeContext(ctx);
+    JS_FreeRuntime(rt);

     return 0;
 }

renatahodovan avatar Mar 27 '24 09:03 renatahodovan

Whoa that is a lot of them :-) I can try to go through them as time allows.

Any chance those could be mirrored here though?

saghul avatar Mar 27 '24 10:03 saghul

Any chance those could be mirrored here though?

According to the docs, the newly found issues can be mirrored into the GitHub issue tracker with setting the file_github_issue in project.yaml (example).

renatahodovan avatar Mar 27 '24 10:03 renatahodovan

IMHO they should indeed be reported to the upstream project.

saghul avatar Mar 27 '24 10:03 saghul

FYI: I've created a PR into oss-fuzz with some improvements including the recreate and destroy problem discussed above.

renatahodovan avatar Apr 02 '24 23:04 renatahodovan

@renatahodovan I believe the issue can be closed, right?

xeioex avatar May 10 '24 00:05 xeioex

@xeioex yes, it can be closed.

renatahodovan avatar May 10 '24 05:05 renatahodovan