1196391 - Make js::ExpandErrorArgumentsVA() more robust

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

As seen in bug 1194562, this method does not deal with invalid input very well. I'm going to try to see what I can do to make it better. For instance, if you have an argCount of greater than 10, I think you get a stack buffer overflow. These errors are only a problem if some other Gecko code uses the API incorrectly, but it is called all over the place by non-experts.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Make the type of locals in js::ExpandErrorArgumentsVA() match the type of JSErrorFormatString::argCount.

I'm not done yet, but I might as well upload what I have so far.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Add JSAPI constant for the max number of error arguments.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Make argument count assertions fatal in js::ExpandErrorArgumentsVA().

Comment 4

[Andrew McCreight [:mccr8]]

Jeff, currently js::ExpandErrorArgumentsVA() only allows there to be 10 arguments for an error message. It seems like either we can go in the direction of enforcing that more aggressively with release asserts (and exporting a constant with the constraint as I've done here), or dynamically allocate |argLengths|. I don't think the additional flexibility of the latter is going to be useful, but it would result in a simpler API that would not expose these implementation details. (While there is no existing constant for the number of arguments, ErrorResult::ThrowErrorWithMessage() in dom/bindings/BindingUtils.cpp already hard-codes this limit.) Do you have a preference between the two?

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Make the type of locals in js::ExpandErrorArgumentsVA() match the type of JSErrorFormatString::argCount. r=Waldo

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Add JSAPI constant for the max number of error arguments. r=Waldo

This will allow users of the API to do checking of values they pass in.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Make argument count assertions fatal in js::ExpandErrorArgumentsVA(). r=Waldo

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8652873 [details] [diff] [review]
part 1 - Make the type of locals in js::ExpandErrorArgumentsVA() match the type of JSErrorFormatString::argCount. r=Waldo

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=337ce9c519d4

These patches are all very small. They eliminate two possible sources of buffer overflows in this code. A third one, caused by passing in an insufficient number of var args, can't be fixed, but I'm working on eliminating that browser-side by removing our usage of the var args side of the error API and replacing it with variadic templates.

Comment 9

[Andrew McCreight [:mccr8]]

There's no need for both a need info and review flags, so I'll cancel that.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8652874 [details] [diff] [review]
part 2 - Add JSAPI constant for the max number of error arguments. r=Waldo

Review of attachment 8652874 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +4421,5 @@
>   * Error reporting.
>   */
>  
> +namespace JS {
> +const uint16_t MaxNumErrorArguments = 10;

Using varargs at all for this junk is dumb, and we want to rip out all these APIs and replace them with something not varargs at some point.  So this is really lipstick on a pig.  And I don't really think anyone's going to be putting ten different items into an error format message.  But whatever.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8652875 [details] [diff] [review]
part 3 - Make argument count assertions fatal in js::ExpandErrorArgumentsVA(). r=Waldo

Review of attachment 8652875 [details] [diff] [review]:
-----------------------------------------------------------------

Again, I'm entirely unworried about this, but whatever.

Comment 12

[Andrew McCreight [:mccr8]]

Yeah, it is a little goofy, but it seems better to check than not. I'm working on excising the use of var args for this on the browser side.

https://hg.mozilla.org/integration/mozilla-inbound/rev/48174ce103c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d204b329da10
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8211485ab2

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48174ce103c3
https://hg.mozilla.org/mozilla-central/rev/d204b329da10
https://hg.mozilla.org/mozilla-central/rev/ba8211485ab2