Closed Bug 1157669 Opened 5 years ago Closed 5 years ago

Possible null pointer dereference in BasicLayerManager::PaintSelfOrChildren

Categories

(Core :: Graphics: Layers, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: nical, Assigned: amanda.sambath, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1244474])

Attachments

(1 file, 1 obsolete file)

https://hg.mozilla.org/mozilla-central/file/a5af73b32ac8/gfx/layers/basic/BasicLayerManager.cpp#l793

aGroupTarget can be null, and we have at least one branch depending on whether it is null before we call PaintSelfOrchildren here: https://hg.mozilla.org/mozilla-central/file/a5af73b32ac8/gfx/layers/basic/BasicLayerManager.cpp#l914

We should either
* early-return if aGroupTarget is null.
* MOZ_ASSERT that aGroupTarget is not null and fix the callers of PaintSelfOrChildren.
Keywords: coverity
I would like to fix this bug.
Attached patch Build successful (obsolete) — Splinter Review
Attachment #8608010 - Flags: review?(nical.bugzilla)
Assignee: nobody → amanda.sambath
Comment on attachment 8608010 [details] [diff] [review]
Build successful

Review of attachment 8608010 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, it'd be nice if you could add a MOZ_ASSERT(aTarget) at the top of BasicLayerManager::PaintLayer, to make it explicit that removing that branch, and the assertion in PaintSelfOrChildren hold because we expect PaintLayer to be called with a non-null target (an no need to add an assertion message for a simple null-check like this one).

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +792,5 @@
>  void
>  BasicLayerManager::PaintSelfOrChildren(PaintLayerContext& aPaintContext,
>                                         gfxContext* aGroupTarget)
>  {
> +  MOZ_ASSERT(aGroupTarget, "aGroupTarget must not be null");

nit: no need to paraphrase the test if the comment doesn't add information.
MOZ_ASSERT(aGroupTarget); will compile just fine.
Attachment #8608010 - Flags: feedback+
Attached patch 1157669.patchSplinter Review
Attachment #8608010 - Attachment is obsolete: true
Attachment #8608010 - Flags: review?(nical.bugzilla)
Attachment #8608706 - Flags: review?(nical.bugzilla)
Attachment #8608706 - Flags: review?(nical.bugzilla) → review+
Keywords: coveritycheckin-needed
https://hg.mozilla.org/mozilla-central/rev/fd77898fed81
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Keywords: coverity
Whiteboard: [CID 1244474]
You need to log in before you can comment on or make changes to this bug.