1123648 - crash in js::gc::RelocationOverlay::isForwarded when accessing ctypes.FunctionType.ptr on js shell with ggc build

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Tooru Fujisawa [:arai]]

Following code causes crash on js shell with ggc build.

  gczeal(14, 1);
  ctypes.FunctionType(ctypes.default_abi, ctypes.void_t, []).ptr;

When I call AddObjectRoot for fninfo->mABI, fninfo->mReturnType, and all fninfo->mArgTypes[i] in NewFunctionInfo (CTypes.cpp:5595), it disappears,
but I'm not sure what the correct fix is.

Here is debug log (without above change):

(lldb) run ~/Desktop/test.js
* thread #1: tid = 0x1ab17f, 0x00000001001cd705 js`void MarkInternal<JSObject>(JSTracer*, JSObject**) [inlined] js::gc::RelocationOverlay::isForwarded() const at jsgc.h:1189, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x10367f138)
    frame #0: 0x00000001001cd705 js`void MarkInternal<JSObject>(JSTracer*, JSObject**) [inlined] js::gc::RelocationOverlay::isForwarded() const at jsgc.h:1189
   1186	    }
   1187	
   1188	    bool isForwarded() const {
-> 1189	        return magic_ == Relocated;
   1190	    }
   1191	
   1192	    Cell *forwardingAddress() const {
(lldb) bt
* thread #1: tid = 0x1ab17f, 0x00000001001cd705 js`void MarkInternal<JSObject>(JSTracer*, JSObject**) [inlined] js::gc::RelocationOverlay::isForwarded() const at jsgc.h:1189, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x10367f138)
  * frame #0: 0x00000001001cd705 js`void MarkInternal<JSObject>(JSTracer*, JSObject**) [inlined] js::gc::RelocationOverlay::isForwarded() const at jsgc.h:1189
    frame #1: 0x00000001001cd705 js`void MarkInternal<JSObject>(JSTracer*, JSObject**) [inlined] bool js::gc::IsForwarded<JSObject>(t=0x000000010367f130) at jsgc.h:1217
    frame #2: 0x00000001001cd705 js`void MarkInternal<JSObject>(JSTracer*, JSObject**) [inlined] JSObject* js::gc::MaybeForwarded<JSObject*>(t=0x000000010367f130) at jsgc.h:1263
    frame #3: 0x00000001001cd705 js`void MarkInternal<JSObject>(JSTracer*, JSObject**) [inlined] void CheckMarkedThing<JSObject>(trc=0x000000010280b550) + 30 at Marking.cpp:165
    frame #4: 0x00000001001cd6e7 js`void MarkInternal<JSObject>(trc=0x000000010280b550, thingp=0x0000000101d07f60) + 23 at Marking.cpp:266
    frame #5: 0x0000000100047747 js`js::ctypes::CType::Trace(trc=0x000000010280b550, obj=<unavailable>) + 391 at CTypes.cpp:3460
    frame #6: 0x00000001001f8e85 js`js::GCMarker::processMarkStackTop(this=0x000000010280b550, budget=0x00007fff5fbfd778) + 1477 at Marking.cpp:1831
    frame #7: 0x00000001001e3165 js`js::GCMarker::drainMarkStack(this=0x000000010280b550, budget=0x00007fff5fbfd778) + 69 at Marking.cpp:1894
    frame #8: 0x00000001005b9d6d js`js::gc::GCRuntime::incrementalCollectSlice(js::SliceBudget&, JS::gcreason::Reason) [inlined] js::gc::GCRuntime::drainMarkStack(this=<unavailable>, sliceBudget=0x00007fff5fbfd778, phase=<unavailable>) + 32 at jsgc.cpp:5225
    frame #9: 0x00000001005b9d4d js`js::gc::GCRuntime::incrementalCollectSlice(this=0x0000000102803760, budget=0x00007fff5fbfd778, reason=DEBUG_GC) + 669 at jsgc.cpp:5893
    frame #10: 0x00000001005babfd js`js::gc::GCRuntime::gcCycle(this=0x0000000102803760, incremental=<unavailable>, budget=0x00007fff5fbfd778, reason=DEBUG_GC) + 381 at jsgc.cpp:6113
    frame #11: 0x00000001005bb498 js`js::gc::GCRuntime::collect(this=0x0000000102803760, incremental=false, budget=(deadline = 9223372036854775807, counter = 9223372036854771373), reason=COMPARTMENT_REVIVED) + 840 at jsgc.cpp:6238
    frame #12: 0x0000000100697c0f js`bool js::gc::CheckAllocatorState<(cx=0x0000000101c07ff0, kind=<unavailable>)1>(js::ExclusiveContext*, js::gc::AllocKind) + 303 at jsgcinlines.h:447
    frame #13: 0x0000000100701f4c js`js::Shape* js::gc::AllocateNonObject<js::Shape, (cx=0x0000000101c07ff0)1>(js::ExclusiveContext*) + 44 at jsgcinlines.h:538
    frame #14: 0x000000010081e9fc js`js::EmptyShape::getInitialShape(js::ExclusiveContext*, js::Class const*, js::TaggedProto, JSObject*, JSObject*, unsigned long, unsigned int) [inlined] js::NewGCShape(cx=0x0000000101c07ff0) + 8 at jsgcinlines.h:646
    frame #15: 0x000000010081e9f4 js`js::EmptyShape::getInitialShape(js::ExclusiveContext*, js::Class const*, js::TaggedProto, JSObject*, JSObject*, unsigned long, unsigned int) [inlined] js::EmptyShape::new_(root=0x0000000101c08018, nfixed=<unavailable>, dummy=<unavailable>) at Shape.cpp:1540
    frame #16: 0x000000010081e9f4 js`js::EmptyShape::getInitialShape(cx=0x0000000101c07ff0, clasp=0x0000000101549200, parent=<unavailable>, metadata=<unavailable>, nfixed=12, objectFlags=<unavailable>, proto=<unavailable>) + 852 at Shape.cpp:1578
    frame #17: 0x000000010065b243 js`NewObject(cx=0x0000000101c07ff0, type_=<unavailable>, parent=0x0000000103677e00, kind=FINALIZE_OBJECT12, newKind=GenericObject) + 611 at jsobj.cpp:1218
    frame #18: 0x000000010065aba0 js`js::NewObjectWithGivenProto(cxArg=0x0000000101c07ff0, clasp=0x0000000101549200, parentArg=<unavailable>, allocKind=<unavailable>, newKind=GenericObject, protoArg=<unavailable>) + 528 at jsobj.cpp:1327
    frame #19: 0x000000010065b8d0 js`js::NewObjectWithClassProtoCommon(cxArg=0x0000000101c07ff0, clasp=0x0000000101549200, protoArg=<unavailable>, parentArg=0x0000000103677e00, allocKind=<unavailable>, newKind=<unavailable>) + 48 at jsobj.cpp:1359
    frame #20: 0x000000010056df10 js`JS_NewObject(JSContext*, JSClass const*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) [inlined] js::NewObjectWithClassProto(clasp=<unavailable>, proto=<unavailable>, parent=<unavailable>, allocKind=<unavailable>, newKind=<unavailable>) + 256 at jsobjinlines.h:590
    frame #21: 0x000000010056df05 js`JS_NewObject(JSContext*, JSClass const*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) [inlined] js::NewObjectWithClassProto(clasp=<unavailable>, proto=<unavailable>, parent=<unavailable>, newKind=<unavailable>) + 33 at jsobjinlines.h:598
    frame #22: 0x000000010056dee4 js`JS_NewObject(cx=0x0000000101c07ff0, jsclasp=<unavailable>, proto=JS::HandleObject at 0x00007fff5fbfdd88, parent=JS::HandleObject at 0x00007fff5fbfdd80) + 212 at jsapi.cpp:2185
    frame #23: 0x00000001000346de js`js::ctypes::CType::Create(cx=0x0000000101c07ff0, type=TYPE_pointer, name_=<unavailable>, ffiType=0x00007fff5fbfde28, typeProto=<unavailable>, dataProto=<unavailable>, size_=<unavailable>, align_=<unavailable>) + 318 at CTypes.cpp:3304
    frame #24: 0x0000000100037a0a js`js::ctypes::PointerType::CreateInternal(cx=0x0000000101c07ff0, baseType=<unavailable>) + 250 at CTypes.cpp:4003
    frame #25: 0x0000000100060402 js`js::ctypes::Property<&(js::ctypes::CType::IsCType(JS::Handle<JS::Value>)), &(js::ctypes::CType::PtrGetter(JSContext*, JS::CallArgs))>::Fun(JSContext*, unsigned int, JS::Value*) [inlined] js::ctypes::CType::PtrGetter(JSContext*, JS::CallArgs) + 52 at CTypes.cpp:3757
    frame #26: 0x00000001000603ce js`js::ctypes::Property<&(js::ctypes::CType::IsCType(JS::Handle<JS::Value>)), &(js::ctypes::CType::PtrGetter(JSContext*, JS::CallArgs))>::Fun(JSContext*, unsigned int, JS::Value*) [inlined] bool JS::CallNonGenericMethod<&(args=CallArgs at 0x00007fff5fbfdf80)), &(js::ctypes::CType::PtrGetter(JSContext*, JS::CallArgs))>(JSContext*, JS::CallArgs) + 120 at CallNonGenericMethod.h:100
    frame #27: 0x0000000100060356 js`js::ctypes::Property<&(cx=0x0000000101c07ff0, argc=<unavailable>, vp=0x00007fff5fbfe570)), &(js::ctypes::CType::PtrGetter(JSContext*, JS::CallArgs))>::Fun(JSContext*, unsigned int, JS::Value*) + 54 at CTypes.cpp:217
    frame #28: 0x000000010078bafc js`js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) [inlined] js::CallJSNative(native=0x0000000100060320)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 76 at jscntxtinlines.h:226
    frame #29: 0x000000010078bab0 js`js::Invoke(cx=0x0000000101c07ff0, args=CallArgs at 0x00007fff5fbfe4d0, construct=<unavailable>) + 560 at Interpreter.cpp:498
    frame #30: 0x00000001007b22e0 js`js::Invoke(cx=0x0000000101c07ff0, thisv=<unavailable>, fval=<unavailable>, argc=<unavailable>, argv=<unavailable>, rval=<unavailable>) + 752 at Interpreter.cpp:554
    frame #31: 0x00000001007e0e33 js`js::Shape::get(JSContext*, JS::Handle<JSObject*>, JSObject*, JSObject*, JS::MutableHandle<JS::Value>) [inlined] js::InvokeGetterOrSetter(obj=0x0000000103667100, fval=Value at 0x00007fff5fbfe630, argc=<unavailable>, argv=<unavailable>) + 81 at Interpreter.cpp:627
    frame #32: 0x00000001007e0de2 js`js::Shape::get(this=<unavailable>, cx=0x0000000101c07ff0, obj=<unavailable>, pobj=<unavailable>, receiver=<unavailable>, vp=<unavailable>) + 242 at Shape-inl.h:44
    frame #33: 0x00000001007c1fd8 js`bool NativeGetExistingPropertyInline<(cx=0x0000000101c07ff0, obj=<unavailable>, receiver=<unavailable>, pobj=<unavailable>, shape=<unavailable>, vp=<unavailable>)1>(JSContext*, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) + 648 at NativeObject.cpp:1561
    frame #34: 0x00000001007c233c js`bool GetPropertyHelperInline<(cx=0x0000000101c07ff0, obj=<unavailable>, receiver=<unavailable>, id=<unavailable>, vp=<unavailable>)1>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) + 220 at NativeObject.cpp:1741
    frame #35: 0x00000001007a6b56 js`Interpret(JSContext*, js::RunState&) [inlined] js::NativeGetProperty(cx=<unavailable>) + 42310 at NativeObject.cpp:1752
    frame #36: 0x00000001007a6b4b js`Interpret(JSContext*, js::RunState&) [inlined] js::GetProperty(cx=<unavailable>, root=0x0000000101c08050, root=0x0000000101c08008, root=0x0000000101c08008, dummy=<unavailable>, dummy=<unavailable>) + 10 at NativeObject.h:1371
    frame #37: 0x00000001007a6b41 js`Interpret(JSContext*, js::RunState&) [inlined] GetPropertyOperation(cx=<unavailable>, root=0x0000000101c08040, fp=<unavailable>, dummy=<unavailable>, pc=<unavailable>) + 1772 at Interpreter.cpp:251
    frame #38: 0x00000001007a6455 js`Interpret(cx=<unavailable>, state=<unavailable>) + 40517 at Interpreter.cpp:2377
    frame #39: 0x000000010079c5f6 js`js::RunScript(cx=0x0000000101c07ff0, state=0x00007fff5fbff1f8) + 342 at Interpreter.cpp:448
    frame #40: 0x00000001007b321a js`js::ExecuteKernel(cx=0x0000000101c07ff0, scopeChainArg=0x000000010365a060, thisv=<unavailable>, type=<unavailable>, result=<unavailable>, script=<unavailable>, evalInFrame=<unavailable>) + 970 at Interpreter.cpp:657
    frame #41: 0x00000001007b3671 js`js::Execute(cx=0x0000000101c07ff0, scopeChainArg=<unavailable>, rval=0x0000000000000000, script=<unavailable>) + 449 at Interpreter.cpp:693
    frame #42: 0x00000001005794eb js`ExecuteScript(cx=0x0000000101c07ff0, rval=0x0000000000000000, obj=<unavailable>, scriptArg=<unavailable>) + 395 at jsapi.cpp:4352
    frame #43: 0x0000000100009a86 js`Process(JSContext*, JSObject*, char const*, bool) [inlined] RunFile(root=0x0000000101c08008, dummy=<unavailable>) + 8 at js.cpp:451
    frame #44: 0x0000000100009a7e js`Process(cx=0x0000000101c07ff0, obj_=<unavailable>, filename=<unavailable>, forceTTY=<unavailable>) + 2430 at js.cpp:584
    frame #45: 0x0000000100005ad9 js`main [inlined] ProcessArgs(this=0x0000000101c08008, cx=<unavailable>, obj_=<unavailable>, op=0x0000000101e008b0) + 140 at js.cpp:5496
    frame #46: 0x0000000100005a4d js`main [inlined] Shell(JSContext*, js::cli::OptionParser*, char**) at js.cpp:5735
    frame #47: 0x0000000100005a4d js`main(argc=<unavailable>, argv=<unavailable>, envp=<unavailable>) + 16781 at js.cpp:6075
    frame #48: 0x0000000100001894 js`start + 52

Comment 1

[Bobby Holley (:bholley)]

(In reply to Tooru Fujisawa [:arai] from comment #0)
> Following code causes crash on js shell with ggc build.
> 
>   gczeal(14, 1);
>   ctypes.FunctionType(ctypes.default_abi, ctypes.void_t, []).ptr;

Hm, that's a pretty simple testcase that should be a no-op. Should be easy to track down. Jonco, do you have time to take a look?

Comment 2

[Andrew McCreight [:mccr8]]

If this is only going to affect ctypes addons, then it doesn't quite seem like a sec-high, but feel free to adjust as desired.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Tooru Fujisawa [:arai] from comment #0)
> When I call AddObjectRoot for fninfo->mABI, fninfo->mReturnType, and all
> fninfo->mArgTypes[i] in NewFunctionInfo (CTypes.cpp:5595), it disappears,
> but I'm not sure what the correct fix is.

Thanks for pointing me at the problem!  The structure pointed to by fninfo is not traced if a GC occurs in the lifetime of this function yet it contains pointers to GC things.  This is wrong.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1123648-ctypes

I changed the way the object creation work so we create the CType JSObject before the FunctionInfo object, and set the former's private to point to the latter so we can ensure everything gets traced if a GC happens while we're initializing it.

Comment 5

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

Comment on attachment 8552456 [details] [diff] [review]
bug1123648-ctypes

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

I guess the analysis can't see this because it's on the heap.

::: js/src/ctypes/CTypes.cpp
@@ +5601,5 @@
> +    return false;
> +  }
> +
> +  // Stash the FunctionInfo in a reserved slot.
> +  JS_SetReservedSlot(typeObj, SLOT_FNINFO, PRIVATE_TO_JSVAL(fninfo));

I *think* this is right, but I had to dig through C++ spec gibberish so I'm going to describe my understanding and see if you agree.

The basic concern is that the FunctionInfo hasn't been manually initialized. So the question is whether the compiler will initialize it with values that are safe to trace.

From http://stackoverflow.com/questions/2417065/does-the-default-constructor-initialize-built-in-types it seems like the exact behavior changes between C++98 and C++03. In C++98, the struct is non-POD, so the default constructor will be called, which will invoke the Heap<JSObject*> constructors, which will initialize to valid object pointers (ie, nullptr). In C++03, there is no user-declared constructor, so the whole struct will be value initialized. My reading of the spec ( http://cs.nyu.edu/courses/fall11/CSCI-GA.2110-003/documents/c++2003std.pdf ) says that will value initialize all class members, which in the case of Heap<> means calling its default constructor.

Or, in short, nothing is ever left uninitialized.

@@ +5659,5 @@
>    }
>  
>    if (fninfo->mIsVariadic)
>      // wait to PrepareCIF until function is called
> +    return true;

Pre-existing, but can you wrap this in {}?

Comment 6

[Jon Coppeard (:jonco)]

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

Thanks for the thorough review.

Comment 7

[Jon Coppeard (:jonco)]

This is a pre-existing issue, but it didn't become a problem until bug 650161 landed.  The GC things referenced by the FunctionInfo structure were rooted in other places, but not every pointer to them was traced.

Comment 8

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1bd4a3c733e

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b1bd4a3c733e