1503086 - Perform the max-source-length check at SourceBufferHolder init time, not far later

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jeff Walden [:Waldo]]

Currently SourceBufferHolder::length() is size_t and stored that way.  But then when it comes to actually being used, the first thing that happens is an is-uint32_t test -- far later, and so you get a bunch of narrowing conversions everywhere to make it work.

Better, IMO, to have the length-check occur once, at SBH init-time.  We can rely on the value being uint32_t after that, and we don't have to fight the type system on it.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

I had to touch a lot of browser users in this patch.  And -- it looks like some of them were kinda dodgy.

Passing PromiseFlatString(str).get() -- a pointer inside a temporary, I think?  hard to follow through the call graph -- to SourceBufferHolder outside the expression that actually performs the compilation.

Also the maybe-take-ownership spots are a bit finicky and could use some careful looking.

Also that one XUL Compile function that takes ownership and has to carefully -- manually -- free in the case where we never reach the SourceBufferHolder initialization.

And I tried to swap over always-SourceBufferHolder::TakeOwnership calls to pass a UniquePtr that's less fragile in not requiring ownership be explicitly specified.

And the nsJSUtils::ExecutionContext bit where I have to write out a bit of error-handling code and could use a double-check (I just cargo-culted from what CompileAndExec does).

Anyway, lots of finick -- so a Gecko reviewer as well for the Gecko bits.  Except bz isn't accepting right now, and this touches a whole lot of disparate places, so I'm not sure what to do for now.  :-|  Posting for the JS half and I'll figure out the Gecko half later.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 9020990 [details] [diff] [review]
Patch

See comment 1 for all the dodgy bits that need lookin'.  Generally, anything not in js/src you probably want to look at, starting from js/public/SourceBufferHolder.h because its changes are what precipitate the rest of the changes.

Yeah, it's kind of silly to have a failure for script length greater than UINT32_MAX, but in a 64-bit world it becomes less and less crazy with time.  And we have to do this check *somewhere*, and at SourceBufferHolder init time is the right place to do it for all sorts of reasons of simplicity.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 9020990 [details] [diff] [review]
Patch

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

I spent some time looking over this, but I really don't understand this well enough to be able to review it.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: As before, with SourceBufferHolder reverted to still only contain non-null data

Turns out downstream code depends on the data pointer in SourceBufferHolder being non-null, and it's non-trivial to change that and arguably digressive.  Reverted that and tweaked some comments, but otherwise this is essentially the same patch.

Looks like I may need to pull in bz for the Gecko bits of this, but seeing as he's still not accepting requests, I'll have to do it manually on Monday.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9022353 [details] [diff] [review]
As before, with SourceBufferHolder reverted to still only contain non-null data

>+++ b/dom/base/nsJSUtils.cpp
>+  const nsPromiseFlatString& flatBody = PromiseFlatString(aBody);

Want to file a followup to use BeginReading() here instead?

>+++ b/dom/script/ScriptLoader.cpp
>   JS::UniqueTwoByteChars chars(aRequest->ScriptText().extractOrCopyRawBuffer());
>+  if (!chars) {
>+    return Nothing();

Caller assumes (and asserts) that Nothing being returned means an exception on cx.  Is that true here?

I mostly skipped the js/src bits, esp. the  tests.

r=me with the question about the Nothing thing above addressed.

Comment 6

[Ted Campbell [:tcampbell]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #5)
> Caller assumes (and asserts) that Nothing being returned means an exception
> on cx.  Is that true here?

Answering so I don't keep asking myself the same..

Yes, false on SourceBufferHolder::init means an exception is pending that is consistent with returning Nothing() here.

Comment 7

[Ted Campbell [:tcampbell]]

(In reply to Ted Campbell [:tcampbell] from comment #6)
> (In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #5)
> > Caller assumes (and asserts) that Nothing being returned means an exception
> > on cx.  Is that true here?
> 
> Answering so I don't keep asking myself the same..
> 
> Yes, false on SourceBufferHolder::init means an exception is pending that is
> consistent with returning Nothing() here.

*Sigh*.. I was looking at the line above. I agree with bz that this looks wrong.

ScriptText() -> JSMallocAllocPolicy. No context passed.

Comment 8

[Ted Campbell [:tcampbell]]

Comment on attachment 9022353 [details] [diff] [review]
As before, with SourceBufferHolder reverted to still only contain non-null data

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

I'm not fully convinced by the motivation of making the holder fallible, but don't have any strong objections either so, okay!

Comments bellow need to be addressed (and those changes probably stamped by an appropriate dom person).

::: dom/script/ScriptLoader.cpp
@@ +1962,5 @@
>  
>    size_t length = aRequest->ScriptText().length();
>    JS::UniqueTwoByteChars chars(aRequest->ScriptText().extractOrCopyRawBuffer());
> +  if (!chars) {
> +    return Nothing();

I think :bz is correct that is is missing an exception.

Add |JS_ReportOutOfMemory(aCx)| here.

::: dom/workers/WorkerPrivate.cpp
@@ +4893,5 @@
>  
>        JS::Rooted<JS::Value> unused(aes.cx());
>  
> +      JS::SourceBufferHolder srcBuf;
> +      if (!srcBuf.init(aes.cx(), script.BeginReading(), script.Length(),

This doesn't look mechanical.

Below, we apply the |retval = false| when JS_IsExceptionPending is false, which won't match if |srcBuf.init(..)| fails.

If the existing code does intentional fun for worker exception handling, then I think something like the following should work:

> if (!srcBuf.init(..) ||
>     !JS::Evaluate(..))
> {
>   if (!JS_IsExceptionPending(aCx)) {
>     retval = false;
>     break;
>   }
> }

::: js/public/SourceBufferHolder.h
@@ +23,5 @@
>   *
>   *    size_t length = 512;
>   *    char16_t* chars = js_pod_malloc<char16_t>(length);
> + *    if (!chars) {
> + *        return false;

I guess this is only a comment, but a ReportOutOfMemory would typically be required for consistency with code below.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Final patch, only WorkerPrivate.cpp change needs a final look

Okay, this deals with all the review comments.  baku, could you take a look at the WorkerPrivate.cpp changes?  Plus whatever of SourceBufferHolder.h's changes seem necessary to understand that.

Gist of things is, where JS::Evaluate used to be able to fail with exception set, I have split out of that call a separate operation, SourceBufferHolder::init, that also can fail and set an exception.  Have I changed things properly so that if either operation fails, the correct actions are performed?  After two people's looking I think I'm now good, but one last looky-lou has been requested.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 9023512 [details] [diff] [review]
Final patch, only WorkerPrivate.cpp change needs a final look

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

Stealing this from Andrea.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c489ba287b49
Initialize all SourceBufferHolders with a fallible function that in all cases assumes ownership of given-ownership data.  r=tcampbell, r=bz, r=mrbkap on some finicky worker code lightly touched here

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/c489ba287b49