If the runtime is created like this: JSRuntime *rt = JS_NewRuntime(0); JS_SetGCParameter(rt, JSGC_MAX_BYTES, (uint32_t)-1); then JS_ValueToString(cx, INT_TO_JSVAL(X))) returns "0K" to "0T" for 0 <= X <= 9
How are you testing that last bit? JS_ValueToString returns a JSString*, right? What do you do with it after that?
I forgot a piece of code: JS_GetStringCharsZ(cx, JS_ValueToString(cx, INT_TO_JSVAL(9))) return L"0T"
Again, how are you testing that? You have a gc hazard in the code in comment 2, for one thing... depending on what happens exactly you can easily be testing cleaned-up memory...
I use the following code: JSRuntime *rt = JS_NewRuntime(0); JS_SetGCParameter(rt, JSGC_MAX_BYTES, (uint32_t)-1); JSContext *cx = JS_NewContext(rt, 8192L); JSObject *globalObject = JS_NewCompartmentAndGlobalObject(cx, &global_class, NULL); JS_InitStandardClasses(cx, globalObject); JSString *jsstr = JS_ValueToString(cx, INT_TO_JSVAL(9)); jsval tmp = STRING_TO_JSVAL(jsstr); JS_SetProperty(cx, globalObject, "rootme", &tmp); _putws(JS_GetStringCharsZ(cx, jsstr)); JS_DestroyContext(cx); JS_DestroyRuntime(rt); JS_ShutDown(); ...the output is 0T.
Ah, thank you. That's enough to track it down. So we have a very memory-constrained runtime. JS_NewContext creating the first context on the runtime calls js::StaticStrings::init which allocates a whole bunch of strings. Partway through, we end up with this lovel stack: #0 js_GC () at ../../../mozilla/js/src/jsgc.cpp:2972 #1 0x131c8247 in js::gc::RunLastDitchGC (cx=0x2159efc0) at ../../../mozilla/js/src/jsgc.cpp:1641 #2 0x131c8392 in js::gc::ArenaLists::refillFreeList (cx=0x2159efc0, thingKind=js::gc::FINALIZE_STRING) at ../../../mozilla/js/src/jsgc.cpp:1656 #3 0x132c2f3d in js::gc::NewGCThing<JSString> (cx=0x2159efc0, kind=js::gc::FINALIZE_STRING, thingSize=16) at jsgcinlines.h:399 #4 0x13335537 in js_NewGCString (cx=0x2159efc0) at jsgcinlines.h:445 #5 0x132c2f5d in JSInlineString::new_ (cx=0x2159efc0) at String-inl.h:197 #6 0x132b59ef in NewShortString (cx=0x2159efc0, chars=0xbfffc2f8, length=1) at ../../../mozilla/js/src/jsstr.cpp:3109 #7 0x132b6cec in js_NewStringCopyN (cx=0x2159efc0, s=0xbfffc2f8, n=1) at ../../../mozilla/js/src/jsstr.cpp:3246 #8 0x13333f54 in js::StaticStrings::init (this=0x1b41b920, cx=0x2159efc0) at ../../../mozilla/js/src/vm/String.cpp:468 and the static string code doesn't set |initialized| until the end of |init|. But if !initialized, StaticStrings::trace returns early and never traces the static strings, so they can be GCed. And they are, in this case. After that you're pointing to garbage data. Bill, waldo says you were in here last... can you take a look please?
Yes, sorry, I'll fix.
Created attachment 598093 [details] [diff] [review] patch This seems to fix the problem. The test fails before and not after.
Insanity. The fix looks perfectly reasonable, but what if we changed the GC instead not to run until after the runtime is fully "initialized"?
(In reply to Luke Wagner [:luke] from comment #8) > Insanity. The fix looks perfectly reasonable, but what if we changed the GC > instead not to run until after the runtime is fully "initialized"? I tried to do this, but it's not so easy. In order to allocate strings, we need to have a context. So we don't actually allocate the static strings until the first JSContext is created. I guess it might be nice to have an invariant like, "we don't GC unless the first context has been fully initialized." But somehow that doesn't have the same appeal as "don't GC until the runtime is initialized." So I'm for going with the current patch, but maybe you'll have a better idea.
Comment on attachment 598093 [details] [diff] [review] patch Ah, bummer. Makes sense.
Is this something that could be triggered by web content (perhaps by creating workers)? If not--if it requires privileged code creating its own runtimes--then it's probably not a security vulnerability.
https://hg.mozilla.org/integration/mozilla-inbound/rev/168d1528a2da I don't think it should be possible to trigger this normally, even in a worker. We trigger a GC here because gcMaxBytes is low, but that will never happen in a browser. I was worried that we might naturally OOM in such a case, but I don't think that can happen. Either we get a whole chunk for the new runtime, or we don't. In neither case will be GC in the middle of string table initialization.
Is there something QA can do to verify this fix? Is it as simple as running the code in comment 4 in a JS shell?
It's as simple as putting the code in comment 4 in a C++ file, compiling it, linking against the JS engine, and executing the resulting binary.... But note that there's an automated test that's part of the patch that does just that.
(In reply to Boris Zbarsky (:bz) from comment #15) > It's as simple as putting the code in comment 4 in a C++ file, compiling it, > linking against the JS engine, and executing the resulting binary.... > > But note that there's an automated test that's part of the patch that does > just that. Seems like a lot of work to duplicate effort an automated test is already performing. Is there somewhere I can go to look at the results of that test to confirm it is passing?
http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi-tests/Makefile.in indicates the tests should be run in |make check|, which I had thought tinderboxen ran. I'm not 100% certain offhand that they do so, should be easy to check.
Jeff, I'm having a difficult time tracking down, can you help?