Heap-use-after-free in delete_finrec_weakref

[Qbtly]

Commit ID

b09ad82

Build platform

Ubuntu 22.04.3

Build steps

cmake -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ -DCONFIG_ASAN=ON -DCMAKE_BUILD_TYPE=Debug
cmake --build . -j $(nproc)

Test case

function main() {
    const registry = new FinalizationRegistry(() => { console.log(console = main()) });
    const token = { a: "v8" };
    registry.register(token, console => {});
    registry.register([]);
    return registry;
}
main();

Execution steps

./qjs poc.js

Output

[object FinalizationRegistry]
=================================================================
==2142064==ERROR: AddressSanitizer: heap-use-after-free on address 0x607000158708 at pc 0x561f729e42a8 bp 0x7fffbdec3890 sp 0x7fffbdec3880
READ of size 8 at 0x607000158708 thread T0
    #0 0x561f729e42a7 in delete_finrec_weakref /quickjs-ng/quickjs.c:52639
    #1 0x561f729e4535 in js_finrec_finalizer /quickjs-ng/quickjs.c:52658
    #2 0x561f7282e506 in free_object /quickjs-ng/quickjs.c:5404
    #3 0x561f7282e7b3 in free_gc_object /quickjs-ng/quickjs.c:5424
    #4 0x561f7282e9a0 in free_zero_refcount /quickjs-ng/quickjs.c:5446
    #5 0x561f7282ed71 in __JS_FreeValueRT /quickjs-ng/quickjs.c:5494
    #6 0x561f7282efa5 in __JS_FreeValue /quickjs-ng/quickjs.c:5523
    #7 0x561f7280b923 in JS_FreeValue /quickjs-ng/quickjs.h:602
    #8 0x561f728a37d9 in JS_CallInternal /quickjs-ng/quickjs.c:17092
    #9 0x561f728a3e52 in JS_Call /quickjs-ng/quickjs.c:17135
    #10 0x561f729e67e0 in reset_weak_ref /quickjs-ng/quickjs.c:52871
    #11 0x561f7282e406 in free_object /quickjs-ng/quickjs.c:5399
    #12 0x561f7282e7b3 in free_gc_object /quickjs-ng/quickjs.c:5424
    #13 0x561f7282e9a0 in free_zero_refcount /quickjs-ng/quickjs.c:5446
    #14 0x561f7282ed71 in __JS_FreeValueRT /quickjs-ng/quickjs.c:5494
    #15 0x561f7282efa5 in __JS_FreeValue /quickjs-ng/quickjs.c:5523
    #16 0x561f7280b923 in JS_FreeValue /quickjs-ng/quickjs.h:602
    #17 0x561f728a3c68 in JS_CallInternal /quickjs-ng/quickjs.c:17125
    #18 0x561f72888635 in JS_CallInternal /quickjs-ng/quickjs.c:15012
    #19 0x561f728a3f60 in JS_CallFree /quickjs-ng/quickjs.c:17142
    #20 0x561f72927aff in JS_EvalFunctionInternal /quickjs-ng/quickjs.c:32786
    #21 0x561f72928f90 in __JS_EvalInternal /quickjs-ng/quickjs.c:32920
    #22 0x561f729291f7 in JS_EvalInternal /quickjs-ng/quickjs.c:32938
    #23 0x561f72929659 in JS_EvalThis /quickjs-ng/quickjs.c:32969
    #24 0x561f7292974b in JS_Eval /quickjs-ng/quickjs.c:32977
    #25 0x561f727dcf23 in eval_buf /quickjs-ng/qjs.c:63
    #26 0x561f727dd2ae in eval_file /quickjs-ng/qjs.c:95
    #27 0x561f727dffdc in main /quickjs-ng/qjs.c:519
    #28 0x7f9996fcf082 in __libc_start_main ../csu/libc-start.c:308
    #29 0x561f727dc87d in _start (/quickjs-ng/qjs+0xd387d)

0x607000158708 is located 40 bytes inside of 72-byte region [0x6070001586e0,0x607000158728)
freed by thread T0 here:
    #0 0x7f999742240f in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:122
    #1 0x561f7280f324 in js_def_free /quickjs-ng/quickjs.c:1717
    #2 0x561f7280d361 in js_free_rt /quickjs-ng/quickjs.c:1383
    #3 0x561f72830c24 in gc_free_cycles /quickjs-ng/quickjs.c:5773
    #4 0x561f72830d14 in JS_RunGC /quickjs-ng/quickjs.c:5789
    #5 0x561f7280d1dc in js_trigger_gc /quickjs-ng/quickjs.c:1365
    #6 0x561f72828bcd in JS_NewObjectFromShape /quickjs-ng/quickjs.c:4714
    #7 0x561f728298aa in JS_NewObjectProtoClass /quickjs-ng/quickjs.c:4833
    #8 0x561f72839d8e in JS_ThrowError2 /quickjs-ng/quickjs.c:6621
    #9 0x561f7283a273 in JS_ThrowError /quickjs-ng/quickjs.c:6653
    #10 0x561f7283a811 in JS_ThrowTypeError /quickjs-ng/quickjs.c:6684
    #11 0x561f72881ead in JS_CallInternal /quickjs-ng/quickjs.c:14634
    #12 0x561f72888db7 in JS_CallInternal /quickjs-ng/quickjs.c:15048
    #13 0x561f728a3e52 in JS_Call /quickjs-ng/quickjs.c:17135
    #14 0x561f729e67e0 in reset_weak_ref /quickjs-ng/quickjs.c:52871
    #15 0x561f7282e406 in free_object /quickjs-ng/quickjs.c:5399
    #16 0x561f7282e7b3 in free_gc_object /quickjs-ng/quickjs.c:5424
    #17 0x561f7282e9a0 in free_zero_refcount /quickjs-ng/quickjs.c:5446
    #18 0x561f7282ed71 in __JS_FreeValueRT /quickjs-ng/quickjs.c:5494
    #19 0x561f7282efa5 in __JS_FreeValue /quickjs-ng/quickjs.c:5523
    #20 0x561f7280b923 in JS_FreeValue /quickjs-ng/quickjs.h:602
    #21 0x561f728a3c68 in JS_CallInternal /quickjs-ng/quickjs.c:17125
    #22 0x561f72888635 in JS_CallInternal /quickjs-ng/quickjs.c:15012
    #23 0x561f728a3f60 in JS_CallFree /quickjs-ng/quickjs.c:17142
    #24 0x561f72927aff in JS_EvalFunctionInternal /quickjs-ng/quickjs.c:32786
    #25 0x561f72928f90 in __JS_EvalInternal /quickjs-ng/quickjs.c:32920
    #26 0x561f729291f7 in JS_EvalInternal /quickjs-ng/quickjs.c:32938
    #27 0x561f72929659 in JS_EvalThis /quickjs-ng/quickjs.c:32969
    #28 0x561f7292974b in JS_Eval /quickjs-ng/quickjs.c:32977
    #29 0x561f727dcf23 in eval_buf /quickjs-ng/qjs.c:63

previously allocated by thread T0 here:
    #0 0x7f9997422808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
    #1 0x561f7280f189 in js_def_malloc /quickjs-ng/quickjs.c:1701
    #2 0x561f7280d2e6 in js_malloc_rt /quickjs-ng/quickjs.c:1378
    #3 0x561f7280d5ab in js_malloc /quickjs-ng/quickjs.c:1416
    #4 0x561f72828bf0 in JS_NewObjectFromShape /quickjs-ng/quickjs.c:4715
    #5 0x561f72829db2 in JS_NewArray /quickjs-ng/quickjs.c:4872
    #6 0x561f72889081 in JS_CallInternal /quickjs-ng/quickjs.c:15066
    #7 0x561f72888635 in JS_CallInternal /quickjs-ng/quickjs.c:15012
    #8 0x561f728a3e52 in JS_Call /quickjs-ng/quickjs.c:17135
    #9 0x561f729e67e0 in reset_weak_ref /quickjs-ng/quickjs.c:52871
    #10 0x561f7282e406 in free_object /quickjs-ng/quickjs.c:5399
    #11 0x561f7282e7b3 in free_gc_object /quickjs-ng/quickjs.c:5424
    #12 0x561f7282e9a0 in free_zero_refcount /quickjs-ng/quickjs.c:5446
    #13 0x561f7282ed71 in __JS_FreeValueRT /quickjs-ng/quickjs.c:5494
    #14 0x561f7282efa5 in __JS_FreeValue /quickjs-ng/quickjs.c:5523
    #15 0x561f7280b923 in JS_FreeValue /quickjs-ng/quickjs.h:602
    #16 0x561f728a3c68 in JS_CallInternal /quickjs-ng/quickjs.c:17125
    #17 0x561f72888635 in JS_CallInternal /quickjs-ng/quickjs.c:15012
    #18 0x561f728a3f60 in JS_CallFree /quickjs-ng/quickjs.c:17142
    #19 0x561f72927aff in JS_EvalFunctionInternal /quickjs-ng/quickjs.c:32786
    #20 0x561f72928f90 in __JS_EvalInternal /quickjs-ng/quickjs.c:32920
    #21 0x561f729291f7 in JS_EvalInternal /quickjs-ng/quickjs.c:32938
    #22 0x561f72929659 in JS_EvalThis /quickjs-ng/quickjs.c:32969
    #23 0x561f7292974b in JS_Eval /quickjs-ng/quickjs.c:32977
    #24 0x561f727dcf23 in eval_buf /quickjs-ng/qjs.c:63
    #25 0x561f727dd2ae in eval_file /quickjs-ng/qjs.c:95
    #26 0x561f727dffdc in main /quickjs-ng/qjs.c:519
    #27 0x7f9996fcf082 in __libc_start_main ../csu/libc-start.c:308

SUMMARY: AddressSanitizer: heap-use-after-free /quickjs-ng/quickjs.c:52639 in delete_finrec_weakref
Shadow bytes around the buggy address:
  0x0c0e80023090: fd fa fa fa fa fa 00 00 00 00 00 00 00 00 00 fa
  0x0c0e800230a0: fa fa fa fa 00 00 00 00 00 00 00 00 00 fa fa fa
  0x0c0e800230b0: fa fa fd fd fd fd fd fd fd fd fd fa fa fa fa fa
  0x0c0e800230c0: 00 00 00 00 00 00 00 00 00 fa fa fa fa fa 00 00
  0x0c0e800230d0: 00 00 00 00 00 00 00 00 fa fa fa fa fd fd fd fd
=>0x0c0e800230e0: fd[fd]fd fd fd fa fa fa fa fa 00 00 00 00 00 00
  0x0c0e800230f0: 00 00 00 00 fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c0e80023100: fd fa fa fa fa fa 00 00 00 00 00 00 00 00 00 fa
  0x0c0e80023110: fa fa fa fa 00 00 00 00 00 00 00 00 00 fa fa fa
  0x0c0e80023120: fa fa fd fd fd fd fd fd fd fd fd fa fa fa fa fa
  0x0c0e80023130: 00 00 00 00 00 00 00 00 00 fa fa fa fa fa 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==2142064==ABORTING

[saghul]

Thanks for the report!

[saghul]

I started to look into this.

Simplified sample code:

let count = 0;
function main() {
    count++;
    const registry = new FinalizationRegistry(() => {
        globalThis.foo = main();
    });
    registry.register([]);
    registry.register([]);
    return registry;
}
main();

console.log(count);

The counter is just as a test.

The problem seems to be some kind of stack overflow I think. Since registering the empty array will immediately trigger the FR callback, due to the use of reference counting, we'll be recursively calling main over and over.

On one of the calls creating the new array will trigger a GC, while we are in the middle of another object collection and I think that's how it all goes to shit.

If I prevent GC from running while already in GC, the script doesn't die, it just runs and eats up all your memory, which I suspect it's expected here?

diff --git a/quickjs.c b/quickjs.c
index 64db3db..a94f2de 100644
--- a/quickjs.c
+++ b/quickjs.c
@@ -1355,7 +1355,7 @@ static void js_trigger_gc(JSRuntime *rt, size_t size)
     force_gc = ((rt->malloc_state.malloc_size + size) >
                 rt->malloc_gc_threshold);
 #endif
-    if (force_gc) {
+    if (force_gc && rt->gc_phase == JS_GC_PHASE_NONE) {
 #ifdef DUMP_GC
         if (check_dump_flag(rt, DUMP_GC)) {
             printf("GC: size=%" PRIu64 "\n",

I worry this is not the right thing to do though!

With V8 the code runs 0 times, because GC is not triggered on the spot, so the FR is likely collected before the empty arrays are.

Calling in the cavalry, @bnoordhuis WDYT?

[bnoordhuis]

I'd say it's reasonable for that test to fail with a stack overflow exception. (UAF is not reasonable, of course.)

FinalizationRegistry makes GC observable and since GC in quickjs amounts to reference counting, the test case is basically an infinite loop.

If we really wanted, we could delay the FinalizationRegistry callback until... function return? next JS_Eval call?... but that may have other undesirable effects and I don't think it's really necessary either. Just don't write code like that :-)

[saghul]

Thanks for the insight, I'm glad to hear I wasn't far off ;-)

I tried to make it error with stack overflow but I don't actually see the recursion on the debugger, not sure it it has to do with byte code handling or something...

[saghul]

Alright, I think I have a fix! #500

MDN says callbacks won't be called synchronously, which they were, and upon inspecting the spec (maybe I should've done that in the first place...) it says: Set finalizationRegistry.[[CleanupCallback]] to [HostMakeJobCallback](https://tc39.es/ecma262/multipage/executable-code-and-execution-contexts.html#sec-hostmakejobcallback)(cleanupCallback). which I understand means the callback will be wrapped to run in the job queue.