Closed Bug 505838 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.1 --- ?

People

(Reporter: timeless, Assigned: timeless)

References

(Blocks 1 open bug, )

Details

(4 keywords)

Attachments

(1 file, 1 obsolete file)

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)
Crash how and why? (I believe you, just trying to understand).
Most easily when v = { toString: function() { throw 17; } }, also in case of OOM.
(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
(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..
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.
(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
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 :)
(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
status1.9.1: --- → ?
Flags: blocking1.9.2?
Keywords: regression, testcase
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.
Attached patch patch (obsolete) — Splinter Review
Assignee: general → timeless
Status: NEW → ASSIGNED
Attachment #390211 - Flags: review?(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)";
    }
};
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. ;)
(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.
Attached patch ToString classSplinter Review
Attachment #390211 - Attachment is obsolete: true
Attachment #390283 - Flags: review?(jorendorff)
Attachment #390211 - Flags: review?(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.
Attachment #390283 - Flags: review?(jorendorff) → review+
(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
(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.
(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.
http://hg.mozilla.org/mozilla-central/rev/cca7171047c3
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Following the advice of comment 9.  -'ing.
Flags: blocking1.9.2? → blocking1.9.2-
Marking wanted 1.9.2+
Flags: wanted1.9.2+
You need to log in before you can comment on or make changes to this bug.