Bug 224835 – Fix handling of overflow of /= and *= operators with double over Checked<uint64_t> and other 64-bit types.

• Component: Platform
• Status: RESOLVED FIXED

[Basuke Suzuki]

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.

[Basuke Suzuki]

Early discussion was on this bug. https://bugs.webkit.org/show_bug.cgi?id=224797

[Basuke Suzuki]

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.

[Darin Adler]

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!

[Basuke Suzuki]

How about this title?

[Darin Adler]

Better!

[Darin Adler]

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.

[Darin Adler]

(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.

[Darin Adler]

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;

[Darin Adler]

Created attachment 427009 [details]
Patch

[Darin Adler]

Created attachment 427010 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/77126529>

[EWS]

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].

[Basuke Suzuki]

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?

[Darin Adler]

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.

[Basuke Suzuki]

Got it. Thanks for clarification.