Bug 104955 – Clamp values in LayoutUnit::operator/ when SATURATED_LAYOUT_ARITHMETIC is enabled

• Component: Platform
• Status: RESOLVED FIXED

[Emil A Eklund]

LayoutUnit::setRawValue(long long) currently does not clamp values and instead overflows when given a value greater than INT_MAX or less than INT_MIN. As the division operator uses the setRawValue(long long) method it can the value to overflow, even if SATURATED_LAYOUT_ARITHMETIC is enabled.

[Emil A Eklund]

Created attachment 179341 [details]
Patch

[WebKit Review Bot]

Attachment 179341 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Tools/TestWebKitAPI/ForwardingHeaders/WebCore/LayoutUnit.h:1:  #ifndef header guard has wrong style, please use: LayoutUnit_h  [build/header_guard] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Emil A Eklund]

The style bot failure looks like a false positive in that it doesn't recognize FWD headers.

[Julien Chaffraix]

Comment on attachment 179341 [details]
Patch

I like the change, especially since it catches an issue. Some comments on it, the ForwardingHeaders part looks fine to me but someone more knowledgeable with the Apple build system should probably comment.

[Emil A Eklund]

(In reply to comment #4)
> (From update of attachment 179341 [details])
> I like the change, especially since it catches an issue. Some comments on it, the ForwardingHeaders part looks fine to me but someone more knowledgeable with the Apple build system should probably comment.

The ForwardingHeaders setup is for the linux windows/ninja build, mac does fine without it.

[Emil A Eklund]

Created attachment 179362 [details]
Patch

[WebKit Review Bot]

Attachment 179362 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Tools/TestWebKitAPI/ForwardingHeaders/WebCore/LayoutUnit.h:1:  #ifndef header guard has wrong style, please use: LayoutUnit_h  [build/header_guard] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Emil A Eklund]

Created attachment 179509 [details]
Patch

[WebKit Review Bot]

Attachment 179509 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Tools/TestWebKitAPI/ForwardingHeaders/WebCore/LayoutUnit.h:1:  #ifndef header guard has wrong style, please use: LayoutUnit_h  [build/header_guard] [5]
Tools/TestWebKitAPI/ForwardingHeaders/WebCore/KURL.h:1:  #ifndef header guard has wrong style, please use: KURL_h  [build/header_guard] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Chaffraix]

Comment on attachment 179509 [details]
Patch

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

Looks fine, some comment. Please, leave some time before landing to get a green EWS and in case some of people more knowledgeable in the different hardware we support can double check.

> Source/WebCore/platform/LayoutUnit.h:182
> +        m_value = clampTo(value, std::numeric_limits<int>::min(), std::numeric_limits<int>::max());

We should probably mention that this may lead to a loss of precision if long long is 64 bits but as we are casting the information to an int (32 bits on all 64 bits architecture that we know about), it shouldn't matter.

Also, unless I am mistaking, this looks like it could be written: m_value = clampTo<int>(value);
(clampToInteger would be ambiguous AFAICT so it would require an unfortunate cast)

> Tools/TestWebKitAPI/ForwardingHeaders/WebCore/KURL.h:4
> +#endif

Nit: We should probably wait until we enable the test to land that

> Tools/TestWebKitAPI/TestWebKitAPI.gyp/TestWebKitAPI.gyp:57
> +                '<(source_dir)/WebCore/WebCore.gyp/WebCore.gyp:webcore',

For archive's sake, we discussed that over IM and it makes more sense to link against WebCore if we are to enable the test for KURL but also to test any class that isn't fully implemented in its header file.

> Tools/TestWebKitAPI/Tests/WebCore/LayoutUnit.cpp:79
> +    ASSERT_NEAR(LayoutUnit(1.0f).toFloat(), 1.0f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(1.1f).toFloat(), 1.1f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(1.25f).toFloat(), 1.25f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(1.33f).toFloat(), 1.33f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(1.3333f).toFloat(), 1.3333f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(1.53434f).toFloat(), 1.53434f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(345634).toFloat(), 345634.0f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(345634.12335f).toFloat(), 345634.12335f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(-345634.12335f).toFloat(), -345634.12335f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(-345634).toFloat(), -345634.0f, tolerance);    

Are these ASSERT_NEAR really needed? Below you use ASSERT_FLOAT_EQ for the division and I wouldn't expect much loss in precisions for most of numbers here as they don't require much mantissa bits to represent.

[Benjamin Poulain]

Nice catch. (and nice of you to add test support for LayoutUnit).

I agree with Julien, I would much prefer this with clampTo<int>(value);.

I'd argue (as usual? :)) that this not the best place to fix this. Most code calling setRawValue() already ensure the value is bounded. By adding this addition clamp in setRawValue(), people might misunderstand how to use that API.

The perf guy in me also cry because we branch twice as much when the call site do a proper job with saturated arithmetic.

What do you think about this?:
-make setRawValue a template
-make it so that it fails to compile for long with a explanatory error message.
-fix the 1 (?) faulty call site.
-....
-profit

[Emil A Eklund]

(In reply to comment #11)
> Nice catch. (and nice of you to add test support for LayoutUnit).
> 
> I agree with Julien, I would much prefer this with clampTo<int>(value);.
> 
> I'd argue (as usual? :)) that this not the best place to fix this. Most code calling setRawValue() already ensure the value is bounded. By adding this addition clamp in setRawValue(), people might misunderstand how to use that API.
> 
> The perf guy in me also cry because we branch twice as much when the call site do a proper job with saturated arithmetic.

You are right, I'll move it to the call site to minimize the perf impact. That'll also make it more consistent with the existing checks.

Thanks for your input, always appreciated!

[Emil A Eklund]

Committed r137924: <http://trac.webkit.org/changeset/137924>

[Simon Fraser (smfr)]

This broke some WebKit API tests. Please fix:
http://build.webkit.org/builders/Apple%20Lion%20Debug%20WK1%20%28Tests%29/builds/5417/steps/run-api-tests/logs/stdio

[Emil A Eklund]

(In reply to comment #14)
> This broke some WebKit API tests. Please fix:
> http://build.webkit.org/builders/Apple%20Lion%20Debug%20WK1%20%28Tests%29/builds/5417/steps/run-api-tests/logs/stdio

Fixed in https://bugs.webkit.org/show_bug.cgi?id=105332