Closed Bug 1461027 Opened 3 years ago Closed 3 years ago

Assertion failure: nslots == numFixedSlots() + (hasPrivate() ? 1 : 0), at js/src/vm/NativeObject-inl.h:36

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: gkw, Assigned: jonco)

References

Details

(4 keywords, Whiteboard: [jsbugmon:][adv-main62+])

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 4303d49c5393 (build with --32 --enable-debug, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

// Adapted from randomly chosen test: js/src/jit-test/tests/ion/inlining/TypedObject-TypeDescrIsSimpleType.js
for (var i = 0; i < 99; i++) {
	    w = new TypedObject.ArrayType(TypedObject.int32, 100).build(function() {});
}
// jsfunfuzz-generated
relazifyFunctions();

Backtrace:

#0  0x56d48f16 in js::NativeObject::fixedData (this=0xf6a77070, nslots=4) at js/src/vm/NativeObject-inl.h:36
#1  0x56d2f2f0 in js::ArrayBufferObject::inlineDataPointer (this=0xf6a77070) at js/src/vm/ArrayBufferObject.cpp:952
#2  0x56b55359 in js::ArrayBufferObject::hasInlineData (this=0xf6a77070) at js/src/vm/ArrayBufferObject.h:372
#3  js::OutlineTypedObject::obj_trace (trc=0xffee093c, object=0xf6a90020) at js/src/builtin/TypedObject.cpp:1628
#4  0x56e2190b in js::Class::doTrace (this=<optimized out>, obj=0xf6a90020, trc=0xffee093c) at /home/ubuntu/shell-cache/js-dbg-32-linux-4303d49c5393/objdir-js/dist/include/js/Class.h:869
/snip

For detailed crash information, see attachment.

ArrayBuffers are on the stack, setting s-s as a start.
This was compiled with:

'sh' '/home/ubuntu/trees/mozilla-central/js/src/configure' '--target=i686-pc-linux' '--enable-debug' '--with-ccache' '--enable-gczeal' '--enable-debug-symbols' '--disable-tests'

python -u -m funfuzz.js.compile_shell -b "--32 --enable-debug" -r 4303d49c5393
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9b7cc103ce95
user:        Jon Coppeard
date:        Thu May 10 10:09:27 2018 +0100
summary:     Bug 1457703 - Don't fixup an associated object's shape when updating moved pointers in another object r=sfink a=abillings

Jon, is bug 1457703 a likely regressor?
Blocks: 1457703
Flags: needinfo?(jcoppeard)
Yes, it is.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Fix another place we can call numFixedSlots() on an associated object while traching during a moving GC.
Attachment #8975440 - Flags: review?(sphink)
Comment on attachment 8975440 [details] [diff] [review]
bug1461027-fixed-data

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

I used my callgraph traversal to look for more instances. Here's one:

  #178229 = void js::ArrayBufferViewObject::trace(JSTracer*, JSObject*)
  #15471 = void* js::NativeObject::getPrivate() const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

similarly

  #178229 = void js::ArrayBufferViewObject::trace(JSTracer*, JSObject*)
  #15475 = void js::NativeObject::setPrivateUnbarriered(void*)
  #15390 = uint32 js::NativeObject::numFixedSlots() const

and while we're there,

  #178229 = void js::ArrayBufferViewObject::trace(JSTracer*, JSObject*)
  #15476 = void js::NativeObject::initPrivate(void*)
  #15390 = uint32 js::NativeObject::numFixedSlots() const

Then there's

  #165462 = void js::ScriptSourceObject::trace(JSTracer*, JSObject*)
  #15434 = JS::Value* js::NativeObject::getReservedSlot(uint32) const
  #15418 = JS::Value* js::NativeObject::getSlot(uint32) const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

paired with

  #178194 = void js::ArrayBufferObject::trace(JSTracer*, JSObject*)
  #15427 = void js::NativeObject::setSlot(uint32, JS::Value*)
  #15393 = uint8 js::NativeObject::slotInRange(uint32, uint32) const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

Hopping over to RegExps:

  #244301 = void js::RegExpObject::trace(JSTracer*)
  #18595 = js::ReadBarriered<js::RegExpShared*>& js::RegExpObject::sharedRef()
  #15468 = void** js::NativeObject::privateRef(uint32) const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

I hope the flow makes this one safe, but I didn't check:

  #307657 = void js::gc::StoreBuffer::SlotsEdge::trace(js::TenuringTracer*) const
  #307659 = void js::TenuringTracer::traceObjectSlots(js::NativeObject*, uint32, uint32)
  #15392 = void js::NativeObject::getSlotRange(uint32, uint32, js::HeapSlot**, js::HeapSlot**, js::HeapSlot**, js::HeapSlot**)
  #15393 = uint8 js::NativeObject::slotInRange(uint32, uint32) const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

I'll skip things similar to that. I'm not sure what to make of this:

  #33538 = void NoteWeakMapsTracer::trace(JSObject*, JS::GCCellPtr, JS::GCCellPtr)
  #307694 = void JS::TraceChildren(JSTracer*, JS::GCCellPtr)
  #105943 = void js::TraceChildren(JSTracer*, void*, int32)
  #313776 = void (TraceChildrenFunctor, int32, JSTracer**, void**)<JSObject>((Forward<Args>)(JS::DispatchTraceKindTyped::args)...)) JS::DispatchTraceKindTyped(F, JS::TraceKind, Args&& ...) [with F = TraceChildrenFunctor; Args = {JSTracer*&, void*&}; decltype (f.operator()<JSObject>((Forward<Args>)(JS::DispatchTraceKindTyped::args)...)) = void]
  #314678 = void TraceChildrenFunctor::operator(JSTracer*, void*)(JSTracer*, void*) [with T = JSObject]
  #162570 = void JSObject::traceChildren(JSTracer*)
  #15423 = js::HeapSlot* js::NativeObject::getSlotRef(uint32)
  #15393 = uint8 js::NativeObject::slotInRange(uint32, uint32) const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

Probably nothing or impossible:

  #169217 = void JS::GCHashSet<T, HashPolicy, AllocPolicy>::trace(JSTracer*) [with T = jsid; HashPolicy = js::DefaultHasher<jsid>; AllocPolicy = js::TempAllocPolicy]
  #169608 = js::detail::HashTable<T, HashPolicy, AllocPolicy>::Enum::~Enum() [with T = const jsid; HashPolicy = js::HashSet<jsid, js::DefaultHasher<jsid>, js::TempAllocPolicy>::SetOps; AllocPolicy = js::TempAllocPolicy]
  #169801 = js::detail::HashTable<T, HashPolicy, AllocPolicy>::Enum::~Enum() [with T = const jsid; HashPolicy = js::HashSet<jsid, js::DefaultHasher<jsid>, js::TempAllocPolicy>::SetOps; AllocPolicy = js::TempAllocPolicy]
  #169803 = void js::detail::HashTable<T, HashPolicy, AllocPolicy>::compactIfUnderloaded() [with T = const jsid; HashPolicy = js::HashSet<jsid, js::DefaultHasher<jsid>, js::TempAllocPolicy>::SetOps; AllocPolicy = js::TempAllocPolicy]
  #168049 = js::detail::HashTable<T, HashPolicy, AllocPolicy>::RebuildStatus js::detail::HashTable<T, HashPolicy, AllocPolicy>::changeTableSize(int, js::detail::HashTable<T, HashPolicy, AllocPolicy>::FailureBehavior) [with T = const jsid; HashPolicy = js::HashSet<jsid, js::DefaultHasher<jsid>, js::TempAllocPolicy>::SetOps; AllocPolicy = js::TempAllocPolicy]
  #13224 = void js::TempAllocPolicy::reportAllocOverflow() const
  #17966 = void js::ReportAllocationOverflow(JSContext*)
  (IN LIMITED 1) #29390 = void JS_ReportErrorNumberASCII(JSContext*, (JSErrorFormatString*)(void*,uint32)*, void*, uint32)
  #153327 = void JS_ReportErrorNumberASCIIVA(JSContext*, (JSErrorFormatString*)(void*,uint32)*, void*, uint32, __va_list_tag*)
  #153328 = uint8 js::ReportErrorNumberVA(JSContext*, uint32, (JSErrorFormatString*)(void*,uint32)*, void*, uint32, uint32, __va_list_tag*)
  #154214 = jscntxt.cpp:void ReportError(JSContext*, JSErrorReport*, (JSErrorFormatString*)(void*,uint32)*, void*)
  #154216 = void js::ErrorToException(JSContext*, JSErrorReport*, (JSErrorFormatString*)(void*,uint32)*, void*)
  #155890 = js::ErrorObject* js::ErrorObject::create(JSContext*, uint32, class JS::Handle<JSObject*>, class JS::Handle<JSString*>, uint32, uint32, struct js::ScopedJSFreePtr<JSErrorReport>*, class JS::Handle<JSString*>, class JS::Handle<JSObject*>)
  #239297 = uint8 js::ErrorObject::init(JSContext*, class JS::Handle<js::ErrorObject*>, uint32, struct js::ScopedJSFreePtr<JSErrorReport>*, class JS::Handle<JSString*>, class JS::Handle<JSObject*>, uint32, uint32, class JS::Handle<JSString*>)
  #15437 = void js::NativeObject::initReservedSlot(uint32, JS::Value*)
  #15428 = void js::NativeObject::initSlot(uint32, JS::Value*)
  #15393 = uint8 js::NativeObject::slotInRange(uint32, uint32) const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

It doesn't report anything more if I suppress things resembling any of the above.
Attachment #8975440 - Flags: review?(sphink)
(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #7)
> I used my callgraph traversal to look for more instances. Here's one:

This looks really useful, is this available publicly somehow?

>  #178229 = void js::ArrayBufferViewObject::trace(JSTracer*, JSObject*)
>  #15471 = void* js::NativeObject::getPrivate() const
>  #15390 = uint32 js::NativeObject::numFixedSlots() const

It's OK to call numFixedSlots() on the object that's being traced, because it's already had its shape updated.  The problem comes when doing this for some other object (e.g. a view of an array buffer being traced).  So I think all of these are safe.

>   #165462 = void js::ScriptSourceObject::trace(JSTracer*, JSObject*)

This doesn't seem to exist any more.

>  #169217 = void JS::GCHashSet<T, HashPolicy, AllocPolicy>::trace(JSTracer*) [with T = jsid; HashPolicy = js::DefaultHasher<jsid>; AllocPolicy = js::TempAllocPolicy]

Hmm, hopefully that's impossible.  Oh, we should be using Range rather than Enum, which doesn't have this possibility.
Has Regression Range: yes → no
Has Regression Range: no → yes
Comment on attachment 8975440 [details] [diff] [review]
bug1461027-fixed-data

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

Oh right, that makes sense. I wonder if we could stick the object being traced into the JSTracer if DEBUG, so we could assert in numFixedSlots().

re: the callgraph tool, I thought I had sent you instructions for using it at some point in the past when I was going on PTO? I can look for it.

But quick instructions:
1. grab the script at https://bitbucket.org/sfink/sfink-tools/raw/default/bin/traverse.py?at=default&fileviewer=file-view-default
2. find a recent hazard build. It'll have a build artifact named callgraph.txt. (The one I used here was pretty old, as you noticed from the missing ScriptSourceObject::trace). You'll need to uncompress it.
3. python traverse.py callgraph.txt
4. run 'help'. Many of the commands are kind of cryptic. Some of them don't work since I keep changing stuff and I don't have tests. It'll take a while to load in the full callgraph (you can do a -p linux64-shell-haz push to get a much smaller shell-only one, but it's not *that* bad.)

I have a Rust version that loads the callgraph in < 1sec, but I haven't implemented any of the traversals for it yet.

As you point out, though, it's not really the right tool for this particular problem since it can't distinguish which object you're looking at. Though come to think of it, I wonder how huge it would get if I recorded every field access  in every method, so you could ask questions like "are all accesses to SomeClass.somefield done with this RAII lock held?" or simply "where are all the places we directly access this struct's field?" Hmm...
Attachment #8975440 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c1b176c63fc8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Group: javascript-core-security → core-security-release
Flags: in-testsuite+
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main62+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.