Closed Bug 1446348 Opened 2 years ago Closed 2 years ago

Poison memory after sweeping TI data

Categories

(Core :: JavaScript Engine, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

(Keywords: sec-audit, Whiteboard: [adv-main61-][post-critsmash-triage])

Attachments

(1 file, 1 obsolete file)

We still have some TI crashes; crash diagnostics we added didn't help much.

One thing we could still do is immediately poison the old ConstraintTypeSet/TypeConstraint data after we move/sweep it, that would make our magic word checks more effective if we're holding a stale pointer somewhere.

Filing this s-s just to be safe.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8960188 - Flags: review?(tcampbell)
Attached patch PatchSplinter Review
This one also poisons the object set.

I also added a missing capacity check to ConstraintTypeSet::sweep. There's a similar release assert in ConstraintTypeSet::trace but we should check this in sweep as well.
Attachment #8960188 - Attachment is obsolete: true
Attachment #8960188 - Flags: review?(tcampbell)
Attachment #8960200 - Flags: review?(tcampbell)
Priority: -- → P1
Comment on attachment 8960200 [details] [diff] [review]
Patch

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

Can we poison ConstraintTypeSet::trace as well?
Can we poison (at least the size slot) for [1]?

(Or r+ to land as is if you prefer)

[1] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/js/src/vm/TypeInference.cpp#575,4210,4226
Attachment #8960200 - Flags: review?(tcampbell) → review+
(In reply to Ted Campbell [:tcampbell] from comment #3)
> Can we poison ConstraintTypeSet::trace as well?
> Can we poison (at least the size slot) for [1]?

I'll poison the objectSet in trace as well, to make sure it matches what we do in sweep. I think that will be sufficient for now but we can poison more if this still doesn't reveal anything.
https://hg.mozilla.org/mozilla-central/rev/99a7578ee8e4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
I assume there's no intent to backport this to Beta, but feel free to change the status and request approval if there is.
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main61-]
Flags: qe-verify-
Whiteboard: [adv-main61-] → [adv-main61-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.