The following testcase crashes on mozilla-central revision 20200131-76ee1827c820 (build with --enable-debug, run with --fuzzing-safe --no-threads --baseline-warmup-threshold=0):

var concat = String.prototype.concat;
function f18(b83) {
    var a11 = arguments;
    if (b83)
        f18(false);
        g22 = {};
    g22.apply = concat;
    g22.apply(null, a11);
}
f18(true);

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555555d9ac9d in JSString* js::ToStringSlow<(js::AllowGC)0>(JSContext*, js::MaybeRooted<JS::Value, (js::AllowGC)0>::HandleType) ()
#0  0x0000555555d9ac9d in JSString* js::ToStringSlow<(js::AllowGC)0>(JSContext*, js::MaybeRooted<JS::Value, (js::AllowGC)0>::HandleType) ()
#1  0x0000555555bd8abb in str_concat(JSContext*, unsigned int, JS::Value*) ()
#2  0x00001595c9a059df in ?? ()
#3  0x00007fffffffafb0 in ?? ()
#4  0x00007fffffffaf58 in ?? ()
#5  0x00003802027a1380 in ?? ()
#6  0x0000000000000000 in ?? ()
rax	0x555556e66f44	93825018523460
rbx	0x7fffffffaed0	140737488334544
rcx	0x555557f3c840	93825036175424
rdx	0x0	0
rsi	0x7ffff6efd770	140737336301424
rdi	0x7ffff6efc540	140737336296768
rbp	0x7fffffffae90	140737488334480
rsp	0x7fffffffae90	140737488334480
r8	0x7ffff6efd770	140737336301424
r9	0x7ffff7f9cd00	140737353731328
r10	0x58	88
r11	0x7ffff6ba47a0	140737332791200
r12	0x7ffff5e27000	140737318645760
r13	0x7fffffffbae0	140737488337632
r14	0x7fffffffaf90	140737488334736
r15	0x1	1
rip	0x555555d9ac9d <JSString* js::ToStringSlow<(js::AllowGC)0>(JSContext*, js::MaybeRooted<JS::Value, (js::AllowGC)0>::HandleType)+509>
=> 0x555555d9ac9d <_ZN2js12ToStringSlowILNS_7AllowGCE0EEEP8JSStringP9JSContextNS_11MaybeRootedIN2JS5ValueEXT_EE10HandleTypeE+509>:	movl   $0x8b0,0x0
   0x555555d9aca8 <_ZN2js12ToStringSlowILNS_7AllowGCE0EEEP8JSStringP9JSContextNS_11MaybeRootedIN2JS5ValueEXT_EE10HandleTypeE+520>:	callq  0x5555557fb496 <abort>

Because what would an All Hands Party be without a JIT bug? \o/

This has to do with Call ICs. We call GuardFunApplyArgumentsOptimization to invalidate the lazy-args optimization, but it's still possible to have the MagicValue stored in a local slot (a11 in this case). We then attach a stub for the str_concat call, letting the MagicValue escape.

When the script uses arguments, we either shouldn't attach a non-apply stub for JSOp::FunApply or we need a CacheIR guard... Eventually we should try to restrict the lazy args optimization to Ion, with recover instructions or so.

Forwarding to Iain because Call ICs, but it's possible this bug predates that work.

Looks like this is a bug in my call IC code. Prior to my changes, this code would avoid attaching a stub in this case. The current implementation will try to attach a normal stub, which is wrong.

(The code I accidentally dropped was added for bug 870034, but we didn't land a matching testcase.)

I'm running local tests on a fix right now.

Depends on D61794

Comment on attachment 9124668 [details]
Bug 1612636: Tidy up TryAttachSpecialCaseNative r=jandem

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Fairly difficult. The patch does not mention the arguments optimization, which is the key problem.
• 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
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions. This patch causes us to fall back to a VM call in a corner case where we would otherwise have generated an IC.

Adding patches for older supported branches. All differences between these two patches and the nightly patch are cosmetic.

Iain your have sec approval but the patch hasn't landed on m-c and there is no uplift request, I guess that this is not aimed at 74 given that we build RC today?

Comment on attachment 9129362 [details] [diff] [review]
esr68.patch

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: Possible sec bug
• Fix Landed on Version: 74
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch causes us to fall back to a safe path (a VM call) instead of attempting to optimize a rare corner case.
• String or UUID changes made by this patch: None

Comment on attachment 9129363 [details] [diff] [review]
beta-and-release.patch

Beta/Release Uplift Approval Request

• User impact if declined: Potential security bug
• Is this code covered by automated tests?: No
• 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): This patch causes us to fall back to a safe path (a VM call) instead of attempting to optimize a rare corner case.
• String changes made/needed:

Oops, I meant for this to make it into 74 and get uplifted. Is it too late? I was working from this part of the Security Bug Approval Process:

When the bug is approved for landing, the sec-approval flag will be set to '+' with a comment from the approver to land the patch. At that point, land it according to instructions provided..
This will allow us to control when we can land security bugs without exposing them too early and to make sure they get landed on the various branches.

I didn't see a comment telling me to land the patch yet, so I was holding off.

(In reply to Iain Ireland [:iain] from comment #15)

Oops, I meant for this to make it into 74 and get uplifted. Is it too late? I was working from this part of the Security Bug Approval Process:

When the bug is approved for landing, the sec-approval flag will be set to '+' with a comment from the approver to land the patch. At that point, land it according to instructions provided..
This will allow us to control when we can land security bugs without exposing them too early and to make sure they get landed on the various branches.

I didn't see a comment telling me to land the patch yet, so I was holding off.

If you land it now we can take it in RC1 which builds in a couple of hours, if you think it really should be in 74 but can't land it now, we can do a RC2.

Landed on autoland. Let's give CI a bit of a chance to run before we push to Beta, though.
https://hg.mozilla.org/integration/autoland/rev/f7dff5881c26fa01523c815509b7fb2bb7d056ce

https://hg.mozilla.org/releases/mozilla-beta/rev/2e6e177dbbff
https://hg.mozilla.org/releases/mozilla-esr68/rev/9b4ac33bddad

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

Christian, can you please help me with some info about how should I run the testcase attached to this issue in order to reproduce and verify that this is fixed?

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #22)

Christian, can you please help me with some info about how should I run the testcase attached to this issue in order to reproduce and verify that this is fixed?

We have the bug auto-verified now thanks to Jason and shouldn't need additional QA :)

(In reply to Christian Holler (:decoder) from comment #24)

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #22)

Christian, can you please help me with some info about how should I run the testcase attached to this issue in order to reproduce and verify that this is fixed?

We have the bug auto-verified now thanks to Jason and shouldn't need additional QA :)

Thanks for your reply, removing qe-verify+ in this case.

Add test:
https://hg.mozilla.org/integration/autoland/rev/a8922843785af0cb8cf49e3028320574f6c4cbc7
https://hg.mozilla.org/mozilla-central/rev/a8922843785a