Closed Bug 1326454 Opened 4 years ago Closed 3 years ago

Make TokenStream report OOM immediately

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(9 files, 1 obsolete file)

7.97 KB, patch
arai
: review+
Details | Diff | Splinter Review
4.87 KB, patch
arai
: review+
Details | Diff | Splinter Review
9.66 KB, patch
arai
: review+
Details | Diff | Splinter Review
7.91 KB, patch
arai
: review+
Details | Diff | Splinter Review
5.60 KB, patch
arai
: review+
Details | Diff | Splinter Review
1.35 KB, patch
arai
: review+
Details | Diff | Splinter Review
3.51 KB, patch
arai
: review+
Details | Diff | Splinter Review
6.80 KB, patch
arai
: review+
Details | Diff | Splinter Review
11.17 KB, patch
arai
: review+
Details | Diff | Splinter Review
TokenStream::getChar and similar, if they parse a line terminator, record this fact by updating line information using TS::updateLineInfoForEOL.  This sometimes attempts to perform an allocation.  If that allocation fails, failure isn't recorded by returning false -- instead, a flag is set, then at some distant future time it gets checked and the error reported.

Aside from just being completely inconsistent with the usual error-reporting  idiom, this also has the apparent demerit of sometimes double-reporting, if line-ending information is updated (or rather, fails to be updated).

We should just fix this to report OOM immediately if it happens.  Some of the offset finicking will be a little tricky to massage into place with the normal idiom, but I'm not convinced what we do now is really totally correct, anyway.

I have patches for all of this *except* for this final bit of massagery.  Will attach what I have in a second.
This is a little bit cleanupy, but it makes this consistent with Parser's error reporting functions.
Attachment #8822734 - Flags: review?(arai.unmht)
Attachment #8822737 - Flags: review?(arai.unmht)
Attachment #8822734 - Flags: review?(arai.unmht) → review+
Attachment #8822735 - Flags: review?(arai.unmht) → review+
Comment on attachment 8822736 [details] [diff] [review]
Rename TokenStream::getBracedUnicode to TokenStream::matchBracedUnicode and make its signature fallible

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

::: js/src/irregexp/RegExpParser.cpp
@@ +242,5 @@
>  }
>  
>  template <typename CharT>
>  RegExpTree*
> +RegExpParser<CharT>::ReportError(unsigned errorNumber, const char* param)

Can we have /* = nullptr */ after param?
Attachment #8822736 - Flags: review?(arai.unmht) → review+
Attachment #8822737 - Flags: review?(arai.unmht) → review+
Attachment #8822738 - Flags: review?(arai.unmht) → review+
Attachment #8822739 - Flags: review?(arai.unmht) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6f7dfbbbc25
Introduce TokenStream::error that reports an error at the current offset.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee1b2ddc8d0a
Introduce TokenStream::warning that warns at the current offset.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/043860a49efd
Rename TokenStream::getBracedUnicode to TokenStream::matchBracedUnicode and make its signature fallible.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/df0c9d78a8ea
Make TokenStream::peekChar's signature fallible.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7ec29bcb862
Make TokenStream::skipChars{,IgnoreEOL} accept an unsigned integral number of chars to skip.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c260a2d1893e
Add assertions to TokenStream::skipChars{,IgnoreEOL} verifying EOF isn't yet hit and that newlines aren't skipped, if appropriate.  r=arai
Keywords: leave-open
Attachment #8823870 - Flags: review?(arai.unmht) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27dda9c4c3dc
Don't report an error when SourceCoords::add fails, because it fails only when an underlying Vector::append fails, and that vector handles OOM reporting itself.  r=arai
Attachment #8847793 - Flags: review?(arai.unmht)
Attachment #8847793 - Attachment is obsolete: true
Attachment #8847793 - Flags: review?(arai.unmht)
It would be nice, IMO, for a failure of getChar/etc. to result in an *immediate* return-false.  But current practice is to goto error, which does a bunch of stuff to poison the TokenStream and returned token so no one can use them, and it's probably best for this bug's chances if I leave that in place.  Notwithstanding that really we shouldn't be relying on such a crutch, IMO.
I'll review these patches maybe this weekend.
if it's urgent, feel free to redirect.
Attachment #8847796 - Flags: review?(arai.unmht) → review+
Comment on attachment 8847827 [details] [diff] [review]
Make TokenStream::getChar's signature fallible (w/minor typo fix)

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

::: js/src/frontend/TokenStream.cpp
@@ +1851,5 @@
>                      goto error;
>                  }
>                  if (!tokenbuf.append(c))
>                      goto error;
> +            } while (true);

just out of curiosity, is there any reason to use do-while(true) instead of while(true) ?
Attachment #8847827 - Flags: review?(arai.unmht) → review+
I generally use do-while because it poses no mental overhead of having to process a useless condition to recognize the loop is always entered.  If I see |while (...) {| I have to look at the |...| to determine what happens.  Even if that's |1| or |true|, it still takes time.  Same for the alternative of |for (;;) {|.  But with do-while, seeing |do {| always indicates the body is entered.

The ideal solution to this, of course, would be having dedicated loop-forever syntax like Rust has, but I doubt this is going to come to C++ any time soon.
I see, thanks :)
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32ea58f62527
Make TokenStream::getChar's signature fallible.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/65f852c28e71
Make TokenStream::updateLineInfoForEOL fallible, and immediately handle errors when it fails instead of temporarily deferring such handling.  r=arai
Depends on: 1385112
Depends on: CVE-2017-7813
Priority: -- → P1
Priority: P1 → P3
Everything landed. -> FIXED.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.