1875797 - AddressSanitizer: heap-use-after-free [@ CustomSerializableObject::ActivityLog::logImpl] with READ of size 8 with makeSerialize

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20240121-1e74418c7ebe (asan-opt build, run with --fuzzing-safe --ion-offthread-compile=off test.js):

a = makeSerializable()
serialize(a, [a])

Backtrace:

==220638==ERROR: AddressSanitizer: heap-use-after-free on address 0x5140000057d0 at pc 0x5572b21b940a bp 0x7ffd3ade0a00 sp 0x7ffd3ade09f8
READ of size 8 at 0x5140000057d0 thread T0
    #0 0x5572b21b9409 in CustomSerializableObject::ActivityLog::logImpl(int, char) /js/src/builtin/TestingFunctions.cpp:5284:11
    #1 0x5572b2070929 in JSStructuredCloneData::discardTransferables() /js/src/vm/StructuredClone.cpp:1121:7
    #2 0x5572b20701de in JSStructuredCloneData::~JSStructuredCloneData() /js/src/vm/StructuredClone.cpp:1026:51
    #3 0x5572b21b3f11 in js_delete<JSStructuredCloneData> /builds/worker/workspace/obj-build/dist/include/js/Utility.h:565:9
    #4 0x5572b21b3f11 in discard /js/src/builtin/TestingFunctions.cpp:5038:5
    #5 0x5572b21b3f11 in CloneBufferObject::Finalize(JS::GCContext*, JSObject*) /js/src/builtin/TestingFunctions.cpp:5196:34
    #6 0x5572b2a5ef20 in doFinalize /builds/worker/workspace/obj-build/dist/include/js/Class.h:649:5
    #7 0x5572b2a5ef20 in finalize /js/src/vm/JSObject-inl.h:99:12
    #8 0x5572b2a5ef20 in unsigned long js::gc::Arena::finalize<JSObject>(JS::GCContext*, js::gc::AllocKind, unsigned long) /js/src/gc/Sweeping.cpp:133:10
    #9 0x5572b2a5e657 in bool FinalizeTypedArenas<JSObject>(JS::GCContext*, js::gc::ArenaList&, js::gc::SortedArenaList&, js::gc::AllocKind, js::SliceBudget&) /js/src/gc/Sweeping.cpp:200:29
    #10 0x5572b2a46041 in js::gc::GCRuntime::foregroundFinalize(JS::GCContext*, JS::Zone*, js::gc::AllocKind, js::SliceBudget&, js::gc::SortedArenaList&) /js/src/gc/Sweeping.cpp:1761:8
    #11 0x5572b2a48234 in js::gc::GCRuntime::finalizeAllocKind(JS::GCContext*, js::SliceBudget&) /js/src/gc/Sweeping.cpp:1962:8
    #12 0x5572b2a6bcf7 in sweepaction::SweepActionForEach<ContainerIter<mozilla::EnumSet<js::gc::AllocKind, unsigned long>>, mozilla::EnumSet<js::gc::AllocKind, unsigned long>>::run(js::gc::SweepAction::Args&) /js/src/gc/Sweeping.cpp:2188:19
    #13 0x5572b2a72ddf in sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) /js/src/gc/Sweeping.cpp:2153:23
    #14 0x5572b2a6b53a in sweepaction::SweepActionForEach<js::gc::SweepGroupZonesIter, JSRuntime*>::run(js::gc::SweepAction::Args&) /js/src/gc/Sweeping.cpp:2188:19
    #15 0x5572b2a72ddf in sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) /js/src/gc/Sweeping.cpp:2153:23
    #16 0x5572b2a6ad88 in sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run(js::gc::SweepAction::Args&) /js/src/gc/Sweeping.cpp:2188:19
    #17 0x5572b2a4bf05 in js::gc::GCRuntime::performSweepActions(js::SliceBudget&) /js/src/gc/Sweeping.cpp:2336:53
    #18 0x5572b2984318 in js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, JS::GCReason, bool) /js/src/gc/GC.cpp:3737:11
    #19 0x5572b2988bb1 in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget const&, JS::GCReason) /js/src/gc/GC.cpp:4248:3
    #20 0x5572b298aac0 in js::gc::GCRuntime::collect(bool, js::SliceBudget const&, JS::GCReason) /js/src/gc/GC.cpp:4439:9
    #21 0x5572b294248b in js::gc::GCRuntime::gc(JS::GCOptions, JS::GCReason) /js/src/gc/GC.cpp:4516:3
    #22 0x5572b1f70cd5 in JSRuntime::destroyRuntime() /js/src/vm/Runtime.cpp:263:8
    #23 0x5572b1d35747 in js::DestroyContext(JSContext*) /js/src/vm/JSContext.cpp:221:7

0x5140000057d0 is located 400 bytes inside of 408-byte region [0x514000005640,0x5140000057d8)
freed by thread T0 here:
    #0 0x5572b162c706 in free /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:52:3
    #1 0x5572b1f70982 in JSRuntime::destroyRuntime() /js/src/vm/Runtime.cpp:214:5
    #2 0x5572b1d35747 in js::DestroyContext(JSContext*) /js/src/vm/JSContext.cpp:221:7

previously allocated by thread T0 here:
    #0 0x5572b162c9ae in malloc /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
    #1 0x5572b21b8a83 in js_arena_malloc /builds/worker/workspace/obj-build/dist/include/js/Utility.h:370:10
    #2 0x5572b21b8a83 in js_malloc /builds/worker/workspace/obj-build/dist/include/js/Utility.h:374:10
    #3 0x5572b21b8a83 in js_new<CustomSerializableObject::ActivityLog> /builds/worker/workspace/obj-build/dist/include/js/Utility.h:530:1
    #4 0x5572b21b8a83 in CustomSerializableObject::ActivityLog::getThreadLog() /js/src/builtin/TestingFunctions.cpp:5264:18
    #5 0x5572b21b72e3 in log /js/src/builtin/TestingFunctions.cpp:5280:14
    #6 0x5572b21b72e3 in log /js/src/builtin/TestingFunctions.cpp:5330:12
    #7 0x5572b21b72e3 in log /js/src/builtin/TestingFunctions.cpp:5333:12
    #8 0x5572b21b72e3 in CustomSerializableObject::CanTransfer(JSContext*, JS::Handle<JSObject*>, bool*, void*) /js/src/builtin/TestingFunctions.cpp:5440:12
    #9 0x5572b2071b22 in JSStructuredCloneWriter::parseTransferable() /js/src/vm/StructuredClone.cpp:1223:12
    #10 0x5572b2068e02 in init /js/src/vm/StructuredClone.cpp:577:12
    #11 0x5572b2068e02 in WriteStructuredClone(JSContext*, JS::Handle<JS::Value>, JSStructuredCloneData*, JS::StructuredCloneScope, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*, JS::Value const&) /js/src/vm/StructuredClone.cpp:752:10
    #12 0x5572b2092c9f in JS_WriteStructuredClone /js/src/vm/StructuredClone.cpp:3907:10
    #13 0x5572b2092c9f in JSAutoStructuredCloneBuffer::write(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) /js/src/vm/StructuredClone.cpp:4028:13
    #14 0x5572b2199c87 in js::testingFunc_serialize(JSContext*, unsigned int, JS::Value*) /js/src/builtin/TestingFunctions.cpp:5621:18
    #15 0x5572b19f9f95 in CallJSNative /js/src/vm/Interpreter.cpp:479:13
    [...]

SUMMARY: AddressSanitizer: heap-use-after-free /js/src/builtin/TestingFunctions.cpp:5284:11 in CustomSerializableObject::ActivityLog::logImpl(int, char)
Shadow bytes around the buggy address:
  0x514000005700: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x514000005780: fd fd fd fd fd fd fd fd fd fd[fd]fa fa fa fa fa
  0x514000005800: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Heap left redzone:       fa
  Freed heap region:       fd

Not sure if this is triggerable outside of the shell but marking s-s until triaged to be safe.

Also, is makeSerializable a new builtin that should be used more during fuzzing?

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Jan de Mooij [:jandem]]

NI sfink because this looks like it could be from bug 1818576.

Comment 4

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20240122123104-f7134e498cbd.
The bug appears to have been introduced in the following build range:

Start: 9d924486b422aecbcf36e84fcb41b89c92c5c66f (20240119223821)
End: 6d2ef7f226a0360135c1955b8805bd41d9af41e4 (20240120000444)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9d924486b422aecbcf36e84fcb41b89c92c5c66f&tochange=6d2ef7f226a0360135c1955b8805bd41d9af41e4

Comment 5

[Steve Fink [:sfink] [:s:]]

Not a security bug, test-only.

It looks like I got my atExit shutdown ordering wrong.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1875797 - wait to run atExit hooks until after final shutdown GC

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24691f8ca5ec
wait to run atExit hooks until after final shutdown GC r=mgaudet

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/24691f8ca5ec

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:sfink, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox123 to wontfix.

For more information, please visit BugBot documentation.

Comment 10

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20240124092956-7079db6b8da9.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 11

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9375849 [details]
Bug 1875797 - wait to run atExit hooks until after final shutdown GC

Beta/Release Uplift Approval Request

• User impact if declined: Crashes, but only when using testing functions that won't be called in regular usage.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Ugh, this was bad timing. I ended up changing the harmless leak from bug 1874785 into a shutdown crash whenever the test function makeSerializable() has been used. It isn't a big deal, but it would be better to have both this patch and bug 1874785's patch in or out together. Having them both in is preferable from a fuzzing perspective, and it's low risk (especially given that it only triggers when that test function is called).
• String changes made/needed: none
• Is Android affected?: Yes

Comment 12

[Pascal Chevrel:pascalc]

Comment on attachment 9375849 [details]
Bug 1875797 - wait to run atExit hooks until after final shutdown GC

Approved for 123 beta 3, thanks.

Comment 13

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/14bb00044055