The following testcase crashes on mozilla-central revision 173de3ea2a27 (build with --disable-jemalloc --enable-address-sanitizer --enable-gczeal --enable-optimize="-O2 -g" --enable-fuzzing --enable-debug, run with --fuzzing-safe):

_=>e
2n

Backtrace:

==20199==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x5575856857f7 bp 0x7ffcaa999d40 sp 0x7ffcaa9997c0 T0)
    #0 0x5575856857f6 in JS::BigInt* JS::BigInt::parseLiteral<char16_t>(JSContext*, mozilla::Range<char16_t const>, bool*) js/src/vm/BigIntType.cpp
    #1 0x557585684686 in js::ParseBigIntLiteral(JSContext*, mozilla::Range<char16_t const> const&) js/src/vm/BigIntType.cpp:3399:17
    #2 0x5575871c04fa in js::frontend::Parser<js::frontend::FullParseHandler, mozilla::Utf8Unit>::newBigInt() js/src/frontend/Parser.cpp:9730:15
    #3 0x55758719dadd in js::frontend::GeneralParser<js::frontend::FullParseHandler, mozilla::Utf8Unit>::newBigInt() js/src/frontend/Parser.cpp:9752:27
    #4 0x55758719dadd in js::frontend::GeneralParser<js::frontend::FullParseHandler, mozilla::Utf8Unit>::primaryExpr(js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::TokenKind, js::frontend::GeneralParser<js::frontend::FullParseHandler, mozilla::Utf8Unit>::PossibleError*, js::frontend::ParserBase::InvokedPrediction) js/src/frontend/Parser.cpp:10763
    [...]

Marking s-s because this will cause an out-of-bounds read in BigInt::parseLiteralDigits later on in release builds.

Waldo, do you think you give this bug a quick look.

I think I understand the problem. I'm going to optimistically assume I do -- can always correct later if needed.

When we're full-parsing and we hit a function, we try to syntax-parse it. That syntax-parse (if the arrow function doesn't have a block body) must tokenize the token (2n here) after the arrow function to determine that the function has actually ended. Once the full arrow function is syntax-parsed, we seek past it in the enclosing full-parse and continue on.

Skipping past, copies over stuff like position, any valid lookahead tokens, and so on. So we'll copy over one lookahead token for the bigint.

When we tokenize a bigint, we accumulate its digits (with separators removed) in tokenStream.charBuffer. We did that inside the syntax-parse for this token. But because we have a bigint token from the syntax parse, we don't do so in the enclosing full-parse. And we don't copy over tokenStream.charBuffer to the enclosing parse.

Before bigints, I think the only case roughly like this was regular expressions. But an arrow function (that doesn't have a block body) can't be followed by a regular expression, because the leading '/' would have extended the arrow function's body. So no problem there.

The simple fix would be to copy the charBuffer over when seeking, but I think that doesn't work. charBuffer is used in parsing displayURL and sourceMapURL directives, and I think we possibly might fail to apply such if we had something like

x=>y
//# displayURL=https://example.com/source.map
2n

(Heck, that might not even work right now.)

I think the right fix might be to always pick up tokenizing from the end of the arrow function. But I'm not absolutely certain of that just yet.

This problem doesn't manifest for strings and template literals because the token created for them contains a JSAtom* of the relevant characters, no depending on transient contents of a buffer. That might be the cleanest solution.

...for the problem originally reported.

It wouldn't deal with displayURL issues, tho, if they exist.

Still investigating...

Yeah, the extra concern from comment 3 is real. Testcase:

Object.defineProperty(this, "q", { get() { print(saveStack().parent.source); } });

// NOTE: ';' after the arrow function.
eval(`x=>y;
//# sourceURL=http://example.com/foo.js
q`);

// NOTE: no ';' after the arrow function.
eval(`x=>y
//# sourceURL=http://example.com/foo.js
q`);

The presence/absence of the semicolon after the arrow function (or more precisely, whether the directive appears before or after the first token after the arrow function) should make no difference: the source URL should take effect either way.

Now, save that as /tmp/foo.js. When I run it, I get

[jwalden@find-waldo-now src]$ dbg/js/src/js -f /tmp/foo.js
http://example.com/foo.js
/tmp/foo.js line 9 > eval

showing that the source URL applies when it's not examined by the syntax-parse of the function, but it's not applied when the syntax-parse sees it and fails to transfer it to the enclosing full parse.

So charBuffer and when to transfer it certainly presents a problem here...but other fields may too present issues.

A fully correct fix for this is going to require some careful code-reading. :-|

The testcase from comment 5 could have used print(new Error().stack) instead as its operation to show the same error. Just in case anyone wonders whether this is directly web-visible.

Object.defineProperty(this, "q", { get() { print(new Error().stack); } })

// NOTE: ';' after the arrow function.
eval(`x=>y;
//# sourceURL=http://example.com/foo.js
q`);

// NOTE: no ';' after the arrow function.
eval(`x=>y
//# sourceURL=http://example.com/foo.js
q`);

[jwalden@find-waldo-now src]$ dbg/js/src/js -f /tmp/foo.js
get@/tmp/foo.js:1:50
@http://example.com/foo.js:3:1
@/tmp/foo.js:4:1

get@/tmp/foo.js:1:50
@/tmp/foo.js line 9 > eval:3:1
@/tmp/foo.js:9:1

The sourceURL issue is actually worse than comment 6 makes it out to be. If you have a directive inside a function that is lazy-parsable, the sourceURL just won't even be applied at all! And if it's inside a function we don't lazy-parse, it will be. Obviously lazy parsing is not supposed to be observable this way!

[jwalden@find-waldo-now src]$ cat /tmp/lazy.js; echo "========"; dbg/js/src/js -f /tmp/lazy.js 
function foo()
{
  //# sourceURL=https://example.com/baz.js
}

print(new Error().stack);
========
@/tmp/lazy.js:6:7

[jwalden@find-waldo-now src]$ cat /tmp/asm.js; echo "========"; dbg/js/src/js -f /tmp/asm.js 
function foo()
{
  "use asm";
  //# sourceURL=https://example.com/baz.js
}

print(new Error().stack);
========
@https://example.com/baz.js:7:7

[jwalden@find-waldo-now src]$ 

Currently I'm finishing up various cleanup patches to get all the TokenStreamAnyChars fields into one place. Next is to figure out how to deal with all this, now that I have the set of fields that can be affected by a nested lazy-parse all identified.

This is looking pretty unlikely to make 72 at this point.

Hi Jeff, any updates on this bug? We've got about two weeks left to land a fix for this cycle.

Might have a patch. It is extraordinarily garish, as patches go. Assuming try checks out, will post for review -- maybe some reviewer will have some flash of brilliance as to how to make it not the fugliest patch I've written in recent memory.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c62e8a3614ebfa6f6005b6e30c2d1b6c4dd31eb1

Depends on D59962

Oh, https://treeherder.mozilla.org/#/jobs?repo=try&revision=d34615a9699901373e83bf87782933111137057f for an updated try-run after various issues sussed out. Does not/did not include the tests, which per normal custom for non-crazy sec bugs I have only verified fail pre-patch, pass post-patch locally.

Hi Ted, looks like this is pending review? We're getting low on time in the Fx73 cycle for this patch to land, so please prioritize this if possible :)

Stamped.

Comment on attachment 9120922 [details]
Bug 1596706. r=tcampbell

Security Approval Request

• How easily could an exploit be constructed based on the patch?: None of this patch ever mentions bigints, and the narrow syntactic case where this matters is a bit obscure in the patch. And even where that does trigger, unless memory is filled just so, with a '0' in a location that ought not be accessed (but wrongly is), I think most likely is going to be an "inexplicable" out-of-memory error.

But this is security, the control flow and logic are not 100% straightforward, we err on the side of caution. It definitely isn't straightforward to make an exploit out of this, and conceivably none can, but I can't say for sure none can. And I left the more-pointed tests to a separate patch that can land after this has landed and shipped, just in case.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All of them, bigint shipped in 68/ESR
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This patch probably backports fairly easily.
• How likely is this patch to cause regressions; how much testing does it need?: Not terribly likely, doesn't need a ton of testing. (The other patch is testing, but because it's security, it seems best not landing directly pointing out the problem.)

Comment on attachment 9120922 [details]
Bug 1596706. r=tcampbell

Approved to land and request uplift; with the crashtest following some time in the future.

https://hg.mozilla.org/integration/autoland/rev/19fec8535fc309800469e147e6961ca3d9cc8729
https://hg.mozilla.org/mozilla-central/rev/19fec8535fc3

Comment on attachment 9120922 [details]
Bug 1596706. r=tcampbell

Beta/Release Uplift Approval Request

• User impact if declined: Certain syntax patterns will trigger (at best) spurious out-of-memory errors. However, the failure mode involves reading past the end of an empty buffer, and precisely what happens depends on the values found there in unclear ways, so the exact memory-unsafety consequences are unknown -- and possibly may be security-issue dangerous.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: There's an automated test, but because this is a security issue we can't land it right now. :-(
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The approach is not aesthetically pleasant, but it's fairly straightforward, and the extra steps this patch will take are clear and understandable.
• String changes made/needed: N/A

Comment on attachment 9120922 [details]
Bug 1596706. r=tcampbell

sec-high fix, approved for 73.0b12

https://hg.mozilla.org/releases/mozilla-beta/rev/23ec77327b8f310b33bb64f928488c3999c67300

AFAICT this needs an ESR68 approval request also? It grafts cleanly as-landed.

Comment on attachment 9120922 [details]
Bug 1596706. r=tcampbell

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: See Comment 19.
  "Certain syntax patterns will trigger (at best) spurious out-of-memory errors. However, the failure mode involves reading past the end of an empty buffer, and precisely what happens depends on the values found there in unclear ways, so the exact memory-unsafety consequences are unknown -- and possibly may be security-issue dangerous."
• Fix Landed on Version: 74 and 73beta
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Patch landed on Nightly last week. The changes are a direct fix to reported testcase and more correct than previous code. Primary challenge of this bug was assessing if there were other variants to patch, but that is complete and not relevant now that patch is landed in other channels.
• String or UUID changes made by this patch:

Comment on attachment 9120922 [details]
Bug 1596706. r=tcampbell

Thanks for the request. Approved for 68.5esr.

https://hg.mozilla.org/releases/mozilla-esr68/rev/18251ad24c9a3b89c42d4098b64e71f54a1e4586

https://hg.mozilla.org/mozilla-central/rev/2626312e935f