993965 - Fix "Assertion failure: gGotError, at js/src/shell/js.cpp:448"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Running the following I still see some gGotError (debug build) for specefic oomAfterAllocations:

for i in {1..14}; do echo $i; js -e "oomAfterAllocations($i)" -f ./tests/shell.js -f ./tests/js1_7/geniter/shell.js -f ./tests/js1_7/geniter/regress-349331.js 2>&1 | grep gGotError; done

For me that is on:
js -e "oomAfterAllocations(14)" -f ./tests/shell.js -f ./tests/js1_7/geniter/shell.js -f ./tests/js1_7/geniter/regress-349331.js

Comment 1

[Hannes Verschore [:h4writer]]

like requested in bug 990806. (security bug since comment 0 can be used to find other oom bugs that haven't been fixed yet)

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: bug-993965-oom-jsworkers-v1.patch

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8404670 [details] [diff] [review]
bug-993965-oom-jsworkers-v1.patch

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

::: js/src/jsscript.cpp
@@ +1691,5 @@
>          PodCopy(ss->data.source, chars, ss->length());
>      } else {
>          // Shrink the buffer to the size of the compressed data. Shouldn't fail.
> +        if (!ss->adjustDataSize(compressedLength))
> +            return false;

So this is the same issue as bug 992264?

This used to have a check, but it did more. It also freed ss->data.compressed. Should we do that again?
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c27fd77dbaa#l2.94

Also Benjamin raised the issue in bug 992264 that this should only shrink the buffer. So cannot fail in normal implementations. Should we add logic to oomAfterAllocations to do the same? (Note: I have no idea how oomAfterAllocations works, so this could already be the case...?)

Comment 4

[Jason Orendorff [:jorendorff]]

(In reply to Hannes Verschore [:h4writer] from comment #3)
> > +        if (!ss->adjustDataSize(compressedLength))
> > +            return false;
> 
> So this is the same issue as bug 992264?

Oh. I guess so. After I fixed the bit in jsworkers.cpp, I ran the script again and found this. The patch addresses both issues.

> This used to have a check, but it did more. It also freed
> ss->data.compressed. Should we do that again?
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1c27fd77dbaa#l2.94

No. The SourceCompressionTask is on the main thread's stack. Its destructor will be called when it goes out of scope, and then the buffer will be freed. So in the interest of not having unnecessary code, we should leave it.

> Also Benjamin raised the issue in bug 992264 that this should only shrink
> the buffer. So cannot fail in normal implementations.

True, but the spec doesn't guarantee that, and there is really no point depending on implementations to be "normal" when it costs so little to check.

> Should we add logic to
> oomAfterAllocations to do the same? (Note: I have no idea how
> oomAfterAllocations works, so this could already be the case...?)

If it keeps happening, maybe?

Probably most realloc sites should be something else, honestly...

Comment 5

[Hannes Verschore [:h4writer]]

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> (In reply to Hannes Verschore [:h4writer] from comment #3)
> > > +        if (!ss->adjustDataSize(compressedLength))
> > > +            return false;
> > 
> > So this is the same issue as bug 992264?
> 
> Oh. I guess so. After I fixed the bit in jsworkers.cpp, I ran the script
> again and found this. The patch addresses both issues.

Awesome :D

> > Also Benjamin raised the issue in bug 992264 that this should only shrink
> > the buffer. So cannot fail in normal implementations.
> 
> True, but the spec doesn't guarantee that, and there is really no point
> depending on implementations to be "normal" when it costs so little to check.

I agree with this. I'm happy it gets fixed ;).


Thanks

Comment 7

[Hannes Verschore [:h4writer]]

To make sure this doesn't get forgotten to land

Comment 8

[Jason Orendorff [:jorendorff]]

Not security-sensitive.

Comment 9

[Jason Orendorff [:jorendorff]]

bhackett already rewrote this code to be nicer, fixing most of the bugs. (I think the new use of js_realloc might be buggy, though, need to look it it.) Landed the remainder.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce78e640c126

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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