Closed
Bug 1341149
Opened 7 years ago
Closed 7 years ago
Use of uninitialized value in SetupImageLayerClip
Categories
(Core :: Web Painting, defect)
Core
Web Painting
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)
64 bytes,
text/html
|
Details | |
5.66 KB,
patch
|
mstange
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•7 years ago
|
Component: Layout → Layout: Web Painting
Updated•7 years ago
|
Keywords: sec-moderate
Updated•7 years ago
|
Flags: needinfo?(mstange)
Comment 1•7 years ago
|
||
Looks like some of the early exits in nsCSSRendering::GetImageLayerClip don't set aClipState->mHasRoundedCorners.
Flags: needinfo?(mstange) → needinfo?(cku)
Comment 2•7 years ago
|
||
This also looks suspiciously related to bug 1338767, which is a crash in DisplayItemClip::GetCommonRoundedRectCount.
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)
Updated•7 years ago
|
Attachment #8840770 -
Flags: review?(mstange) → review+
https://hg.mozilla.org/mozilla-central/rev/ea1dda72c26d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 10•7 years ago
|
||
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)
Updated•7 years ago
|
status-firefox52:
--- → wontfix
status-firefox53:
--- → affected
Updated•7 years ago
|
status-firefox-esr52:
--- → affected
Assignee | ||
Comment 11•7 years 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 12•7 years ago
|
||
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+
Comment 13•7 years ago
|
||
uplifted by ryan in https://hg.mozilla.org/releases/mozilla-aurora/rev/3df2a420389b
Updated•7 years ago
|
Comment 14•7 years ago
|
||
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)
Comment 15•7 years ago
|
||
(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
Comment 16•7 years ago
|
||
That's a very good point.
Assignee | ||
Comment 17•7 years 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)
Updated•7 years ago
|
Updated•7 years ago
|
Group: layout-core-security → core-security-release
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•