Uninitialised value use in IsEmpty (gfx/2d/BaseRect.h:61)

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jseward, Assigned: lsalzman)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
DISPLAY=:1.0 ./mach mochitest \
  --valgrind-args=--show-mismatched-frees=no,--fullpath-after=REL/ \
  --valgrind=/home/sewardj/VgTRUNK/merge/Inst/bin/valgrind \
  dom/canvas/test/test_canvas.html 2>&1 | tee spew-05-mc-11

SkCanvas::getClipBounds is inconsistent in its return conventions.  It
returns a boolean indicating whether or not it succeeded, and it
appears that the original plan was for it to set the out-parameter
|bounds| to a safe value even on failure, so that callers can ignore
the boolean if they want.  But the first return point from the
function doesn't do that: it returns |false| without changing the
out-parameter.

The result is that "Rect GetClipBounds(SkCanvas *aCanvas)" in
DrawTargetSkia.cpp can pass stack-allocated junk back to its caller:

  static inline Rect
  GetClipBounds(SkCanvas *aCanvas)
  {
    SkRect clipBounds;
    aCanvas->getClipBounds(&clipBounds);
    return SkRectToRect(clipBounds);
  }

The uninitialised values get passed around a bit and Valgrind
eventually comments on them (somewhat confusingly) in
BaseRect::IsEmpty (BaseRect.h:61).
(Reporter)

Comment 1

3 years ago
Created attachment 8740888 [details]
Valgrind complaint
(Reporter)

Comment 2

3 years ago
Created attachment 8740890 [details] [diff] [review]
bug1264246-IsEmpty-1.cset

A possible fix.
(Assignee)

Comment 3

3 years ago
(In reply to Julian Seward [:jseward] from comment #2)
> Created attachment 8740890 [details] [diff] [review]
> bug1264246-IsEmpty-1.cset
> 
> A possible fix.

We shouldn't really be modifying Skia for something that is better fixed in our own code.
(Assignee)

Comment 4

3 years ago
Created attachment 8741033 [details] [diff] [review]
verify that SkCanvas::getClipBounds succeeds before using result

This makes GetClipBounds check the result of SkCanvas::getClipBounds, and if it fails return an empty rect like it should be.

Also this deals with an incidental finding that getClipBounds is actually doing quite expensive math, so only calls it in the case we need to check it against mask bounds.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8741033 - Flags: review?(mchang)
Attachment #8741033 - Flags: review?(mchang) → review+

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1442d37b6449
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.