Crash [@ nsSVGFETileElement::Filter]

VERIFIED FIXED

Status

()

Core
SVG
P2
critical
VERIFIED FIXED
9 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: jwatt)

Tracking

(Blocks: 1 bug, {crash, testcase, verified1.9.1})

Trunk
x86
Mac OS X
crash, testcase, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

9 years ago
Created attachment 358622 [details]
testcase (crashes Firefox when loaded)

###!!! ASSERTION: Tile overflows surface rect, this code can't handle it: 'instance->GetSurfaceRect().Contains(tile)', file /Users/jruderman/central/content/svg/content/src/nsSVGFilters.cpp, line 2857

Crash [@ nsSVGFETileElement::Filter] trying to access 0x1ed51400.

Bug 448244's testcase triggers the same assertion, but does not crash.
(Reporter)

Updated

9 years ago
Whiteboard: [sg:critical?]
(Assignee)

Updated

9 years ago
Assignee: nobody → jwatt
Flags: blocking1.9.1?
Created attachment 358941 [details] [diff] [review]
patch

This patch will happily stop us from reading bad memory in the feTile case.

I'm wondering if the more fundamental problem though is that we don't clamp mFilterPrimitiveSubregion to be within the filter region. (In the testcase, because the 'y' attribute on the feImage is set to a large value, it's filter primitive subregion lies way outside the filter region.) I'm still trying to figure out whether it ever makes sense to have a filter primitive subregion that is partially or fully outside the filter region.
If we did, that would create visible rendering changes for things like feImage. (Wouldn't it?) And therefore the spec would have to say that clamping should happen.
Attachment #358622 - Flags: superreview+
Attachment #358622 - Flags: review+
Attachment #358941 - Flags: superreview+
Attachment #358941 - Flags: review+
Well the spec seems less clear than it could be, but it does seem to effectively say that. The first sentence of the section on the filter primitive subregion says that x, y, width and height identify the subregion, but it goes on to say in the fourth paragraph:

  All intermediate offscreens are defined to not exceed the intersection of
  x, y, width and height with the filter region. The filter region and any
  of the x, y, width and height subregions are to be set up such that all
  offscreens are made big enough to accommodate any pixels which even partly
  intersect with either the filter region or the x,y,width,height subregions.

  http://www.w3.org/TR/SVG/filters.html#FilterPrimitiveSubRegion

If intermediate offscreens are clipped to the intersection of the filter region and filter primitive subregion, I'm not sure why you wouldn't just clip the filter primitive subregion straight off the bat. In fact I'm not sure why the spec doesn't simply say that the filter primitive subregion is defined to be x, y, width and height clipped to the filter region.

I'm also not sure whether allowing filter primitive subregions AND intermediary offscreens (contrary to spec) to extend outside the filter region is even useful. You could certainly test for that (see the following snippet), but I can't so far think of an example that is both useful and not possible if filter primitive subregions were clipped to the filter region.

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
  <filter id="filter" x="0" y="0" width="100" height="100">
    <feImage x="50" width="100" xlink:href="green.png"/> <!-- native size {100,100} -->
    <feOffset x="-50"/>
  </filter>
  <rect width="100" height="100" fill="red"/>
  <rect width="100" height="100" fill="red" filter="url(#filter)"/>
</svg>
Keywords: checkin-needed
Whiteboard: [sg:critical?] → [sg:critical?] [needs landing]
(In reply to comment #3)
>   All intermediate offscreens are defined to not exceed the intersection of
>   x, y, width and height with the filter region. The filter region and any
>   of the x, y, width and height subregions are to be set up such that all
>   offscreens are made big enough to accommodate any pixels which even partly
>   intersect with either the filter region or the x,y,width,height subregions.

I don't think this quite says what you want it to say; it's just talking about the offscreens, and doesn't tell us to adjust the filter subregion itself. If the spec intent is that the subregions are clamped to the filter region, it really needs to say that.

(And spec text about "offscreens" really should be relegated to non-normative notes, because that's implementation detail that may or may not make sense for a particular implementation and should not make any behavioural difference.)
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Pushed http://hg.mozilla.org/mozilla-central/rev/7b3ae53a4181 and the test as a crashtest http://hg.mozilla.org/mozilla-central/rev/1a63a436e531.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [sg:critical?] [needs landing] → [sg:critical?] [needs 191 landing]
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/81f711716c9b and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/529bf01785e8
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [sg:critical?] [needs 191 landing] → [sg:critical?]
(Assignee)

Updated

9 years ago
Blocks: 448244
Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090211 Shiretoko/3.1b3pre. Adding verified keyword.

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090211 Minefield/3.2a1pre.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Crash Signature: [@ nsSVGFETileElement::Filter]
Group: core-security
You need to log in before you can comment on or make changes to this bug.