Bug 224835 - Fix handling of overflow of /= and *= operators with double over Checked<uint64_t> and other 64-bit types.
Summary: Fix handling of overflow of /= and *= operators with double over Checked<uint...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-20 15:32 PDT by Basuke Suzuki
Modified: 2021-04-27 13:24 PDT (History)
12 users (show)

See Also:


Attachments
Patch (7.63 KB, patch)
2021-04-25 13:02 PDT, Darin Adler
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (9.10 KB, patch)
2021-04-25 13:13 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2021-04-20 15:32:02 PDT
When building with clang 11 for PlayStation, it generates warning like this:

WTF/Headers\wtf/CheckedArithmetic.h:836:58: warning: implicit conversion from 'std::numeric_limits<unsigned long>::_Ty' (aka 'unsigned long') to 'double' changes value from 18446744073709551615 to 18446744073709551616 [-Wimplicit-const-int-float-conversion]
        if (!(std::numeric_limits<T>::min() <= result && std::numeric_limits<T>::max() >= result))
                                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~
../../Tools/TestWebKitAPI/Tests/WTF/CheckedArithmeticOperations.cpp:511:10: note: in instantiation of member function 'WTF::Checked<uint64_t, WTF::RecordOverflow>::operator/=' requested here
    size /= 10.5;
         ^
1 warning generated.

In file included from WebCore/DerivedSources/unified-sources/UnifiedSource-043dd90b-5.cpp:8:
../../Source/WebCore\rendering/RenderBlockFlow.cpp:1785:25: warning: implicit conversion from 'const int' to 'const float' changes value from 33554431 to 33554432 [-Wimplicit-const-int-float-conversion]
        logicalOffset = intMaxForLayoutUnit;
                      ~ ^~~~~~~~~~~~~~~~~~~
1 warning generated.

https://github.com/WebKit/WebKit/blob/f3be0ab411b677048ba40b760a6a65760ca64c1c/Source/WTF/wtf/CheckedArithmetic.h#L836

It says when converting the maximum number of unsigned long (8 byte) which is 0xffff'ffff'ffff'ffff (18'446'744'073'709'551'615) to double, that value is converted to 18'446'744'073'709'551'616 which is 0x1'0000'0000'0000'0000. This won't catch the case when result is 0x1'0000'0000'0000'0000.
Comment 1 Basuke Suzuki 2021-04-20 15:32:34 PDT
Early discussion was on this bug. https://bugs.webkit.org/show_bug.cgi?id=224797
Comment 2 Basuke Suzuki 2021-04-20 15:34:04 PDT
We need to make sure the warning is correct by adding the test case for 64-bit used with Checked type.

If that is proofed, we need to seek how we can resolve this.
Comment 3 Darin Adler 2021-04-20 15:38:52 PDT
The title here is too oblique. The issue is not the precision. The issue is that we will fail to correctly do the overflow check! We will allow overflow!
Comment 4 Basuke Suzuki 2021-04-20 16:07:57 PDT
How about this title?
Comment 5 Darin Adler 2021-04-20 18:07:33 PDT
Better!
Comment 6 Darin Adler 2021-04-24 10:30:11 PDT
I suggest we use this code that selects either double or long double based on the size of the integer.

    std::conditional<sizeof(m_value) <= 4, double, long double>::type result = rhs;
    result *= m_value;

Note the importance of converting rhs to long double *before* doing the multiplication. I’m grabbing this bug and will fix it.
Comment 7 Darin Adler 2021-04-25 09:59:26 PDT
(In reply to Darin Adler from comment #6)
> I suggest we use this code that selects either double or long double based
> on the size of the integer.

I learned more about the problem. That technique will *not* work. Fixing this is harder than it looks.
Comment 8 Darin Adler 2021-04-25 12:05:20 PDT
I had a hard enough timing fixing this that I considered just deleting the *= overloads for double and float, since the rest of CheckedArithmetic.h doesn’t have any floating point type support.

When I tried that (and it still might be the best solution to this annoyingly thorny problem), I found one call site that is using this, which may be the *only* call site using it, GPUBindGroupAllocator::reallocate:

    newLength *= 1.25

I think now that I should just rewrite this to:

    newLength += newLength / 4;
Comment 9 Darin Adler 2021-04-25 13:02:07 PDT
Created attachment 427009 [details]
Patch
Comment 10 Darin Adler 2021-04-25 13:13:23 PDT
Created attachment 427010 [details]
Patch
Comment 11 Radar WebKit Bug Importer 2021-04-25 13:14:17 PDT
<rdar://problem/77126529>
Comment 12 EWS 2021-04-25 15:59:11 PDT
Committed r276577 (237013@main): <https://commits.webkit.org/237013@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427010 [details].
Comment 13 Basuke Suzuki 2021-04-26 14:18:06 PDT
Thanks Darin. Removing this feature is the best way. Less feature, less bug.


(In reply to Darin Adler from comment #6)
> I suggest we use this code that selects either double or long double based
> on the size of the integer.
> 
>     std::conditional<sizeof(m_value) <= 4, double, long double>::type result
> = rhs;
>     result *= m_value;
> 
> Note the importance of converting rhs to long double *before* doing the
> multiplication. I’m grabbing this bug and will fix it.

Can I ask what was wrong with this trial? Just curious about that. I see platform issue on long double. Are there any others?
Comment 14 Darin Adler 2021-04-26 15:53:41 PDT
The result needs to match what C++ specifies for "64-bit integer * double", which is "convert 64-bit integer to double, multiply by double, convert back to 64-bit integer, truncating". If you use long double, you get a different result. It’s not OK to just get a different result. We want the correct result, but also correctly checked for overflow.

So using "long double" lets you compute something *different* and range check that, but it doesn’t let you compute what "64-bit-int *= double" would do, with the expected loss of precision.

Even more extreme is what "64-bit-int *= float" is supposed to do in terms of loss of precision. Same issue.

When I wrote test cases I became sensitive to what it would take to do this absolutely correctly, reading up what the C++ standard specifies. In my view it’s important that the checked version does the same operation that the non-checked equivalent would do, but also checks for overflow.
Comment 15 Basuke Suzuki 2021-04-27 13:24:03 PDT
Got it. Thanks for clarification.