1669616 - MOZ_ASSERT in BigInt::parseLiteral

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Chris Salls]

Attachment: poc1.js

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:81.0) Gecko/20100101 Firefox/81.0

Steps to reproduce:

The Following code results in an assertion error in a debug build of SpiderMonkey.

function blah() {
	return 0;
	if (true) {} 400n == "abc";
}
blah();

When parsing the above code the BigInt 400n is recognized sucessfully and a BigIntToken is emmited via TokenStreamSpecific<Unit, AnyCharsAccess>::bigIntLiteral.
However, before the BigInt is instantiated for the SciptThingsVector, we encounter a JSMSG_STMT_AFTER_RETURN warning which clears the TokenStream's charBuffer.
Because we emmited a BigInt Token when parsing, our CompilationStencil's bigIntData will tell us we have a BigInt, thus in js::frontend::EmitScriptThingsVector we'll hit the BigInt matcher which will attempt to create a new BigInt.
Because our charBuffer has been cleared though, it will be of length 0 causing us to hit the following assert:
MOZ_ASSERT(chars.length());
in BigInt::parseLiteral.

We didn't investigate what happens in a non-debug build. So I'm marking this as security just in case.

Found by a fuzzing project at UCSB by Chris Salls, Jake Corina, Chani Jindal

Comment 1

[Daniel Veditz [:dveditz]]

Decoder: do you know who's working on BigInt?

Comment 2

[Christian Holler (:decoder)]

Hm, good question. I know Waldo was, but I don't know who else and this might also be more parser-related.

Jan, do you have any idea who should take a look here?

I am also surprised that none of the fuzzers found this, I'm going to take a closer look next week. It could be related to syntax somehow.

Comment 3

[Jan de Mooij [:jandem]]

Forwarding this to Waldo (with his permission).

Comment 4

[Jeff Walden [:Waldo]]

Guh, this is messy.

When we have effectful (i.e. not plain declaration) statements after a return statement, in a statement list or in the child statements of a switch case/default, we emit a warning. That warning happens after an ASI-peek, that could examine and unget a bigint (to be converted to parse node on final get).

Getting a bigint fills its un-separated digits in the token stream's charBuffer. It's assumed the digits are there when the token is used to create a parse node.

But if we warn at end of statement, while a bigint has been gotten but not converted to parse node, we add a line of context to the warning's error report. That line of context is accumulated character by character into charBuffer -- supplanting the bigint digits. Then it's extracted into the error report for the warning, leaving charBuffer cleared when the bigint parse node is to be created.

Parsing from a cleared charBuffer reaches BigInt::calculateMaximumDigitsRequired and these lines:

  uint64_t n = CeilDiv(static_cast<uint64_t>(charcount) * bitsPerChar,
                       DigitBits * bitsPerCharTableMultiplier);
  if (n > MaxDigitLength) {
    ReportOutOfMemory(cx);
    return false;
  }

charcount is the number of digits being parsed -- "zero". Our CeilDiv assumes (asserts) the numerator is nonzero, because its implementation approach is 1 + (numerator - 1) / denominator. So the computed n ends up being a very very large number -- much larger than MaxDigitLength. And so we consistently OOM, consistently, safely.

However, we do dereference the pointer of the start of characters:

  // Skipping leading zeroes.
  while (start[0] == '0') {
    start++;
    if (start == end) {
      return zero(cx, heap);
    }
  }

That pointer would be to inline storage in charBuffer, filled with whatever was previously written there -- which happens to be the leading characters of the line of context. The line can't be all zeroes (an all-zeroes line followed by ASI followed by a bigint hits a special case where we don't add a line of context for an error not on the current line), so the loop above will eventually halt, but it'll leave start > end, that'll feed a very large (but somewhat user-controlled) number into the CeilDiv, and end of it all probably we'll still hit n > MaxDigitLength and OOM safely again. But I'm not sure.

Sharing charBuffer across things is the trouble here. The sharing isn't necessarily a bad idea, but it's tricky and prone to error. There's an argument for eliminating it, just always using a stacked buffer. I might agonize over that question more now if it were my day job still. :-)

The line-of-context code is reachable for syntax errors, for strict mode errors if in strict mode, and for warnings. The first two would abort parsing before the cached digits would be expected to be examined. For the third, if I tracked through all callers correctly, I think this and switch statement lists are the only places where a warning could occur with a peeked bigint. I think. And so I think probably it's best/safest to just make the line-of-context code not reuse charBuffer.

Patch is easy to write, already written, just need to write a test to go with it (that will not land simultaneously).

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Bug 1669616 - Make the |CharBuffer|-manipulating functions standalone. r=jorendorff!

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Bug 1669616 - Don't use the shared |CharBuffer| to create a line of context for error reports, because doing so can blow away not-yet-consumed data. r=jorendorff!

Depends on D94491

Comment 7

[Jeff Walden [:Waldo]]

(jorendorff said he'd take on the burden of writing a test to go with this, so from my point of view this is basically out of my hands now)

Comment 8

[Jason Orendorff [:jorendorff]]

Filed bug 1675626 to land these patches in the open.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1669616 - Test unreachable code warning when token lookahead sees a BigInt literal. r=jwalden

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:jorendorff, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 11

[Jason Orendorff [:jorendorff]]

Fix landed in bug 1675626, out of an abundance of caution. Releng decided not to backport—it was too late in the cycle for 83/84, and I don't think the bug is exploitable. This bug is still open to land the test, which we can do once 85 is shipping.

Comment 12

[Nicolas B. Pierron [:nbp]]

(In reply to Jason Orendorff [:jorendorff] from comment #11)

This bug is still open to land the test, which we can do once 85 is shipping.

I think this condition is true now.
https://wiki.mozilla.org/Release_Management/Calendar

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Test unreachable code warning when token lookahead sees a BigInt literal. r=jwalden DONTBUILD
https://hg.mozilla.org/integration/autoland/rev/ee57bce47f644351ec57616384173f17b3d8a1ac
https://hg.mozilla.org/mozilla-central/rev/ee57bce47f64