Bug 61603 – FEConvolveMatrix::getPixelValue() fails to properly check if y is within bounds, causing it to fail to correctly apply the kernel and edge mode to the first targetY pixels

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Sleevi]

FEConcolveMatrix::getPixelValue() is range checking x against height, instead of y, when determining if a pixel is in bounds

[Ryan Sleevi]

Created attachment 95110 [details]
Patch

[Ryan Sleevi]

This was reported against Chromium by a user running a static analyzer (PVS Studio) against the build. The downstream bug is http://code.google.com/p/chromium/issues/detail?id=84139

The original text of the user report was:

V501 There are identical sub-expressions 'x >= 0' to the left and to the right of the '&&' operator. webcore_platform feconvolvematrix.cpp 289

ALWAYS_INLINE int
FEConvolveMatrix::getPixelValue(PaintingData& paintingData, int x, int y)
{
  if (x >= 0 && x < paintingData.width && x >= 0 && y < paintingData.height)
    return (y * paintingData.width + x) << 2;
  ...
}

[Darin Adler]

Comment on attachment 95110 [details]
Patch

Seems straightforward to construct a test case. We normally require test cases for bugs. Could you add one please?

[Nikolas Zimmermann]

Comment on attachment 95110 [details]
Patch

Good catch, though we need a test for this. Can you construct a test, that leads to a crash here? (somehow possible to get y to a negative value via a test?) CC'ing Zoltan.

[Nikolas Zimmermann]

CC'ing Zoltan & Reni to comment on how to possible construct a testcase, or whether it's not possible.

[Ryan Sleevi]

Created attachment 95144 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 95144 [details]
Patch

Layout test results as well as pixel test results are missing.

I guess this is a crash testcase, so we won't need a pixel test result nor a render tree dump. So I suggest you to include this before the <defs> section:
<script>
if (window.layoutTestController)
    layoutTestController.dumpAsText();
</script>

And create a text node somewhere in the document saying "Passes, if it doesn't crash", so the "feConvolveFilter-bounds-expected.txt" file includes just this text.

I guess this is a crash test, you haven't explained the test in the ChangeLog, you should do that, and also rename to test to sth like "feConvoleMatrix-y-bounds-crash.svg".
I am not sure though how this testcase triggers the bug? SHouldn't y be negative??

Last note: The ChangeLog is wrong, you have to create individual change logs once for LayoutTests/ChangeLog, once for Source/WebCore/ChangeLog.
Easiest is to use "prepare-ChangeLog --bug=<bugNumber>" in the root directory of your checkout, it will take care of using the templates for you.

Looking forward to the next patch :-) The first webkit contribution is always tricky, so no worries...

[Ryan Sleevi]

I've attached a simple test that generates visibly different results with and without the fix.

First, the logic begins with setOuterPixels/fastSetOuterPixels called with x1=0,y1=0,x1=width,y1=targetY in FEConvolveMatrix::apply()

In a 3x3 kernelMatrix, order is 3 3, and the default targetY of floor(3/2) == 1 is used.

When fastSetOuterPixels is called, it initializes startKernelPixelY = y1 - targetY. For the first row, this is 0 - 1, or startKernelPixelY = -1.

During the inner-most loop of fastSetOuterPixels, it calls getPixelValue with kernelPixelX, kernelPixelY. The initial value of kernelPixelY is startKernelPixelY, or -1.

As a result, getPixelValue is invoked with (-1,-1) for the pixel at (0,0), (0,-1) for the pixel at (1, 0), etc. For the entire row, y is -1

Note, this is *NOT* a crash. This is because pixelIndex, returned as a negative value, is *not* used in fastSetOuterPixels. Instead, fastSetOuterPixels performs no illegal accesses due to pixelIndex being negative - it is properly checked at all times. However, because of the negative value, the totals are all 0, instead of their expected values, which is the result of the pixel from the row beneath. This is why I uploaded a pixel test.

[Ryan Sleevi]

Since the ChangeLog will be crucial to getting this accepted, can you please point out what I might be doing wrong? I've been using webkit-patch and prepare-ChangeLog this entire time, and it seems it will not generate the change logs in the format you've requested

Ryan-Sleevis-Mac-mini:WebKit Ryan$ ./Tools/Scripts/prepare-ChangeLog --bug=61603
  Running status to find changed, added, or removed files.
?       Source/WebKit/chromium/WebKit.xcodeproj
?       Source/WebCore/WebCore.gyp/idls_list_temp_file.tmp
?       Source/ThirdParty/glu/glu.xcodeproj
?       LayoutTests/platform/chromium-mac/svg/filters/feConvolveFilter-bounds-expected.txt
?       LayoutTests/platform/chromium-mac/svg/filters/feConvolveFilter-bounds-expected.png
  Reviewing diff to determine which lines changed.
svn: '../../Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp' is not a working copy
svn: '../../Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp' does not exist
  Change author: Ryan Sleevi <[e-mail]>.
  Description from bug 61603:
    "FEConcolveMatrix::getPixelValue() is range checking x against height, instead of y, when determining if a pixel is in bounds".
  Running 'svn update' to update ChangeLog files.
    At revision 87486.
  Editing the ./ChangeLog file.
-- Please remember to include a detailed description in your ChangeLog entry. --
-- See <http://webkit.org/coding/contributing.html> for more info --

I've also tried with Tools/Scripts/ in my $PATH. I get the exact same results. I've also tried explicitly specifying the SVN directory via a relative path (prepare-ChangeLog can't handle absolute paths) to the same result.

Is there some other tool or guidance about how a change log should be formatted, if I needed to do it by hand?

[Nikolas Zimmermann]

(In reply to comment #8)
> I've attached a simple test that generates visibly different results with and without the fix.
> 
> Note, this is *NOT* a crash. This is because pixelIndex, returned as a negative value, is *not* used in fastSetOuterPixels. Instead, fastSetOuterPixels performs no illegal accesses due to pixelIndex being negative - it is properly checked at all times. However, because of the negative value, the totals are all 0, instead of their expected values, which is the result of the pixel from the row beneath. This is why I uploaded a pixel test.

Ok, great, please include a pixel test and the render tree dump generated with a vanilla mac. They have to be present in the patch, all other results can be generated - if needed - by using "webkit-patch rebaseline" after the patch has landed, if it generates different results on other platforms.


(In reply to comment #9)
> Since the ChangeLog will be crucial to getting this accepted, can you please point out what I might be doing wrong? I've been using webkit-patch and prepare-ChangeLog this entire time, and it seems it will not generate the change logs in the format you've requested?
> 
> Is there some other tool or guidance about how a change log should be formatted, if I needed to do it by hand?
It's really weird. I have never had trouble with it, maybe Eric Seidel or Adam Barth can help with your specific tool problem?

What happens if you invoke prepare-ChangeLog right in Source/WebCore?
I guess this is not a vanilla WebKit checkout, but a Chromium/Mac environment you're working in, I'm not familiar with that though. Maybe chromium folks on IRC can help you.

[Nikolas Zimmermann]

(In reply to comment #9)
>   Reviewing diff to determine which lines changed.
> svn: '../../Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp' is not a working copy
> svn: '../../Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp' does not exist
^^^ Totally overlooked this before, your checkout seems broken.
You should try fixing this up (what does svn st say in Source/WebCore/platform/graphics/filters?) or re-checkout.

[Ryan Sleevi]

(In reply to comment #11)
> (In reply to comment #9)
> >   Reviewing diff to determine which lines changed.
> > svn: '../../Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp' is not a working copy
> > svn: '../../Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp' does not exist
> ^^^ Totally overlooked this before, your checkout seems broken.
> You should try fixing this up (what does svn st say in Source/WebCore/platform/graphics/filters?) or re-checkout.

Ryan-Sleevis-Mac-mini:filters Ryan$ svn st
M       FEConvolveMatrix.cpp

From the top-level WK dir (note, reverted ChangeLog to try to figure out what is up)
Ryan-Sleevis-Mac-mini:WebKit Ryan$ svn st
?       Source/WebKit/chromium/WebKit.xcodeproj
?       Source/WebCore/WebCore.gyp/idls_list_temp_file.tmp
M       Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp
?       Source/ThirdParty/glu/glu.xcodeproj
?       LayoutTests/platform/chromium-mac/svg/filters/feConvolveFilter-bounds-expected.txt
?       LayoutTests/platform/chromium-mac/svg/filters/feConvolveFilter-bounds-expected.png
A       LayoutTests/svg/filters/feConvolveFilter-bounds.svg

[Nikolas Zimmermann]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > >   Reviewing diff to determine which lines changed.
> > > svn: '../../Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp' is not a working copy
> > > svn: '../../Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp' does not exist
> > ^^^ Totally overlooked this before, your checkout seems broken.
> > You should try fixing this up (what does svn st say in Source/WebCore/platform/graphics/filters?) or re-checkout.
> 
> Ryan-Sleevis-Mac-mini:filters Ryan$ svn st
> M       FEConvolveMatrix.cpp
> 
> From the top-level WK dir (note, reverted ChangeLog to try to figure out what is up)
> Ryan-Sleevis-Mac-mini:WebKit Ryan$ svn st
> ?       Source/WebKit/chromium/WebKit.xcodeproj
> ?       Source/WebCore/WebCore.gyp/idls_list_temp_file.tmp
> M       Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp
> ?       Source/ThirdParty/glu/glu.xcodeproj
> ?       LayoutTests/platform/chromium-mac/svg/filters/feConvolveFilter-bounds-expected.txt
> ?       LayoutTests/platform/chromium-mac/svg/filters/feConvolveFilter-bounds-expected.png
> A       LayoutTests/svg/filters/feConvolveFilter-bounds.svg

I'm still not sure why prepare-ChangeLog tries to lookup "../../Source/WebCore" if you invoke it right from the top level webkit checkout directory, it should look at "Source/WebCore" instead, hmm.

[Ryan Sleevi]

Created attachment 95252 [details]
Patch

[Ryan Sleevi]

(In reply to comment #14)
> Created an attachment (id=95252) [details]
> Patch

While abarth tried to help me set this up as a non-pixel-dependent test, he was unsuccessful. After much headache, I believe the above patch+test case should provide a sufficient reduction for the problem. ToT, Chrome 13.0.772 (WK base 86955), and Safari 5.0.5 each show a noticeable black band in the middle due to the improper bounds check. With the correct check, it appears as attached - a solid green block (with a suitable amount of blue at the top to distinguish).

[Nikolas Zimmermann]

Comment on attachment 95252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95252&action=review

r-, because of a test issue, but it's easy to fix:

> LayoutTests/svg/filters/feConvolveFilter-y-bounds.svg:7
> +

Patch looks almost perfect, though I'd love to have a comment here stating how the result should look like.

Even better, you'd maybe include a blue <line> and a green <rect> below, that looks the same, and add a comment "Both shapes should look identical" (but only in the file as comment, not as <text> element, to get a cross-platform pixel test result, if possible).

[Ryan Sleevi]

Created attachment 95274 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 95274 [details]
Patch

Rejecting attachment 95274 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-..." exit_code: 2

Last 500 characters of output:
tests/xmlhttprequest ................................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers ...........
http/tests/xmlviewer .
http/tests/xmlviewer/dumpAsText ...........
820.29s total testing time

23679 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
17 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8740938

[WebKit Commit Bot]

Created attachment 95306 [details]
Archive of layout-test-results from cr-jail-4

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: cr-jail-4  Port: Mac  Platform: Mac OS X 10.6.7

[WebKit Commit Bot]

Comment on attachment 95274 [details]
Patch

Clearing flags on attachment: 95274

Committed r87680: <http://trac.webkit.org/changeset/87680>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.