bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

Crash during minor GC in JSString::isPermanentAtom - Possible casting issue to JS::shadow::runtime




JavaScript: GC
4 years ago
4 years ago


(Reporter: Yves Gwerder, Unassigned)


31 Branch

Firefox Tracking Flags

(Not tracked)



(1 attachment)



4 years ago
I discovered this problem when testing the PersistentRooted<T> type.
Any use of these types made my example program crash during GC (full example attached):

JS_GC(rt); // works
JS::RootedValue tmp1(cx, JS::UndefinedValue());
JS::PersistentRootedValue val1(rt, tmp1);
JS_GC(rt); // crash

#0 0x7ffff651712e	JSString::isPermanentAtom(this=0xfff9000000000000) (/home/yves/Projekte/firefox31/mozjs31/js/src/vm/String.h:389)
#1 0x7ffff66613e6	ThingIsPermanentAtom<JSString>(str=0xfff9000000000000) (/home/yves/Projekte/firefox31/mozjs31/js/src/gc/Marking.cpp:141)
#2 0x7ffff6672eb8	CheckMarkedThing<JSString>(trc=0x6055e0, thing=0xfff9000000000000) (/home/yves/Projekte/firefox31/mozjs31/js/src/gc/Marking.cpp:163)
#3 0x7ffff66704aa	MarkInternal<JSString>(trc=0x6055e0, thingp=0x7fffffffe2d8) (/home/yves/Projekte/firefox31/mozjs31/js/src/gc/Marking.cpp:211)
#4 0x7ffff666e581	MarkRoot<JSString>(trc=0x6055e0, thingp=0x7fffffffe2d8, name=0x7ffff6ecff41 "PersistentRooted<JSString *>") (/home/yves/Projekte/firefox31/mozjs31/js/src/gc/Marking.cpp:304)
#5 0x7ffff66632f3	js::gc::MarkStringRoot(trc=0x6055e0, thingp=0x7fffffffe2d8, name=0x7ffff6ecff41 "PersistentRooted<JSString *>") (/home/yves/Projekte/firefox31/mozjs31/js/src/gc/Marking.cpp:493)
#6 0x7ffff668e08a	js::gc::PersistentRootedMarker<JSString*>::markChainIfNotNull<&js::gc::MarkStringRoot>(trc=0x6055e0, list=..., name=0x7ffff6ecff41 "PersistentRooted<JSString *>") (/home/yves/Projekte/firefox31/mozjs31/js/src/gc/RootMarking.cpp:630)
#7 0x7ffff668afcb	js::gc::MarkPersistentRootedChains(trc=0x6055e0) (/home/yves/Projekte/firefox31/mozjs31/js/src/gc/RootMarking.cpp:658)
#8 0x7ffff668b39d	js::gc::MarkRuntime(trc=0x6055e0, useSavedRoots=false) (/home/yves/Projekte/firefox31/mozjs31/js/src/gc/RootMarking.cpp:713)
#9 0x7ffff6a514e0	BeginMarkPhase(rt=0x605140) (/home/yves/Projekte/firefox31/mozjs31/js/src/jsgc.cpp:2857)
#10 0x7ffff6a56763	IncrementalCollectSlice(rt=0x605140, budget=0, reason=JS::gcreason::API, gckind=js::GC_NORMAL) (/home/yves/Projekte/firefox31/mozjs31/js/src/jsgc.cpp:4382)
#11 0x7ffff6a56df9	GCCycle(rt=0x605140, incremental=false, budget=0, gckind=js::GC_NORMAL, reason=JS::gcreason::API) (/home/yves/Projekte/firefox31/mozjs31/js/src/jsgc.cpp:4565)
#12 0x7ffff6a573ac	Collect(rt=0x605140, incremental=false, budget=0, gckind=js::GC_NORMAL, reason=JS::gcreason::API) (/home/yves/Projekte/firefox31/mozjs31/js/src/jsgc.cpp:4698)
#13 0x7ffff6a5758d	js::GC(rt=0x605140, gckind=js::GC_NORMAL, reason=JS::gcreason::API) (/home/yves/Projekte/firefox31/mozjs31/js/src/jsgc.cpp:4729)
#14 0x7ffff69ea876	JS_GC(rt=0x605140) (/home/yves/Projekte/firefox31/mozjs31/js/src/jsapi.cpp:1887)
#15 0x401a5b	run(cx=0x619e60) (/home/yves/Projekte/examples/SpiderMonkey31_persistentRooted/main.cpp:65)
#16 0x401be7	main(argc=1, argv=0x7fffffffe418) (/home/yves/Projekte/examples/SpiderMonkey31_persistentRooted/main.cpp:90)

I thought it's weird that it crashes when marking PersistentRooted<JSString *> even though there's no such type in my example (should be PersistentRooted<Value>) and debugging didn't reveal any engine-internal uses either. It seems to be caused by incorrect casting in jspubtb.h:208:

static JS::shadow::Runtime *asShadowRuntime(JSRuntime *rt) {
   return reinterpret_cast<JS::shadow::Runtime*>(rt);

I think that should be a static_cast here. I couldn't test it because the type JSRuntime is only forward declared in jspubtd.h. But it really looks like the pointer gets corrupted.

If I set a breakpoint in RootingAPI.h:1144 and check some pointer addresses, I get this:
&srt->valuePersistentRooteds	0x6071c0 <-- !
&rt->valuePersistentRooteds 	0x6071d8
&rt->stringPersistentRooteds 	0x6071c0 <-- !
&srt->stringPersistentRooteds 	0x6071a8

As you see, both the value pointer of srt and the string pointer of rt point to the address 0x6071c0.
This explains why it apparently tries to free a JSString* when it should free a JS::Value.

I have tested this with the ESR31 beta, but it most likely affects multiple versions.

Comment 1

4 years ago
Created attachment 8440299 [details]
a simple example to reproduce the crash
So the pointers are 24 bytes off from each other, right?

And I assume this is a 64-bit system?  On ESR31, JSRuntime has 24 bytes of data members that are conditional on JSGC_GENERATIONAL and come before the *PersistentRooteds members.  So it sounds like your code and the spidermonkey library you're linking against were compiled with different values of JSGC_GENERATIONAL.  Specifically, looks like your code is compiled without JSGC_GENERATIONAL, while SpiderMonkey is compiled with JSGC_GENERATIONAL.
I wonder if we can make this situation fail fast somehow...  I'm not sure how.
Component: JavaScript Engine → JavaScript: GC

Comment 4

4 years ago
Indeed, adding "#define JSGC_GENERATIONAL 1" on top of main.cpp makes the crashes disappear.
I wondered why it does not crash in Firefox but didn't get that last piece of the puzzle, sorry.

Now I'm wondering why that define is not in js-config.h. I'm quite sure I'm using the headers that were generated during the build of the debug binaries, but I'll check again. Someone can remove the "Security-Sensitive Core Bug" checkbox (I don't have that right).
Group: core-security
This particular define is set in browser confvars, not via configure flags, in case that matters....

Comment 6

4 years ago
In my opinion there are multiple issues remaining here.

1. Getting the right defines for JSAPI embedders
From a JSAPI embedder point of view, the JSGC_USE_EXACT_ROOTING and JSGC_GENERATIONAL defines behave differently than other build settings that affect the public API. 
For all the settings I used until now, I could expect that the required defines end up in js-config.h and I just had to include the right one. Could this behaviour be used here too (at least for standalone API builds)? I'd expect other embedders to run into the same problem.

2. Compatibility for system provided libraries
There's another issues when thinking about using the JSAPI as a system provided library. Exact rooting and GGC are enabled or disabled at library compile time. If an embedder does not yet do exact rooting, he can't use the system library if it has exact rooting enabled. It should work better the other way around. As soon as the program code supports exact rooting, the packager can theoretically decide if it should be enabled or not in both the library and the program. However, the library won't work with other programs that don't support exact rooting yet.
Exact rooting is going to be mandatory in the future, so this problem will be solved automatically, but what about GGC (I assume it will become mandatory too)? And are there other settings defined at compile time that could cause the same kind of problem? For debug builds there definitely are such problems, but release builds are more important.

What should happen with this bug now?
You need to log in before you can comment on or make changes to this bug.