Last Comment Bug 726429 - JS_ValueToString fails to convert integers to string because static string setup can trigger gc in memory-constrained runtimes
: JS_ValueToString fails to convert integers to string because static string se...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla13
Assigned To: Bill McCloskey (:billm)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-12 06:59 PST by Franck
Modified: 2012-05-31 13:19 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch (4.38 KB, patch)
2012-02-16 18:43 PST, Bill McCloskey (:billm)
luke: review+
Details | Diff | Review

Description Franck 2012-02-12 06:59:06 PST
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
Comment 1 Boris Zbarsky [:bz] 2012-02-12 11:51:39 PST
How are you testing that last bit?  JS_ValueToString returns a JSString*, right?  What do you do with it after that?
Comment 2 Franck 2012-02-12 12:03:46 PST
I forgot a piece of code:
  JS_GetStringCharsZ(cx, JS_ValueToString(cx, INT_TO_JSVAL(9))) return L"0T"
Comment 3 Boris Zbarsky [:bz] 2012-02-12 12:52:17 PST
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...
Comment 4 Franck 2012-02-16 10:26:05 PST
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.
Comment 5 Boris Zbarsky [:bz] 2012-02-16 11:41:37 PST
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?
Comment 6 Bill McCloskey (:billm) 2012-02-16 11:51:54 PST
Yes, sorry, I'll fix.
Comment 7 Bill McCloskey (:billm) 2012-02-16 18:43:40 PST
Created attachment 598093 [details] [diff] [review]
patch

This seems to fix the problem. The test fails before and not after.
Comment 8 Luke Wagner [:luke] 2012-02-16 19:55:25 PST
Insanity.  The fix looks perfectly reasonable, but what if we changed the GC instead not to run until after the runtime is fully "initialized"?
Comment 9 Bill McCloskey (:billm) 2012-02-17 12:35:03 PST
(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 10 Luke Wagner [:luke] 2012-02-18 18:13:43 PST
Comment on attachment 598093 [details] [diff] [review]
patch

Ah, bummer.  Makes sense.
Comment 11 Daniel Veditz [:dveditz] 2012-02-22 17:15:50 PST
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.
Comment 12 Bill McCloskey (:billm) 2012-02-23 14:41:08 PST
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.
Comment 13 Bill McCloskey (:billm) 2012-02-24 08:12:14 PST
https://hg.mozilla.org/mozilla-central/rev/168d1528a2da
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-29 11:34:25 PDT
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?
Comment 15 Boris Zbarsky [:bz] 2012-05-29 18:17:24 PDT
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.
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-29 18:47:55 PDT
(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?
Comment 17 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-29 18:59:40 PDT
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.
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-31 13:19:25 PDT
Jeff, I'm having a difficult time tracking down, can you help?

Note You need to log in before you can comment on or make changes to this bug.