1987428 - CallArgs::operator[] should release assert

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task)

Description

[Andrew McCreight [:mccr8]]

CallArgs::get() does a bounds check on length(), but CallArgs::operator[] does not. This means that if somebody is refactoring code from the former to the latter it can become unsafe. The easiest way to fix this is to change the MOZ_ASSERT in operator[] to a MOZ_RELEASE_ASSERT.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1987428 - Make CallArgs::operator[] consistent with ::get.

Comment 2

[Andrew McCreight [:mccr8]]

I haven't done any performance testing but it is probably fine right?

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

I don't think I'm inherently opposed to this patch (though I wouldn't be surprised if there are workloads where we do in fact detect a perf regression -- this is on some hot paths).

I will say though, get and operator[] have different semantics and I'd be kind of uncomfortable with patches that are trying to change from one to the other without a fair amount of review.

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #3)

I don't think I'm inherently opposed to this patch (though I wouldn't be surprised if there are workloads where we do in fact detect a perf regression -- this is on some hot paths).

I'm hoping that the branch being very well predicted will avoid performance regressions. That being said, there are a handful of places where we had to opt-out of release mode bounds checks for nsTArray because it was showing up on profiles. Still, I think a safe-by-default approach is better.

I will say though, get and operator[] have different semantics and I'd be kind of uncomfortable with patches that are trying to change from one to the other without a fair amount of review.

Yes, that is why I am not changing the semantics. An out-of-bounds access will be undefined behavior without my patch, so changing it to a crash is consistent.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/16efc1c29ba3
https://hg.mozilla.org/integration/autoland/rev/46fe6d00eaf4
Make CallArgs::operator[] consistent with ::get. r=spidermonkey-reviewers,mgaudet

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/46fe6d00eaf4

Comment 8

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: This provides "belt and suspenders" protection against potential sec-high issues in SpiderMonkey. That being said, the fuzzers should be good at finding any place that has this problem so it is unlikely we have any.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: none
• Risk associated with taking this patch: low
• Explanation of risk level: This adds some checks that should never fail. The only real risk is performance regressions, but it has been on m-c for about a week and a half without any regressions so I think it is fine.
• String changes made/needed: none
• Is Android affected?: yes

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: Bug 1987428 - Make CallArgs::operator[] consistent with ::get.

Original Revision: https://phabricator.services.mozilla.com/D264095

Comment 10

[Phabricator Automation]

firefox-esr115 Uplift Approval Request

• User impact if declined: This provides "belt and suspenders" protection against potential sec-high issues in SpiderMonkey. That being said, the fuzzers should be good at finding any place that has this problem so it is unlikely we have any.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: none
• Risk associated with taking this patch: low
• Explanation of risk level: This adds some checks that should never fail. The only real risk is performance regressions, but it has been on m-c for about a week and a half without any regressions so I think it is fine.
• String changes made/needed: none
• Is Android affected?: yes

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: Bug 1987428 - Make CallArgs::operator[] consistent with ::get.

Original Revision: https://phabricator.services.mozilla.com/D264095

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/c6c83536941e

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/dd8dd9c8118c