RotatedContentBuffer::BeginPaint can return with|result.mClip| uninitialized

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: erahm, Assigned: wlitwinczyk)

Tracking

(Blocks 1 bug, {coverity})

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CID 1157299])

Attachments

(1 attachment)

In various error/early return paths we return a PaintState that has an uninitialized |mClip| member. It's possible that calling code will then use this value.

This highlights two issues:
  #1 - mClip should probably be initialized in PaintState's ctor
  #2 - There should probably be an error state that is checked in calling code. There might actually be one, but we should make sure it's set when returning on error.
Keywords: coverity
If there is any risk associated with this kind of bugs please file them as security sensitive until they have been evaluated.
We have PaintState::mMode == SurfaceMode::SURFACE_NONE as an error state. That's set by the PaintState constructor, and isn't modified to a valid value until the very end of BeginPaint.
May as well just initialize all explicitly.

Walter, if this gets through the review, could you handle getting it landed, and if it doesn't could you fix any issues that come up?
Attachment #8450501 - Flags: review?(matt.woodrow)
Assignee: nobody → wlitwinczyk
Comment on attachment 8450501 [details] [diff] [review]
Initialize all members of RotatedContentBuffer::PaintState

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

::: gfx/layers/RotatedBuffer.h
@@ +208,5 @@
>     */
>    struct PaintState {
>      PaintState()
> +      : mRegionToDraw()
> +      , mRegionToInvalidate()

The region initializers are unnecessary here since they'll use the default constructor implicitly.
Attachment #8450501 - Flags: review?(matt.woodrow) → review+
True, but it doesn't hurt and the "order of initialization is wrong" requirement on the Mac is better enforced.  And if somebody adds another member variables, they'll be shamed into initializing it because all the other ones are in the list.
Try: https://tbpl.mozilla.org/?tree=Try&rev=1a0ad9f71aac

The B2G Desktop Linux x64 Opt <unknown> test is failing across a number of my patches, so I don't think that's specific to this patch.
Well the <unknown> test disappeared, and everything else passes.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d980cd9e3482
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.