Closed Bug 1341149 Opened 3 years ago Closed 3 years ago

Use of uninitialized value in SetupImageLayerClip

Categories

(Core :: Web Painting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: tsmith, Assigned: u459114)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files, 3 obsolete files)

Attached file test_case.html
This was found using a firefox-valgrind-opt build from task cluster:
https://tools.taskcluster.net/index/artifacts/#gecko.v2.mozilla-central.latest.firefox/gecko.v2.mozilla-central.latest.firefox.linux64-valgrind-opt

Valgrind was launched using the following args:
valgrind -q --smc-check=all-non-file --show-mismatched-frees=no --show-possibly-lost=no --read-inline-info=yes --track-origins=yes --vex-iropt-register-updates=allregs-at-mem-access ...

Conditional jump or move depends on uninitialised value(s)
   at 0x12A04E99: SetupImageLayerClip (nsCSSRendering.cpp:2147)
   by 0x12A04E99: nsCSSRendering::PaintStyleImageLayerWithSC(nsCSSRendering::PaintBGParams const&, nsStyleContext*, nsStyleBorder const&) (nsCSSRendering.cpp:3425)
   by 0x1297704A: PaintMaskSurface(nsSVGIntegrationUtils::PaintFramesParams const&, mozilla::gfx::DrawTarget*, float, nsStyleContext*, nsTArray<nsSVGMaskFrame*> const&, gfxMatrix const&, nsPoint const&) (nsSVGIntegrationUtils.cpp:529)
   by 0x12977A61: CreateAndPaintMaskSurface (nsSVGIntegrationUtils.cpp:611)
   by 0x12977A61: nsSVGIntegrationUtils::PaintMaskAndClipPath(nsSVGIntegrationUtils::PaintFramesParams const&) (nsSVGIntegrationUtils.cpp:972)
   by 0x129F4A17: nsDisplayMask::PaintAsLayer(nsDisplayListBuilder*, nsRenderingContext*, mozilla::layers::LayerManager*) (nsDisplayList.cpp:7711)
   by 0x12A0F19D: PaintInactiveLayer (FrameLayerBuilder.cpp:3706)
   by 0x12A0F19D: mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem>&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, gfxContext*, nsRenderingContext*, nsDisplayListBuilder*, nsPresContext*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, float, float, int) (FrameLayerBuilder.cpp:6017)
    by 0x12A0F8D1: mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*) (FrameLayerBuilder.cpp:6209)
    by 0x11A8EDE9: mozilla::layers::ClientPaintedLayer::PaintThebes(nsTArray<mozilla::layers::ReadbackProcessor::Update>*) (ClientPaintedLayer.cpp:91)
    by 0x11A92568: mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) (ClientPaintedLayer.cpp:139)
    by 0x11A9078D: mozilla::layers::ClientContainerLayer::RenderLayer() (ClientContainerLayer.h:57)
    by 0x11A9078D: mozilla::layers::ClientContainerLayer::RenderLayer() (ClientContainerLayer.h:57)
    by 0x11A8D8CC: mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) (ClientLayerManager.cpp:358)
    by 0x11A98B53: mozilla::layers::ClientLayerManager::EndTransaction(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) (ClientLayerManager.cpp:411)
  Uninitialised value was created by a stack allocation
    at 0x12A049D5: nsCSSRendering::PaintStyleImageLayerWithSC(nsCSSRendering::PaintBGParams const&, nsStyleContext*, nsStyleBorder const&) (nsCSSRendering.cpp:3255)
Component: Layout → Layout: Web Painting
Flags: needinfo?(mstange)
Looks like some of the early exits in nsCSSRendering::GetImageLayerClip don't set aClipState->mHasRoundedCorners.
Flags: needinfo?(mstange) → needinfo?(cku)
This also looks suspiciously related to bug 1338767, which is a crash in DisplayItemClip::GetCommonRoundedRectCount.
Let me check
Assignee: nobody → cku
Flags: needinfo?(cku)
Attachment #8840717 - Attachment description: 344321.patch → Bug 1341149 - Implement ImageLayerClipState::ctor and ImageLayerClipState::IsValid.
Attachment #8840717 - Attachment is obsolete: true
Attachment #8840729 - Attachment is obsolete: true
Since this is a sec bug, let's keep the solution as simple as possible. Implement default ctor of ImageLayerClipState should be enough to fix this bug.
I will do more fine tune in the follow up (bug 1342302)
Attachment #8840742 - Flags: review?(mstange)
Attachment #8840742 - Attachment is obsolete: true
Attachment #8840742 - Flags: review?(mstange)
Attachment #8840770 - Flags: review?(mstange)
Attachment #8840770 - Flags: review?(mstange) → review+
https://hg.mozilla.org/mozilla-central/rev/ea1dda72c26d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please nominate this for uplift to Aurora when you get a chance, in case it fixes bug 1338767. (The only crashes on Nightly are from 2-22, so it is hard to draw conclusion one way or the other.)
Flags: needinfo?(cku)
Comment on attachment 8840770 [details] [diff] [review]
Bug 1341149 - Implement ImageLayerClipState::ctor.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1311270
[User impact if declined]: We need this fix to identify whether the root cause of bug 1338767 is the same with this one. 
[Is this code covered by automated tests?]:no
[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]: NA
[Is the change risky?]: no.
[Why is the change risky/not risky?]: I implemented the default constructor of ImageLayerClipState to give default value of its data member, which should be very safe change.
[String changes made/needed]: NA
Flags: needinfo?(cku)
Attachment #8840770 - Flags: approval-mozilla-aurora?
Comment on attachment 8840770 [details] [diff] [review]
Bug 1341149 - Implement ImageLayerClipState::ctor.

Diagnostic patch, let's take it for 53 so we can follow up on related issues.
Attachment #8840770 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Please request ESR52 approval on this when you get a chance since we have strong evidence that this indeed fixes bug 1338767.
Flags: needinfo?(cku)
(In reply to C.J. Ku[:cjku](UTC+8) from comment #11)
> [Feature/Bug causing the regression]: bug 1311270

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> ESR52 approval [...] we have strong evidence that this indeed fixes bug 1338767.

How can both of these things be true? bug 1311270 was landed in Firefox 53 and almost all of the crashes for bug 1338767 were in versions before that.
Blocks: 1311270
Keywords: regression
That's a very good point.
Since bug 1311270 was landed in 53 and the patch here fix regression of it, uplift it to 52 does not make sense to me
Flags: needinfo?(cku)
Group: layout-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.