1685420 - ~nsCOMPtr rooting hazards and similar, revealed by improvements in virtual method resolution and elimination of annotations

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Steve Fink [:sfink] [:s:]]

I have a collection of rooting hazards that I needed to fix as a result of (I think?) fixing virtual method resolution. It is hard to determine whether ~nsCOMPtr can GC or not. I personally don't know the answer.

I don't have a sample hazard. But a random example ~nsCOMPtr stack is:

    nsCOMPtr<T>::~nsCOMPtr() [with T = mozilla::dom::EventTarget]
    mozilla::dom::EventTarget.Release:0
    mozilla::DOMEventTargetHelper.Release:0
    uint32 mozilla::DOMEventTargetHelper::Release()
    mozilla::DOMEventTargetHelper.DeleteCycleCollectable:0
    (unknown-definition)
    internal

I have at times annotated away all Release calls, but I'm not convinced it's valid. For now at least, it seems better to avoid the problem via careful rooting. But I can look closer if any of these seem unnecessary or overly complex.

Marking s-s even though I'm really not sure if there are any real issues here. It could be sec-low or sec-audit or something?

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1685420 - ~nsCOMPtr and related hazard fixes.

Comment 2

[Andrew McCreight [:mccr8]]

Yeah, hard to say what the rating should be here. Some of these seem like they are certainly benign, but I don't know if they all are.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9195751 [details]
Bug 1685420 - ~nsCOMPtr and related hazard fixes.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Very difficult. You might be able to flail around and get a limited UAF, but even then it would be hard to use.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• 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?: No
• If not, how different, hard to create, and risky will they be?: Should apply pretty cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. This makes some slight reorderings between non-observable things.

Comment 4

[Tom Ritter [:tjr]]

Comment on attachment 9195751 [details]
Bug 1685420 - ~nsCOMPtr and related hazard fixes.

approved to land and request uplift

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/33f934b2338419131c94834ef282256eba647bfa

Please nominate this for Beta and ESR78 approval when you get a chance.

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9195751 [details]
Bug 1685420 - ~nsCOMPtr and related hazard fixes.

Beta/Release Uplift Approval Request

• User impact if declined: It closes some security flaws, some of which are probably only theoretical. Difficult to exploit, but the changes are simple so it feels riskier to leave the flaws in.
• Is this code covered by automated tests?: Yes
• 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 is minor reorderings of normally unobservable stuff. (The only way it becomes observable is if something goes wrong and you hit the problematic case of a GC at just the wrong time.)
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: See above. Simple fix, theoretically exploitable flaws even if difficult to attack.
• User impact if declined: very rare crashes, security flaw exposure
• Fix Landed on Version: 86
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): (see above)
• String or UUID changes made by this patch: none

Comment 7

[Julien Cristau [:jcristau]]

Comment on attachment 9195751 [details]
Bug 1685420 - ~nsCOMPtr and related hazard fixes.

approved for 85.0b9

Comment 8

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/10ca1920e445

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9195751 [details]
Bug 1685420 - ~nsCOMPtr and related hazard fixes.

Approved for 78.7esr.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/b12df3a6f00f

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/33f934b23384