922593 - Signed integer overflows in content/svg/content/src/SVGFEGaussianBlurElement.cpp

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Christian Holler (:decoder)]

The following parts of our codebase show signed integer overflows at runtime (when running our tests, based on mozilla-inbound 8bf84234319a):

# Format is: Location, Error, Test that triggered it (each in a single line)

content/svg/content/src/SVGFEGaussianBlurElement.cpp:141
runtime error: signed integer overflow: 603 * 4210752 cannot be represented in type 'int'
/tests/editor/libeditor/html/tests/test_bug520189.html

content/svg/content/src/SVGFEGaussianBlurElement.cpp:160
runtime error: signed integer overflow: 159 * 16843009 cannot be represented in type 'int'
file:///builds/slave/test/build/tests/reftest/tests/layout/svg/crashtests/322215-1.svg

content/svg/content/src/SVGFEGaussianBlurElement.cpp:149
runtime error: signed integer overflow: 844 * 2807168 cannot be represented in type 'int'
file:///builds/slave/test/build/tests/reftest/tests/content/svg/content/src/crashtests/369568-1.svg


Signed integer overflows are undefined behavior according to the C/C++ standard and discouraged by the CERT Secure Coding Standard. Even if the overflow itself is intended/checked (which should also be verified), we should not rely on the result, nor on any checks performed with the result after the computation. In the optimization stage, the compiler may assume overflow is not happening and produce wrong results as well as eliminate further checks, recognized as dead code. For further information, see the tracking bug.

I quickly took a look at the code itself and it looks like a computation for pixels to display, so this should not be security-sensitive in any way. Still it would be good to fix the undefined behavior, also because it could cause display errors or rendering differences.

Putting needinfo on terrence according to hg blame for that particular code. If you're not the right person to take a look, could you please suggest someone? :) Thanks!

Comment 1

[Daniel Holbert [:dholbert]]

After digging past the mysterious bogus hg blame for JS patches that didn't actually modify the code (?), it looks like roc was last to touch this code (albeit in 2008), in http://hg.mozilla.org/mozilla-central/rev/baa5a51b7f90 .

(At that point, this code lived in nsSVGFilters.cpp )

He and longsonr (who reviewed) are most likely to understand this code.

Having said that: from looking at it myself, I think this (from comment 0) is correct:
{
> it looks like a computation for pixels to display
> so this should not be security-sensitive in any way.
}

I suppose we could change the OUTPUT() definition to test whether (INT32_MAX / scaledDivisor >= sums[j]) before doing the multiplication, and if it is, then just use INT32_MAX instead of multiplying.

roc/longsonr, any thoughts?

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> After digging past the mysterious bogus hg blame for JS patches that didn't
> actually modify the code (?)
(for the record: that bogus-blame was from broken "hg rebase" in old hg versions, and was fixed for future pushes in bug 846953)

Comment 3

[Robert Longson [:longsonr]]

Attachment: overflow.txt

Does this fix it?

Comment 4

[Christian Holler (:decoder)]

Comment on attachment 812828 [details] [diff] [review]
overflow.txt

Confirmed to fix it, thanks! :)

Comment 5

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=086a5d4cae60

Comment 6

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00d04d43079d

Comment 7

[Robert Longson [:longsonr]]

FWIW there's no security impact here, the worst that can happen is that something gets rendered incorrectly.

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/00d04d43079d