[go: nahoru, domu]

Open Bug 1866518 Opened 9 months ago Updated 7 months ago

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

Categories

(Core :: JavaScript: GC, defect, P1)

x86
Linux
defect

Tracking

()

ASSIGNED
Tracking Status
firefox-esr115 --- unaffected
firefox120 --- unaffected
firefox121 --- wontfix
firefox122 --- wontfix

People

(Reporter: decoder, Assigned: sfink, NeedInfo)

References

(Regression)

Details

(4 keywords, Whiteboard: [bugmon:update,bisect])

Attachments

(3 files)

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.

Attached file Testcase

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

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

Severity: -- → S3
Flags: needinfo?(sphink)
Priority: -- → P3

Changing Severity/priority since this now marked Sec-High

Severity: S3 → S2
Priority: P3 → P1

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

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
Group: javascript-core-security

: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)

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

Flags: needinfo?(jcoppeard)

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.

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.

(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.

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.

Assignee: nobody → sphink
Status: NEW → ASSIGNED
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

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)
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

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]

(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.

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)

The patch is not r+ed.

Flags: needinfo?(sphink)
Flags: needinfo?(jcoppeard)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: