Remove RELEASE_BUILD conditionality of SubjectPrincipal() MOZ_CRASH

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({sec-other})

unspecified
mozilla45
sec-other
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [adv-main45-][post-critsmash-triage])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=1072150#c54

During the 45 cycle, I plan to remove the RELEASE_BUILD conditionality of the MOZ_CRASH. Once 44 hits release (and 45 is on beta), I'll check telemetry to make sure we're not seeing a bunch of these cases on release. If we are, we can back the MOZ_CRASH out on beta.
Group: core-security → core-security-release
Scary. Have you gone through all the SubjectPrincipal() usage manually and figured out whether
we need to change the code to use something else?
(Assignee)

Comment 2

3 years ago
(In reply to Olli Pettay [:smaug] from comment #1)
> Scary. Have you gone through all the SubjectPrincipal() usage manually and
> figured out whether
> we need to change the code to use something else?

No, that's pretty intractable, since that analysis depends on whether there is a callstack that reaches the callsite from native code. I'm open to suggestions on how to find culprits beyond the current technique of dynamic analysis (which risks crashes, but not security bugs).
Group: dom-core-security
(Assignee)

Comment 3

3 years ago
Created attachment 8691070 [details] [diff] [review]
Remove RELEASE_BUILD conditionality. v1
Attachment #8691070 - Flags: review?(bzbarsky)
Comment on attachment 8691070 [details] [diff] [review]
Remove RELEASE_BUILD conditionality. v1

What is that telemetry probe showing so far?
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 5

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #4)
> Comment on attachment 8691070 [details] [diff] [review]
> Remove RELEASE_BUILD conditionality. v1
> 
> What is that telemetry probe showing so far?

We don't need the telemetry probe here, because this code is currently on aurora and nightly, which means that the MOZ_CRASH will take effect. Last I checked, the only remaining crash site was for bug 1222280 (at extremely low volume), which is now landed on central and will soon be uplifted to aurora.

The more interesting question is what happens when we get to beta, but we'll have to wait another cycle for that telemetry to tell us anything.
Flags: needinfo?(bobbyholley)
> The more interesting question is what happens when we get to beta, but we'll have to wait
> another cycle for that telemetry to tell us anything.

OK.  Is it worth waiting that cycle to see whether this gets hit in the wild?  Seems to me like it is....
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 7

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #6)
> > The more interesting question is what happens when we get to beta, but we'll have to wait
> > another cycle for that telemetry to tell us anything.
> 
> OK.  Is it worth waiting that cycle to see whether this gets hit in the
> wild?  Seems to me like it is....

I don't understand why it would be - the Telemetry is already going out one cycle ahead of the crash (and I am watching it), so we have all the room we need to disable the release-mode crash if we see the Telemetry firing (either an aurora uplift for Telemetry activity on beta, or a beta uplift for Telemetry activity on release).
Flags: needinfo?(bobbyholley)
Comment on attachment 8691070 [details] [diff] [review]
Remove RELEASE_BUILD conditionality. v1

Ah, I see.

OK, r=me assuming we plan to adjust course as needed based on the telemetry.
Attachment #8691070 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/9689a552174c
status-firefox45: --- → fixed
Target Milestone: --- → mozilla45
(Assignee)

Comment 10

3 years ago
According to Telemetry, we hit this case three times out of 23.55 million on beta. So I'm optimistic about going forward.

I found one new low-volume crash on crash-stats which I'll file now.
(Assignee)

Comment 11

3 years ago
Note sure why this didn't get closed in comment 9.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Group: dom-core-security
(Assignee)

Comment 12

3 years ago
I see 10 samples out of 10 million on release. I think we're good here.
Whiteboard: [adv-main45-]
Whiteboard: [adv-main45-] → [adv-main45-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.