Closed Bug 1323241 Opened 3 years ago Closed 3 years ago

Don't report that cells are gray when we don't know

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main52+])

Attachments

(2 files)

The gray marking state can become invalid if we OOM while unmarking gray.  In that case we could report things as gray when they are not.  I guess that makes this s-s since the cycle collector may consider them for collection incorrectly.

This patch changes the API functions to just return false if the gray marking state is invalid.

I moved the validity flag from GCRuntime to the shadow runtime so that we could carry on inling the IsMarkedGray checks.  I'm not sure that's essential.
Attachment #8818324 - Flags: review?(sphink)
Attachment #8818324 - Attachment is patch: true
Comment on attachment 8818324 [details] [diff] [review]
gray-marking-validity

Review of attachment 8818324 [details] [diff] [review]:
-----------------------------------------------------------------

Finally noticing the review requests buried in my secure bugmail. Sorry for the delay. Looks good to me.
Attachment #8818324 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/9b9d0cfd3fa3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: javascript-core-security → core-security-release
Should we take this on Beta as well?
Flags: needinfo?(jcoppeard)
Attached patch bug1323241-betaSplinter Review
Approval Request Comment
[Feature/Bug causing the regression]: Since forever.
[User impact if declined]: Possible crashes / spurious assertion failures in testing.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a fairly simple change to report GC things as gray less often.  It's been on m-c since 9th Jan.
[String changes made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8833269 - Flags: approval-mozilla-beta?
Comment on attachment 8833269 [details] [diff] [review]
bug1323241-beta

gc fix for beta52
Attachment #8833269 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.