The following testcase crashes on mozilla-central revision 20210906-1b49e7328ae4 (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --ion-warmup-threshold=1 --blinterp-warmup-threshold=1):

See attachment.

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555576946b7 in js::jit::BaselineScript::retAddrEntryFromReturnAddress(unsigned char*) ()
#1  0x0000555557a6a102 in js::jit::JSJitFrameIter::baselineScriptAndPc(JSScript**, unsigned char**) const ()
#2  0x0000555557a71ab1 in js::jit::HandleException(js::jit::ResumeFromException*) ()
#3  0x0000377e6d9e2d56 in ?? ()
[...]
#93 0x0000000000000000 in ?? ()
rax	0x5555557c9d27	93824994811175
rbx	0x31fd49806110	54963929637136
rcx	0x55555814fdd0	93825038351824
rdx	0x0	0
rsi	0x7ffff6abd770	140737331844976
rdi	0x7ffff6abc540	140737331840320
rbp	0x7fffffffb1b0	140737488335280
rsp	0x7fffffffb1b0	140737488335280
r8	0x7ffff6abd770	140737331844976
r9	0x7ffff7fe3840	140737354020928
r10	0x0	0
r11	0x0	0
r12	0x0	0
r13	0x7ffff5719000	140737311248384
r14	0x7fffffffb2d0	140737488335568
r15	0x28d6b9466060	44902696509536
rip	0x5555576946b7 <js::jit::BaselineScript::retAddrEntryFromReturnAddress(unsigned char*)+167>
=> 0x5555576946b7 <_ZN2js3jit14BaselineScript29retAddrEntryFromReturnAddressEPh+167>:	movl   $0x294,0x0
   0x5555576946c2 <_ZN2js3jit14BaselineScript29retAddrEntryFromReturnAddressEPh+178>:	callq  0x555556b0cf8e <abort>

I managed to reduce the testcase significantly by readjusting the OOM parameter while reducing, but I was not able to reduce it further than what is attached and it still contains large portions of the fuzzing driver (Marking attachment s-s separately, do not use as a regression test). While reducing, I noticed that the test is highly sensitive to any changes, even single dead operations/branches.

Also marking s-s based on the JIT assert.

Bugmon Analysis
Unable to reproduce bug 1729269 using build mozilla-central 20210906031657-1b49e7328ae4. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Looked into this. This is a regression from the Map/Set patches (bug 1341265), which I missed when reviewing. Specifically: the baseline CacheIR code that handles string keys calls emitAtomizeString, which discards the stack, pushes some preserved registers, and then enters a stub frame for a VM call. Unfortunately, the VM call code assumes that the return address is on top of the stack: see, for example, the x64 implementation.

If we OOM while atomizing the string, then we throw an exception. While unwinding the stack, we try to look up the return address in retAddrEntryFromReturnAddress, but instead look up a bogus pointer (the address of the Set object, in this case). In a debug build we assert. I haven't worked through what happens in a release build, but I doubt it's good.

This is the only place in the CacheIR compiler where we try to keep a register live across a VM call. I'm going to try rewriting this code to change the VM call into a fallible callWithABI. If that doesn't work, the safest fix is probably to limit this IC to already atomized strings.

(Note that the reduced testcase has to include parts of the driver because the Set that goes wrong is in the driver code, not the fuzzed testcase itself.)

Depends on D124997

Iain already looked at this one (thanks!).

Comment on attachment 9240168 [details]
Bug 1729269: Use callWithABI for emitAtomizeString r=jandem!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It would be somewhat tricky. The bug requires triggering an OOM at precisely the right moment, so it might be hard to exploit reliably.
• 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?: 93
• If not all supported branches, which bug introduced the flaw?: Bug 1341265
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This patch applies cleanly to the latest beta.
• How likely is this patch to cause regressions; how much testing does it need?: This patch makes unusual code more boring. It is fairly unlikely to cause new regressions.

Comment on attachment 9240168 [details]
Bug 1729269: Use callWithABI for emitAtomizeString r=jandem!

sec-approval = dveditz

Comment on attachment 9240168 [details]
Bug 1729269: Use callWithABI for emitAtomizeString r=jandem!

Beta/Release Uplift Approval Request

• User impact if declined: Possible sec bug
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change itself is relatively safe: it converts unusual code to be more boring. The patch and the testcase have not landed in Nightly yet to avoid painting a bullseye on the bug.
• String changes made/needed:

Use callWithABI for emitAtomizeString r=jandem
https://hg.mozilla.org/integration/autoland/rev/e7654c6d242a51291d6f80042c9909e9f571ef56
https://hg.mozilla.org/mozilla-central/rev/e7654c6d242a

Comment on attachment 9240168 [details]
Bug 1729269: Use callWithABI for emitAtomizeString r=jandem!

Approved for 93.0b7.

https://hg.mozilla.org/releases/mozilla-beta/rev/25420306af89

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

This test can land if it's still relevant.

In a weird coincidence, I finally landed this testcase just before checking my needinfos.

Add testcase r=jandem
https://hg.mozilla.org/integration/autoland/rev/4b5c1ea3ef603231ff71f470b81fc26f0f1fa163
https://hg.mozilla.org/mozilla-central/rev/4b5c1ea3ef60