Closed Bug 1649728 Opened 10 months ago Closed 10 months ago

Crash in [@ mozilla::SharedStyleSheetCache::CancelLoadsForLoader]

Categories

(Core :: CSS Parsing and Computation, defect, P2)

79 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox77 --- unaffected
firefox78 --- unaffected
firefox79 + fixed
firefox80 + fixed

People

(Reporter: philipp, Assigned: emilio)

References

(Regression)

Details

(4 keywords, Whiteboard: [post-critsmash-triage][sec-survey])

Crash Data

Attachments

(1 file)

This bug is for crash report bp-ab4b473d-2a8a-427a-bb3d-533c00200701.

Top 10 frames of crashing thread:

0 xul.dll mozilla::SharedStyleSheetCache::CancelLoadsForLoader layout/style/SharedStyleSheetCache.cpp:503
1 xul.dll mozilla::css::Loader::DeregisterFromSheetCache layout/style/Loader.cpp:523
2 xul.dll mozilla::dom::Document::SetPrincipals dom/base/Document.cpp:3748
3 xul.dll mozilla::dom::Document::ResetToURI dom/base/Document.cpp:2539
4 xul.dll mozilla::dom::Document::Reset dom/base/Document.cpp:2445
5 xul.dll mozilla::dom::Document::StartDocumentLoad dom/base/Document.cpp:3064
6 xul.dll mozilla::dom::XMLDocument::StartDocumentLoad dom/xml/XMLDocument.cpp:251
7 xul.dll nsContentDLF::CreateInstance layout/build/nsContentDLF.cpp
8 xul.dll mozilla::image::SVGDocumentWrapper::SetupViewer image/SVGDocumentWrapper.cpp:292
9 xul.dll mozilla::image::SVGDocumentWrapper::OnStartRequest image/SVGDocumentWrapper.cpp:196

this is another content crash signature looking related to the changes from bug 1599160 landing in 79. different to bug 1646330 it's also hitting users on the beta channel though.

Flags: needinfo?(emilio)

I think this is an instance of bug 1605895. We shouldn't have a bogus data there unless necko doesn't call us back to clean up, which is what bug 1605895 is about :/

I could probably paper over this somehow, but it'd probably leak instead of crashing, which is not amazing either...

Flags: needinfo?(emilio)
Group: layout-core-security

Assigning to Emilio for now just to keep an eye on until bug 1605895 is closed.

Assignee: nobody → emilio
Severity: -- → S3
Status: NEW → ASSIGNED
Priority: -- → P2

Bug 1649555 has STR. Will take a look if Honza can't figure out with that. This is a pre-existing issue, but it seems way more prominent because we now use the cache across documents, so we should try to get this fixed in 79.

Duplicate of this bug: 1649555
Flags: needinfo?(emilio)

This is safer in case Necko fails to notify us. The only repro we have
is fixed by bug 1651661, but this should hopefully be uncontroversial as
well and prevents crashing in release builds.

The only repro we have is bug 1651661, but I think landing this defensive patch is worth it.

Flags: needinfo?(emilio)

Comment on attachment 9163770 [details]
Bug 1649728 - Use WeakPtr in SharedStyleSheetCache::mLoadingDatas. r=heycam,#style

Beta/Release Uplift Approval Request

  • User impact if declined: Crashes.
  • Is this code covered by automated tests?: No
  • 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): Very minor change to not store raw pointers, and store WeakPtr<>s instead, so that we get null instead of garbage when Necko fails to notify (and thus we fail to clean up).

This doesn't fix the correctness issue (there are <link> elements which won't get their load / error events fired and so on), but it prevents the crash on release (keeping the same behavior as before).

  • String changes made/needed: none

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: not easily, finding ways to trigger this is hard and depends on the network.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
  • Which older supported branches are affected by this flaw?: all, but issue is more visible in 79+ because of shared stylesheet caching changes
  • 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?: It should apply cleanly to 79. For 78 I probably need a separate patch, which shouldn't be too hard to create.
  • How likely is this patch to cause regressions; how much testing does it need?: not likely, it should only change behavior in the error / crashing condition.
Attachment #9163770 - Flags: sec-approval?
Attachment #9163770 - Flags: approval-mozilla-beta?

Comment on attachment 9163770 [details]
Bug 1649728 - Use WeakPtr in SharedStyleSheetCache::mLoadingDatas. r=heycam,#style

Approved to land

Attachment #9163770 - Flags: sec-approval? → sec-approval+

Comment on attachment 9163770 [details]
Bug 1649728 - Use WeakPtr in SharedStyleSheetCache::mLoadingDatas. r=heycam,#style

Hopefully wallpapers a new topcrash in 79. Approved for 79.0b9.

Attachment #9163770 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: layout-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Flags: needinfo?(emilio)
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][sec-survey]
Flags: needinfo?(emilio)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.