Closed Bug 1424732 Opened 2 years ago Closed 2 years ago

Crash [@ std::__atomic_base<unsigned int>::load] or Assertion failure: isInt32(), at js/Value.h:608 with Debugger and wasm

Categories

(Core :: JavaScript Engine, defect, P2, critical)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: decoder, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(1 file)

The following testcase crashes on mozilla-central revision e9262199b3a7 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe):

const root = newGlobal();
root.eval("this.dbg = new Debugger()");
root.dbg.addDebuggee(this);
root.dbg.memory.trackingAllocationSites = true;
let maxSize = 10;
let memory = new WebAssembly.Memory({
    initial: 1,
    maximum: maxSize,
    shared: true
});


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00000000009df38d in std::__atomic_base<unsigned int>::load (__m=std::memory_order_acquire, this=<optimized out>) at /usr/include/c++/5/bits/atomic_base.h:396
396		return __atomic_load_n(&_M_i, __m);
#0  0x00000000009df38d in std::__atomic_base<unsigned int>::load (__m=std::memory_order_acquire, this=<optimized out>) at /usr/include/c++/5/bits/atomic_base.h:396
#1  mozilla::detail::IntrinsicMemoryOps<unsigned int, (mozilla::MemoryOrdering)1>::load (aPtr=...) at mozilla/Atomics.h:196
#2  mozilla::detail::AtomicBaseIncDec<unsigned int, (mozilla::MemoryOrdering)1>::operator unsigned int (this=<optimized out>) at mozilla/Atomics.h:369
#3  js::SharedArrayRawBuffer::refcount (this=<optimized out>) at js/src/vm/SharedArrayObject.h:155
#4  js::SharedArrayBufferObject::addSizeOfExcludingThis (obj=<optimized out>, mallocSizeOf=<optimized out>, info=info@entry=0x7fffffffc980) at js/src/vm/SharedArrayObject.cpp:341
#5  0x0000000000879aa4 in JSObject::addSizeOfExcludingThis (this=<optimized out>, mallocSizeOf=<optimized out>, info=info@entry=0x7fffffffc980) at js/src/jsobj.cpp:3907
#6  0x0000000000879da8 in JS::ubi::Concrete<JSObject>::size (this=this@entry=0x7fffffffca90, mallocSizeOf=<optimized out>) at js/src/jsobj.cpp:3956
#7  0x000000000094824e in JS::ubi::Node::size (mallocSizeof=<optimized out>, this=0x7fffffffca90) at js/UbiNode.h:791
#8  js::Debugger::appendAllocationSite (this=0x7ffff5f39800, cx=cx@entry=0x7ffff5f15000, obj=obj@entry=..., frame=..., frame@entry=..., when=..., when@entry=...) at js/src/vm/Debugger.cpp:2291
#9  0x0000000000948a6c in js::Debugger::slowPathOnLogAllocationSite (cx=cx@entry=0x7ffff5f15000, obj=..., obj@entry=..., frame=frame@entry=..., when=when@entry=..., dbgs=...) at js/src/vm/Debugger.cpp:2254
#10 0x00000000009d2569 in js::Debugger::onLogAllocationSite (when=..., frame=..., obj=0x7ffff4381190, cx=0x7ffff5f15000) at js/src/vm/Debugger.h:1823
#11 js::SavedStacks::MetadataBuilder::build (this=<optimized out>, cx=0x7ffff5f15000, target=..., oomUnsafe=...) at js/src/vm/SavedStacks.cpp:1648
#12 0x000000000080f3f8 in JSCompartment::setNewObjectMetadata (this=0x7ffff5f2b000, cx=0x7ffff5f15000, obj=obj@entry=...) at js/src/jscompartment.cpp:1133
#13 0x000000000080f676 in js::SetNewObjectMetadata<JSObject> (obj=0x7ffff4381190, cx=<optimized out>) at js/src/jsobjinlines.h:389
#14 js::AutoSetNewObjectMetadata::~AutoSetNewObjectMetadata (this=0x7fffffffcd40, __in_chrg=<optimized out>) at js/src/jscompartment.cpp:1494
#15 0x00000000009f9cab in js::SharedArrayBufferObject::createEmpty (cx=cx@entry=0x7ffff5f15000) at js/src/vm/SharedArrayObject.cpp:364
#16 0x00000000008e0f91 in CreateBuffer<js::SharedArrayBufferObject, js::SharedArrayRawBuffer> (maybeSharedObject=..., maxSize=..., initialSize=65536, cx=0x7ffff5f15000) at js/src/vm/ArrayBufferObject.cpp:803
#17 js::CreateWasmBuffer (cx=cx@entry=0x7ffff5f15000, memory=..., buffer=buffer@entry=...) at js/src/vm/ArrayBufferObject.cpp:886
#18 0x0000000000b2ca96 in js::WasmMemoryObject::construct (cx=0x7ffff5f15000, argc=1, vp=0x7ffff40ae090) at js/src/wasm/WasmJS.cpp:1313
#19 0x000000000052158f in js::CallJSNative (args=..., native=<optimized out>, cx=0x7ffff5f15000) at js/src/jscntxtinlines.h:291
[...]
#32 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9040
rax	0x0	0
rbx	0x7ffff4381000	140737290702848
rcx	0x1bebd00	29277440
rdx	0xfff2000000000000	-3940649673949184
rsi	0x7fffffffc980	140737488341376
rdi	0x7ffff43811b0	140737290703280
rbp	0x1	1
rsp	0x7fffffffc978	140737488341368
r8	0x15820	88096
r9	0x560	1376
r10	0x200000000	8589934592
r11	0x7ffff5fd7d00	140737320418560
r12	0x7ffff5f39800	140737319770112
r13	0x7fffffffcbe0	140737488341984
r14	0x7ffff5f2b000	140737319710720
r15	0x9899a93b96e79c	42953148588287900
rip	0x9df38d <js::SharedArrayBufferObject::addSizeOfExcludingThis(JSObject*, unsigned long (*)(void const*), JS::ClassInfo*)+45>
=> 0x9df38d <js::SharedArrayBufferObject::addSizeOfExcludingThis(JSObject*, unsigned long (*)(void const*), JS::ClassInfo*)+45>:	mov    (%rdx),%ecx
   0x9df38f <js::SharedArrayBufferObject::addSizeOfExcludingThis(JSObject*, unsigned long (*)(void const*), JS::ClassInfo*)+47>:	xor    %edx,%edx
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, failed due to error (try manually).
Luke, could you take a look?
Flags: needinfo?(luke)
Priority: -- → P2
Lars, perhaps you could take a look?
Flags: needinfo?(luke) → needinfo?(lhansen)
The problem is that the debugger observes an object that is in the process of being created.  At this point its private slots are UNDEFINED, but addSizeOfExcludingThis() does not consider this eventuality.

We can fix that by having the default slot value for the bytelength slot be 0 and not undefined, or by adding guards and hope we covered all cases.  I'll investigate.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Flags: needinfo?(lhansen)
So I was looking at ArrayBufferObject.cpp:CreateBuffer which will allocate an array buffer object, then allocate its memory (and init these slots and stuff).  Is there a reason allocating the memory can't happen first, then allocating the (S)ABO happen second with the initing performed by SABO::createEmpty (which would be renamed, but whatever)?  It's just bad to allocate an object, then have it be exposed, then finish initializing it.
Recording a discussion with Waldo:

The reason we allocate the object first and then the memory has to do with the semantics of growing the memory.  If the grow fails we want to return an error code, not an oom, so if we were to grow the memory first (in the case where we do that) and then the object, we would have to back out the grow if the object allocation fails.  A comment in ArrayBufferObject::wasmGrowToSizeInPlace() explains this.

In principle we could maybe make that a revertible operation but it feels like that would put the complexity in the right position.  IMO it is better if we do something special around the observation of the empty object so that we avoid the problem reported in this bug.

BTW the fix I proposed earlier, where the bytelength slot is initially 0, is not quite sufficient, because addSizeOfExcludingThis reads both bytelength and the buffer object (it wants to divide the bytelength by the refcount).  We really need some proper kind of guard here.
The cleanest thing might be to rename createEmpty as createEmptyNoAccounting or createObjectWithoutBuffer and then lift the accounting into the three callers, with adequate commenting to prevent abuse.  And/or maybe force the function to take an AutoSetNewObjectMetadata& cookie to ensure that the caller has such accounting in place.
This seems to do the trick for me.  It places the initialization-for-rawbuf into code before the new (S)ABO is exposed, which is all we really need here.  Also has the benefit of moving initialization steps closer to where the raw (S)ABO is merely allocated.

It would be nice to do the further steps of cleaning up the grow-in-place code, maybe, to avoid separating allocation and init-with-raw-buffer.  But it's not necessary here, and that trickier case doesn't have the same issue as is reported in this bug.

Or so it appears to me.
Attachment #8937169 - Flags: review?(lhansen)
Assignee: lhansen → jwalden+bmo
Comment on attachment 8937169 [details] [diff] [review]
Make CreateBuffer allocate the raw buffer, then create the array buffer object around it

Review of attachment 8937169 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK, but do remove dead code flagged in comments below.

::: js/src/vm/ArrayBufferObject.h
@@ +291,5 @@
>  
>      // Create an ArrayBufferObject that is safely finalizable and can later be
>      // initialize()d to become a real, content-visible ArrayBufferObject.
>      static ArrayBufferObject* createEmpty(JSContext* cx);
>  

While createEmpty remains used here, initializeRawBuffer() can be removed.

::: js/src/vm/SharedArrayObject.h
@@ +252,5 @@
>      // WebAssembly support:
>  
>      // Create a SharedArrayBufferObject without an attached SARB.  This is a
>      // valid object only if a SARB is then attached with initializeRawBuffer().
>      static SharedArrayBufferObject* createEmpty(JSContext* cx);

createEmpty is now unused and should be removed here and in the cpp file; after all it's ill-defined.

@@ +264,2 @@
>      // Install the buffer if the object was created by createEmpty().
>      void initializeRawBuffer(JSContext* cx, SharedArrayRawBuffer* buffer, uint32_t length);

initializeRawBuffer can be removed here and in the cpp file.
Attachment #8937169 - Flags: review?(lhansen) → review+
Are we able to land this now?
Flags: needinfo?(jwalden+bmo)
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f7574c4a92
When creating a wasm buffer, fully initialize the buffer object before exposing it to the rest of the JS engine (in particularly, to memory-accounting code).  r=lth
https://hg.mozilla.org/mozilla-central/rev/e1f7574c4a92
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Landed -- sorry, patch buried underneath a series of other patches in-flight, didn't seem worth the pain to dig it out to land alone.
Flags: needinfo?(jwalden+bmo)
You need to log in before you can comment on or make changes to this bug.