Use of uninitialized value in SetupImageLayerClip

RESOLVED FIXED in Firefox 53

Status

()

Core
Layout: Web Painting
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: tsmith, Assigned: cjku)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla54
csectype-uninitialized, regression, sec-moderate, testcase
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 fixed, firefox54 fixed)

Details

(Whiteboard: [post-critsmash-triage])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

9 months ago
Created attachment 8839291 [details]
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)

Updated

9 months ago
Component: Layout → Layout: Web Painting
Keywords: sec-moderate
Flags: needinfo?(mstange)
Looks like some of the early exits in nsCSSRendering::GetImageLayerClip don't set aClipState->mHasRoundedCorners.
Flags: needinfo?(mstange) → needinfo?(cku)
Blocks: 1338767
This also looks suspiciously related to bug 1338767, which is a crash in DisplayItemClip::GetCommonRoundedRectCount.
(Assignee)

Comment 3

9 months ago
Let me check
Assignee: nobody → cku
Flags: needinfo?(cku)
(Assignee)

Comment 4

9 months ago
Created attachment 8840717 [details] [diff] [review]
Bug 1341149 - Implement ImageLayerClipState::ctor and ImageLayerClipState::IsValid.
(Assignee)

Updated

9 months ago
Attachment #8840717 - Attachment description: 344321.patch → Bug 1341149 - Implement ImageLayerClipState::ctor and ImageLayerClipState::IsValid.
(Assignee)

Comment 5

9 months ago
Created attachment 8840729 [details] [diff] [review]
Bug 1341149 - Implement ImageLayerClipState::ctor.
(Assignee)

Updated

9 months ago
Attachment #8840717 - Attachment is obsolete: true
(Assignee)

Comment 6

9 months ago
Created attachment 8840742 [details] [diff] [review]
Bug 1341149 - Implement ImageLayerClipState::ctor.
Attachment #8840729 - Attachment is obsolete: true
(Assignee)

Comment 7

9 months ago
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)
(Assignee)

Updated

9 months ago
Attachment #8840742 - Flags: review?(mstange)
(Assignee)

Comment 8

9 months ago
Created attachment 8840770 [details] [diff] [review]
Bug 1341149 - Implement ImageLayerClipState::ctor.
Attachment #8840742 - Attachment is obsolete: true
Attachment #8840742 - Flags: review?(mstange)
(Assignee)

Updated

9 months ago
Attachment #8840770 - Flags: review?(mstange)
Attachment #8840770 - Flags: review?(mstange) → review+

Comment 9

9 months ago
https://hg.mozilla.org/mozilla-central/rev/ea1dda72c26d
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox54: affected → fixed
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)
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox-esr52: --- → affected
(Assignee)

Comment 11

9 months ago
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+
uplifted by ryan in https://hg.mozilla.org/releases/mozilla-aurora/rev/3df2a420389b

Updated

9 months ago
status-firefox53: affected → fixed
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.
(Assignee)

Comment 17

9 months ago
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)
status-firefox52: wontfix → unaffected
status-firefox-esr52: affected → unaffected
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.