1866518 - Assertion failure: !obj->runtimeFromMainThread()->gc.nursery().isInside(src.dataPointer()), at vm/ArrayBufferObject.cpp:2070

Reporter

Description

•

2 years ago

The following testcase crashes on mozilla-central revision 20231124-dbd1a6aef0a7 (debug build, run with --fuzzing-safe --ion-offthread-compile=off):

evalInWorker(`
  gczeal(14);
  a = [];
  for (b = 0;;) 
    a.push(new ArrayBuffer);
`);

Backtrace:

received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf2dffb40 (LWP 18735)]
0x585e9282 in js::ArrayBufferObject::objectMoved(JSObject*, JSObject*) ()
#0  0x585e9282 in js::ArrayBufferObject::objectMoved(JSObject*, JSObject*) ()
#1  0x588d7a5d in js::gc::ArenaList::relocateArenas(js::gc::Arena*, js::gc::Arena*, js::SliceBudget&, js::gcstats::Statistics&) ()
#2  0x588d9160 in js::gc::ArenaLists::relocateArenas(js::gc::Arena*&, JS::GCReason, js::SliceBudget&, js::gcstats::Statistics&) ()
#3  0x588d5297 in js::gc::GCRuntime::relocateArenas(JS::Zone*, JS::GCReason, js::gc::Arena*&, js::SliceBudget&) ()
#4  0x588d4ec7 in js::gc::GCRuntime::compactPhase(JS::GCReason, js::SliceBudget&, js::gc::AutoGCSession&) ()
#5  0x58900b0e in js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, JS::GCReason, bool) ()
#6  0x58903d1b in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget const&, JS::GCReason) ()
#7  0x589053da in js::gc::GCRuntime::collect(bool, js::SliceBudget const&, JS::GCReason) ()
#8  0x588ce430 in js::gc::GCRuntime::gc(JS::GCOptions, JS::GCReason) ()
#9  0x588ceb57 in js::gc::GCRuntime::runDebugGC() ()
#10 0x588ccecc in bool js::gc::CellAllocator::PreAllocChecks<(js::AllowGC)1>(JSContext*, js::gc::AllocKind) ()
#11 0x57f205d6 in void* js::gc::CellAllocator::AllocNurseryOrTenuredCell<(JS::TraceKind)0, (js::AllowGC)1>(JSContext*, js::gc::AllocKind, unsigned int, js::gc::Heap, js::gc::AllocSite*) ()
#12 0x57f204b7 in js::NativeObject* js::gc::CellAllocator::NewObject<js::NativeObject, (js::AllowGC)1>(JSContext*, js::gc::AllocKind, js::gc::Heap, JSClass const*, js::gc::AllocSite*) ()
#13 0x57f1f5cf in js::NativeObject::create(JSContext*, js::gc::AllocKind, js::gc::Heap, JS::Handle<js::SharedShape*>, js::gc::AllocSite*) ()
#14 0x585e5371 in NewArrayBufferObject(JSContext*, JS::Handle<JSObject*>, js::gc::AllocKind) ()
#15 0x585e5d44 in std::tuple<js::ArrayBufferObject*, unsigned char*> js::ArrayBufferObject::createBufferAndData<(js::ArrayBufferObject::FillContents)0>(JSContext*, unsigned int, js::AutoSetNewObjectMetadata&, JS::Handle<JSObject*>) ()
#16 0x585de023 in js::ArrayBufferObject::createZeroed(JSContext*, unsigned int, JS::Handle<JSObject*>) ()
#17 0x585dde2d in js::ArrayBufferObject::class_constructor(JSContext*, unsigned int, JS::Value*) ()
#18 0x27bd83c7 in ?? ()
#19 0x27bcc5a2 in ?? ()
#20 0x27ba87ed in ?? ()
#21 0x58ae071f in js::jit::EnterBaselineInterpreterAtBranch(JSContext*, js::InterpreterFrame*, unsigned char*) ()
#22 0x57f8b149 in js::Interpret(JSContext*, js::RunState&) ()
#23 0x57f829af in MaybeEnterInterpreterTrampoline(JSContext*, js::RunState&) ()
#24 0x57f8250f in js::RunScript(JSContext*, js::RunState&) ()
#25 0x57f86006 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) ()
#26 0x57f865aa in js::Execute(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) ()
#27 0x580f29a8 in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) ()
#28 0x580f2783 in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) ()
#29 0x57e088e6 in WorkerMain(mozilla::UniquePtr<WorkerInput, JS::DeletePolicy<WorkerInput> >) ()
#30 0x57e08efb in js::detail::ThreadTrampoline<void (&)(mozilla::UniquePtr<WorkerInput, JS::DeletePolicy<WorkerInput> >), mozilla::UniquePtr<WorkerInput, JS::DeletePolicy<WorkerInput> > >::Start(void*) ()
#31 0x57e51942 in set_alt_signal_stack_and_start(PthreadCreateParams*) ()
#32 0xf7fb226a in start_thread (arg=0xf2dffb40) at pthread_create.c:333
#33 0xf7c8e41e in clone () from /lib32/libc.so.6
eax	0x567a78bb	1450866875
ebx	0x598cd1b8	1502400952
ecx	0x598ceddc	1502408156
edx	0x0	0
esi	0xf23fffd0	-230686768
edi	0x0	0
ebp	0xf2dfdf58	4074757976
esp	0xf2dfdf20	4074757920
eip	0x585e9282 <js::ArrayBufferObject::objectMoved(JSObject*, JSObject*)+834>
=> 0x585e9282 <_ZN2js17ArrayBufferObject11objectMovedEP8JSObjectS2_+834>:	movl   $0x816,0x0
   0x585e928c <_ZN2js17ArrayBufferObject11objectMovedEP8JSObjectS2_+844>:	call   0x57e51230 <abort>

Seems to be 32-bit only, likely something involving OOM.

Christian Holler (:decoder)

Reporter

Comment 1

•

2 years ago

Attached file Detailed Crash Information — Details

Christian Holler (:decoder)

Reporter

Comment 2

•

2 years ago

Attached file Testcase — Details

Bugmon [:jkratzer for issues]

Comment 3

•

2 years ago

Unable to reproduce bug 1866518 using build mozilla-central 20231124092418-dbd1a6aef0a7. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Keywords: bugmon

Andrew McCreight [:mccr8]

Updated

•

2 years ago

Keywords: csectype-uaf

Steven DeTar [:sdetar]

Comment 4

•

2 years ago

Steve, could you take look at this when you have a chance.

Severity: -- → S3

Flags: needinfo?(sphink)

Priority: -- → P3

Daniel Veditz [:dveditz]

Updated

•

2 years ago

Keywords: sec-high

Steven DeTar [:sdetar]

Comment 5

•

2 years ago

Changing Severity/priority since this now marked Sec-High

Severity: S3 → S2

Priority: P3 → P1

Donal Meehan [:dmeehan]

Updated

•

2 years ago

tracking-firefox122: --- → +

Donal Meehan [:dmeehan]

Comment 6

•

2 years ago

Not sure about the regressor, setting it as Bug 1862530 but :sdetar/:sfink if you could confirm?

status-firefox120: --- → unaffected

status-firefox121: --- → affected

status-firefox-esr115: --- → unaffected

tracking-firefox121: --- → +

Regressed by: 1862530

Steve Fink [:sfink] [:s:]

Assignee

Comment 7

•

2 years ago

Recently-added assertion that is checking more than it wants to.

What's going on is that we have an empty ArrayBuffer that gets compacted to a memory location that happens to land right before the nursery. That means that its dataPointer() (which is its inlineDataPointer() since it's zero-length) points to the very beginning of the nursery. There's no assertion for this, so it continues on. A later compaction then moves it to another location, but this time there's an assertion that the source ABO's data pointer is not inside the nursery, and that assertion fails. But it's spurious, because even though the pointer points to the nursery, it's pointing to zero bytes of data, so it's fine.

Fuzzers gotta fuzz.

Flags: needinfo?(sphink)

Regressed by: 1859335
No longer regressed by: 1862530

Steve Fink [:sfink] [:s:]

Assignee

Updated

•

2 years ago

Group: javascript-core-security

Andrew McCreight [:mccr8]

Updated

•

2 years ago

Keywords: csectype-uaf, sec-high

BugBot [:suhaib / :marco/ :calixte]

Comment 8

•

2 years ago

:jonco, since you are the author of the regressor, bug 1859335, could you take a look?

For more information, please visit BugBot documentation.

Flags: needinfo?(jcoppeard)

Steve Fink [:sfink] [:s:]

Assignee

Comment 9

•

2 years ago

BugBot, leave jonco alone. ;-) I have a patch, but I'm looking for more places that might need it first.

Flags: needinfo?(jcoppeard)

Andrew McCreight [:mccr8]

Comment 10

•

2 years ago

It turned out that this is just an overly strict assertion, not a security issue, so I don't think it needs to be tracked.

tracking-firefox121: + → ---

tracking-firefox122: + → ---

Steve Fink [:sfink] [:s:]

Assignee

Comment 11

•

2 years ago

Ok, I'll pester Jon instead. Jon, this is trickier than I expected. I made a Nursery::isInside(void*, size_t) to return false for a zero-length region, passing in the length where it made sense, but that revealed that we have both situations: in the above case, we have a zero-length pointer to the beginning of the nursery that logically belongs with a Cell that immediately precedes the nursery. But in another case, we have a zero-length nursery buffer that is being used for a different ArrayBuffer (I'm not sure it started out as zero-length, it might have gotten detached). So for that, a pointer to a zero-length region at the beginning of the nursery should return true.

Currently this can definitely happen to ArrayBuffers. I don't think it can happen to JSStrings, since dependent strings will never point to inline base strings' chars. I'm not sure if it can happen with NativeObject or not. BigInt would be another one to worry about.

Options are:

disallow using the final Cell, Arena, or Chunk before a nursery Chunk
disallow inline pointers past the end of a Cell's memory, or at pointers to zero-length regions
skip the first CellAlignBytes of every nursery Chunk when allocating a buffer
make every isInline caller specify what a pointer to the beginning of the nursery means

I just thought of that 3rd one while writing this, so I think I'll try that first.

Jon Coppeard (:jonco)

Comment 12

•

2 years ago

(In reply to Steve Fink [:sfink] [:s:] from comment #11)

skip the first CellAlignBytes of every nursery Chunk when allocating a buffer

This should happen already since there's a header at the start of the chunk.

disallow inline pointers past the end of a Cell's memory, or at pointers to zero-length regions

There have been problems with this before so I'm sympathetic to this idea but I guess it's simpler to have a pointer to a zero length region than have a possibly-null pointer and have to check it every time.

Steve Fink [:sfink] [:s:]

Assignee

Comment 13

•

2 years ago

Oops, forgot to post this last night:

Uh, I'm an idiot. We already disallow using the beginning of a NurseryChunk, or any Chunk for that matter. There's a header. I'll have to go back and see why I was getting a pointer to there.

I'm not sure why I was seeing what seemed to be a nursery pointer to the beginning of the nursery, but it was as part of a incomplete patch so I was probably just messing something up. The actual fix is trivial and seems to resolve the problem. I still need to figure out how to land a test, since the fuzz test takes over 2 minutes to eventually OOM.

disallow inline pointers past the end of a Cell's memory, or at pointers to zero-length regions

There have been problems with this before so I'm sympathetic to this idea but I guess it's simpler to have a pointer to a zero length region than have a possibly-null pointer and have to check it every time.

I would probably have it point to a shared empty space rather than a nullptr. But it still seems like a nonobvious restriction.

Steve Fink [:sfink] [:s:]

Assignee

Comment 14

•

2 years ago

Attached file Bug 1866518 - Pointers to the beginning of a NurseryChunk should not be automatically considered to be in the nursery — Details

Phabricator Automation

Updated

•

2 years ago

Assignee: nobody → sphink

Status: NEW → ASSIGNED

Pulsebot

Comment 15

•

2 years ago

Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8038f0b90bbd Pointers to the beginning of a NurseryChunk should not be automatically considered to be in the nursery r=jonco

Norisz Fay [:noriszfay]

Comment 16

•

2 years ago

•

Edited

Backed out for causing SM bustages on bug1866518-nursery-AB.js

Backout link

Push with failures

Failure log

Jit test failure log. Assertion failure log

Flags: needinfo?(sphink)

Pulsebot

Comment 17

•

2 years ago

Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8a70af1e5277 Pointers to the beginning of a NurseryChunk should not be automatically considered to be in the nursery r=jonco

Natalia Csoregi [:nataliaCs]

Comment 18

•

2 years ago

Backed out for causing assertions on Nursery-inl.h

[task 2023-12-05T00:54:35.530Z] TEST-PASS | js/src/jit-test/tests/gc/pretenure-array-long-then-short-lived.js | Success (code 0, args "") [8.4 s]
[task 2023-12-05T00:54:36.031Z] Assertion failure: isInside(oldData), at /builds/worker/checkouts/gecko/js/src/gc/Nursery-inl.h:82
[task 2023-12-05T00:54:36.031Z] #01: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x1db953c]
[task 2023-12-05T00:54:36.031Z] #02: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x2356217]
[task 2023-12-05T00:54:36.031Z] #03: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x2351c74]
[task 2023-12-05T00:54:36.031Z] #04: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x235155e]
[task 2023-12-05T00:54:36.031Z] #05: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x2352f5f]
[task 2023-12-05T00:54:36.031Z] #06: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x2354a6c]
[task 2023-12-05T00:54:36.031Z] #07: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x2353963]
[task 2023-12-05T00:54:36.031Z] #08: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x235370d]
[task 2023-12-05T00:54:36.031Z] #09: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x2313152]
[task 2023-12-05T00:54:36.031Z] #10: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x230fdb8]
[task 2023-12-05T00:54:36.031Z] #11: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x230f1ed]
[task 2023-12-05T00:54:36.031Z] #12: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x22950b9]
[task 2023-12-05T00:54:36.031Z] #13: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x22618c7]
[task 2023-12-05T00:54:36.031Z] #14: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x22610ba]
[task 2023-12-05T00:54:36.031Z] #15: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x193eea4]
[task 2023-12-05T00:54:36.031Z] #16: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x1f31277]
[task 2023-12-05T00:54:36.031Z] #17: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x1f30a57]
[task 2023-12-05T00:54:36.031Z] #18: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x1f30961]
[task 2023-12-05T00:54:36.031Z] #19: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x25d57d9]
[task 2023-12-05T00:54:36.031Z] #20: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x25d1de7]
[task 2023-12-05T00:54:36.031Z] #21: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x25dc51b]
[task 2023-12-05T00:54:36.031Z] #22: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x25dcc40]
[task 2023-12-05T00:54:36.031Z] #23: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x245ed40]
[task 2023-12-05T00:54:36.031Z] #24: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x1910abd]
[task 2023-12-05T00:54:36.031Z] #25: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x1909cff]
[task 2023-12-05T00:54:36.031Z] #26: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x190985f]
[task 2023-12-05T00:54:36.031Z] #27: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x190d376]
[task 2023-12-05T00:54:36.031Z] #28: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x190d8ba]
[task 2023-12-05T00:54:36.031Z] #29: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x1a7b428]
[task 2023-12-05T00:54:36.031Z] #30: JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>)[/builds/worker/workspace/obj-spider/dist/bin/js +0x1a7b5fe]
[task 2023-12-05T00:54:36.031Z] #31: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x187311f]
[task 2023-12-05T00:54:36.031Z] #32: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x1872353]
[task 2023-12-05T00:54:36.031Z] #33: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x180cad5]
[task 2023-12-05T00:54:36.031Z] #34: ???[/builds/worker/workspace/obj-spider/dist/bin/js +0x1806d86]
[task 2023-12-05T00:54:36.031Z] Exit code: -11
[task 2023-12-05T00:54:36.031Z] FAIL - gc/pretenuring.js
[task 2023-12-05T00:54:36.031Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/gc/pretenuring.js | Assertion failure: isInside(oldData), at /builds/worker/checkouts/gecko/js/src/gc/Nursery-inl.h:82 (code -11, args "") [0.7 s]

Ryan VanderMeulen [:RyanVM]

Updated

•

2 years ago

Severity: S2 → S3

status-firefox121: affected → wontfix

status-firefox122: affected → fix-optional

Steve Fink [:sfink] [:s:]

Assignee

Comment 19

•

2 years ago

(In reply to Jon Coppeard (:jonco) from comment #12)

(In reply to Steve Fink [:sfink] [:s:] from comment #11)

skip the first CellAlignBytes of every nursery Chunk when allocating a buffer

This should happen already since there's a header at the start of the chunk.

Unfortunately, this isn't enough to fix the problem. A pointer just past the end of a nursery chunk is ambiguous as to whether it is in the nursery or not. The above change of taking the header into account removes the problem of incorrectly treating a pointer to the beginning of a nursery chunk as being in the nursery, since there is no valid data there until you get past the header. But you could answer the above question as either "no, not inside the nursery, it's pointing to the beginning of a malloced page" or "yes, inside the nursery, it's a zero-length pointer just past the end of a nursery cell". And both are used in practice. In order for the header to help, we would need a header on all non-nursery chunks as well.

I guess the straightforward fix is to add a nursery chunk footer as well. :-( I'll give that a try.

BugBot [:suhaib / :marco/ :calixte]

Comment 20

•

1 year ago

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:sfink, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Flags: needinfo?(sphink)

Flags: needinfo?(jcoppeard)

Jon Coppeard (:jonco)

Comment 21

•

1 year ago

The patch is not r+ed.

Flags: needinfo?(sphink)

Flags: needinfo?(jcoppeard)

Donal Meehan [:dmeehan]

Updated

•

1 year ago

status-firefox122: fix-optional → wontfix

Steve Fink [:sfink] [:s:]

Assignee

Comment 22

•

1 year ago

Jon nerfed the assertion in bug 1892564.

Status: ASSIGNED → RESOLVED

Closed: 1 year ago

Duplicate of bug: 1892564

Flags: needinfo?(sphink)

Resolution: --- → DUPLICATE

Detailed Crash Information 2 years ago Christian Holler (:decoder) 4.03 KB, text/plain		Details
Testcase 2 years ago Christian Holler (:decoder) 88 bytes, text/plain		Details
Bug 1866518 - Pointers to the beginning of a NurseryChunk should not be automatically considered to be in the nursery 2 years ago Steve Fink [:sfink] [:s:] 48 bytes, text/x-phabricator-request		Details \| Review