505838 - JS_GetStringBytes(JS_ValueToString(...)..) is a bad pattern

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[timeless]

This pattern can crash, and the only two consumers are jsshell (which people are likely to clone) and jsfile (which spidermonkey people want to forget)

Comment 1

[Andreas Gal :gal]

Crash how and why? (I believe you, just trying to understand).

Comment 2

[Jeff Walden [:Waldo]]

Most easily when v = { toString: function() { throw 17; } }, also in case of OOM.

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> Most easily when v = { toString: function() { throw 17; } }, also in case of
> OOM.

JS_GetStringBytes could tolerate a null str parameter, avoiding some pain. Why not? It's already taking pains never to return null ("" on deflated string cache fill OOM).

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #3)
> JS_GetStringBytes could tolerate a null str parameter, avoiding some pain. Why
> not? It's already taking pains never to return null ("" on deflated string
> cache fill OOM).

This seems like a deep hole to go down -- should STRING_TO_JSVAL also tolerate failure? Why not? Also, applications that use this may unknowingly squash the result of a script..

Comment 5

[Jeff Walden [:Waldo]]

I tend to agree with Blake, at least regarding this particular use case.  I could imagine utility for it in cases approximating how JS_GetStringBytes is used in dumpValue when interpreting JSFunction::atom, but I don't think this use case is a good argument for it and would rather see one not motivated by JSAPI misuse.

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #4)
> (In reply to comment #3)
> > JS_GetStringBytes could tolerate a null str parameter, avoiding some pain. Why
> > not? It's already taking pains never to return null ("" on deflated string
> > cache fill OOM).
> 
> This seems like a deep hole to go down

Agree it's a hole. But only for JS_GetStringBytes, nothing else.

> -- should STRING_TO_JSVAL also tolerate
> failure? Why not?

No sane recovery semantics there -- returning JSVAL_NULL does not return a string-tagged jsval.

> Also, applications that use this may unknowingly squash the
> result of a script..

No one should be using JS_GetStringBytes for results that matter! It decimates from "UCS2" to 8-bit chars.

Such an embedding has a null deref crash bug now, under the hypothesis, in addition to wrongly chopping string data in half. If you want to help such an embedding find the latter bug by keeping the crash caused by the former bug on the embedding's part, then maybe that wins.

But it really depends on how common such an embedding double-bug is, compared to the bug filed here.

/be

Comment 7

[timeless]

here's the crash test:
js> it.noisy=true; it.q={ toString: function() { throw 17; } }
resolving its property q, flags {qualified,assigning,}
Bus error

- thanks waldo :)

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #7)
> here's the crash test:
> js> it.noisy=true; it.q={ toString: function() { throw 17; } }
> resolving its property q, flags {qualified,assigning,}
> Bus error
> 
> - thanks waldo :)

This crash has been occurring since 01 Jan 2003 CVS js shells. Smells like an ancient bug.

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000ff0
Crashed Thread:  0

Thread 0 Crashed:
0   js-opt-tm-intelmac            	0x0002b873 js_GetGCStringRuntime + 11
1   js-opt-tm-intelmac            	0x0007e7e0 js_GetStringBytes + 34
2   js-opt-tm-intelmac            	0x000069f6 JS_GetStringBytes + 32
3   js-opt-tm-intelmac            	0x000048b9 its_addProperty(JSContext*, JSObject*, long, long*) + 105
4   js-opt-tm-intelmac            	0x0004c24b js_SetPropertyHelper + 985
5   js-opt-tm-intelmac            	0x0002f3dc js_Interpret + 4844
6   js-opt-tm-intelmac            	0x0003f539 js_Execute + 541
7   js-opt-tm-intelmac            	0x00006cf0 JS_ExecuteScript + 60
8   js-opt-tm-intelmac            	0x0000253b Process(JSContext*, JSObject*, char*, int) + 947
9   js-opt-tm-intelmac            	0x00005c83 main + 2361
10  js-opt-tm-intelmac            	0x000019db _start + 209
11  js-opt-tm-intelmac            	0x00001909 start + 41

Comment 9

[timeless]

no need to block, we don't ship jsshell and jsfile isn't even built regularly.

note that it.noisy is just one of n ways to trigger this crash, it just was the easiest way i could find. i was working on fixing this but got caught up in style questions and distracted by things.

Comment 10

[timeless]

Attachment: patch

Comment 11

[Jason Orendorff [:jorendorff]]

I think the id parameter to its_addProperty and friends can be an integer, so JSVAL_TO_STRING is no good there.

CallArguments has GC hazards that are a bit more obvious with your patch.

I suggest a C++ object for this.

class ToString {
    ToString(JSContext *cx, jsval v) : mCx(cx) {
        mStr = JS_ValueToString(cx, v);

        JS_AddRoot(cx, &mStr);
    }
    ~ToString() {
        JS_RemoveRoot(cx, &mStr);
    }
    char *getBytes() {
        return mStr ? JS_GetStringBytes(mStr) : "(error converting value)";
    }
};

Comment 12

[Jason Orendorff [:jorendorff]]

I was going to add these two lines after JS_ValueToString:

        if (!mStr && JS_IsExceptionPending(cx))
            JS_ReportPendingException(cx);

But in Bugzilla, `TAB i f SPACE` submits your comment. ;)

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #11)
> class ToString {
>     ToString(JSContext *cx, jsval v) : mCx(cx) {
>         JS_AddRoot(cx, &mStr);

Instead of JS_AddRoot/JS_RemoveRoot (should be JS_AddNamedRoot?) would it be terrible if this inherited from JSAutoTempValueRooter which should be a lot cheaper?

(In reply to comment #12)
> I was going to add these two lines after JS_ValueToString:
> 
>         if (!mStr && JS_IsExceptionPending(cx))
>             JS_ReportPendingException(cx);

This would make the error uncatchable. IMO it would be nicer (if not cleaner, where are my C++ exceptions, already!) to have
  bool ok() { return !!mStr; }
to allow the caller to decide.

Comment 14

[timeless]

Attachment: ToString class

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 390283 [details] [diff] [review]
ToString class

Trying not to lose my lunch over the GC hazards in jsfile.cpp.

This is an improvement though.

Please change it back to mCx before pushing though. Thanks.

Comment 16

[Brendan Eich [:brendan]]

(Isn't jsfile.cpp being hg rm'ed?)

We want JSAutoTempValueRooter (with a better, shorter name, and without any jscntxt.h requirement) in public API. Either prefigure that by using it here instead of JS_AddNamedRoot/JS_RemoveRoot, or leave a FIXME: bug nnnnnn and file that bug.

ToString (in a C++ namespace) seems good for public API too, not something for shell/js.cpp to hoard. The other basic types would want their own helpers too (templates or not, not sure).

This seems like the time to hit the "better C++ API" target, hard. Jason, would you lead the charge?

/be

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #16)
> We want JSAutoTempValueRooter (with a better, shorter name, and without any
> jscntxt.h requirement) in public API. Either prefigure that by using it here
> instead of JS_AddNamedRoot/JS_RemoveRoot, or leave a FIXME: bug nnnnnn and file
> that bug.

That bug is already filed as bug 332648.

Comment 18

[Jason Orendorff [:jorendorff]]

(In reply to comment #16)
> This seems like the time to hit the "better C++ API" target, hard. Jason, would
> you lead the charge?

Give me a couple weeks to finish up with getters/setters, but yes.

Comment 19

[timeless]

http://hg.mozilla.org/mozilla-central/rev/cca7171047c3

Comment 20

[Damon Sicore (:damons)]

Following the advice of comment 9.  -'ing.

Comment 21

[Damon Sicore (:damons)]

Marking wanted 1.9.2+