The default bug view has changed. See this FAQ.

JS_ValueToString fails to convert integers to string because static string setup can trigger gc in memory-constrained runtimes

RESOLVED FIXED in Firefox 13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Franck, Assigned: billm)

Tracking

Trunk
mozilla13
x86
Windows XP
Points:
---

Firefox Tracking Flags

(firefox13 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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?
(Reporter)

Comment 2

5 years ago
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...
(Reporter)

Comment 4

5 years ago
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?
Group: core-security
Summary: JS_ValueToString fails to convert integers to string → JS_ValueToString fails to convert integers to string because static string setup can trigger gc in memory-constrained runtimes
(Assignee)

Comment 6

5 years ago
Yes, sorry, I'll fix.
Assignee: general → wmccloskey
(Assignee)

Comment 7

5 years ago
Created attachment 598093 [details] [diff] [review]
patch

This seems to fix the problem. The test fails before and not after.
Attachment #598093 - Flags: review?(luke)

Comment 8

5 years ago
Insanity.  The fix looks perfectly reasonable, but what if we changed the GC instead not to run until after the runtime is fully "initialized"?
(Assignee)

Comment 9

5 years ago
(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

5 years ago
Comment on attachment 598093 [details] [diff] [review]
patch

Ah, bummer.  Makes sense.
Attachment #598093 - Flags: review?(luke) → review+
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.
(Assignee)

Comment 12

5 years ago
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.
Target Milestone: --- → mozilla13
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/mozilla-central/rev/168d1528a2da
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-firefox13: --- → fixed
Group: core-security
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?
You need to log in before you can comment on or make changes to this bug.