Bug 107093 – UMBRELLA: WTF should build with -Wshorten-64-to-32

• Component: Web Template Framework
• Status: NEW

[David Kilzer (:ddkilzer)]

Created attachment 183118 [details]
Patch

<http://webkit.org/b/000000>
<rdar://problem/12551754>

Reviewed by NOBODY (OOPS!).

Source/JavaScriptCore:

* runtime/JSDateMath.cpp:
(JSC::parseDateFromNullTerminatedCharacters): Change 'offset'
from int to long to match change to function in WTF.  Also
remove now unneeded static_cast<int>().

Source/WTF:

* Configurations/Base.xcconfig:
- Enable -Wshorten-64-to-32 by setting
GCC_WARN_64_TO_32_BIT_CONVERSION to YES.

* wtf/Assertions.cpp:
(vprintf_stderr_common):
- Use CFIndex type instead of int for the return value of
CFStringGetMaximumSizeForEncoding().

* wtf/CryptographicallyRandomNumber.cpp:
(ARC4RandomNumberGenerator::stir):
- Use static_cast<int>() since the value is being used to
generate a hash.

* wtf/DateMath.cpp:
(WTF::isLeapYear):
(WTF::ymdhmsToSeconds):
(WTF::parseDateFromNullTerminatedCharacters):
(WTF::parseDateFromNullTerminatedCharacters):
* wtf/DateMath.h:
(WTF::parseDateFromNullTerminatedCharacters):
(WTF::isLeapYear):
- Change types from int to long since most local variables for
'year', 'month', 'day', 'hour' and 'minute' are already type
long.
- Also changed 'offset' variable from int to long, which made it
possible to remove a static_cast<int>().

* wtf/DecimalNumber.cpp:
(WTF::DecimalNumber::toStringDecimal):
(WTF::DecimalNumber::toStringExponential):
- Add overflow checks and static_cast<unsigned>() as needed.

* wtf/FastMalloc.cpp:
(fastMallocStatistics): Change length from int to size_t.
(PageMapFreeObjectFinder::visit): Add ASSERT and
static_cast<int>().
(PageMapMemoryUsageRecorder::recordPendingRegions): Add ASSERT
and static_cast<unsigned>().
(PageMapMemoryUsageRecorder::visit): Add ASSERT and
static_cast<int>() three times.
(AdminRegionRecorder::recordPendingRegions): Add ASSERT and
static_cast<unsigned>().
- NOTE: These ASSERT macros won't be compiled by default on
Debug builds because FastMalloc is disabled in favor of
system malloc.

* wtf/GregorianDateTime.cpp:
(WTF::GregorianDateTime::setToCurrentLocalTime):
- Add static_cast<int>() to convert tm.tm_gmtoff.

* wtf/MD5.cpp:
(WTF::MD5::addBytes):
- Add overflow check with CRASH() for 'length'.  Replace
'length' with local variable, 'len', that is static_cast to
unsigned.

* wtf/dtoa.cpp:
(WTF::multadd): Change int to size_t since these variables are
only used for iterating.
(WTF::mult): Add static_cast<int>() to BigInt::size() results.
(WTF::lshift): Ditto.
(WTF::cmp): Ditto.
(WTF::diff): Ditto.
(WTF::d2b): Ditto.
(WTF::dtoa): Add ASSERT.  Add static_cast<int32_t>().
(WTF::dtoa): Add overflow check with CRASH() and
static_cast<int>().

* wtf/text/ASCIIFastPath.h:
(WTF::copyLCharsFromUCharSource): Change local variables from
unsigned to size_t.

* wtf/text/AtomicString.h:
(WTF::AtomicString::find): Change 'start' arguments from size_t
to unsigned.
(WTF::operator==): Add overflow check with CRASH() and
static_cast<unsigned>().

* wtf/text/Base64.cpp:
(WTF::base64Encode): Add overflow check with CRASH() and
static_cast<unsigned>().
(WTF::base64Decode): Extract in.size() into local variable.
Change UINT_MAX to std::numeric_limits<unsigned>::max().  Add
static_cast<unsigned>().
* wtf/text/Base64.h:
(WTF::base64Encode): Add overflow check with CRASH() and
static_cast<unsigned>() to all four variants.

* wtf/text/StringBuilder.h:
(WTF::StringBuilder::append): Add overflow check with CRASH()
and static_cast<unsigned>().

* wtf/text/StringConcatenate.h:
(StringTypeAdapter<>): Add overflow check with CRASH() and
static_cast<unsigned>() when setting m_length.  One variant
already had an overflow check.

* wtf/text/StringImpl.cpp:
(WTF::StringImpl::createFromLiteral): Add overflow check with
CRASH() and static_cast<unsigned>().
(WTF::StringImpl::create): Add static_cast<unsigned>() since
the overflow check already exists.
(WTF::StringImpl::getData16SlowCase): Change local 'offset'
variable from unsigned to size_t.
(WTF::StringImpl::removeCharacters): Add static_cast<unsigned>()
since we're removing characters and assume the current object
has already been bounds-checked.
(WTF::StringImpl::find): Add static_cast<unsigned>() since an
overflow check already exists.
(WTF::StringImpl::findIgnoringCase): Ditto.
(WTF::StringImpl::replace): Ditto.

* wtf/text/StringImpl.h:
(WTF::StringImpl::StringImpl): Add static_cast<unsigned>() since
the value is being used to compute a hash.

* wtf/text/WTFString.cpp:
(WTF::String::String): Add static_cast<unsigned>() since the
overflow check already exists.
(WTF::String::split): Add static_cast<unsigned>() since the
current object has already been bounds-checked.
(WTF::String::make8BitFrom16BitSource): Add overflow check with
CRASH() and static_cast<unsigned>().
(WTF::String::make16BitFrom8BitSource): Ditto.
(WTF::String::fromUTF8): Add static_cast<unsigned>() since the
overflow check already exists.  Change 'utf16Length' from unsigned
to size_t.  Static cast for 'utf16Length' is checked by ASSERT.
(WTF::String::fromUTF8WithLatin1Fallback): Add overflow check with
CRASH() and static_cast<unsigned>().
(WTF::lengthOfCharactersAsInteger): Ditto.

* wtf/unicode/icu/CollatorICU.cpp:
(WTF::Collator::collate): Add overflow checks with CRASH() and
static_cast<unsigned>() for 'lhsLength' and 'rhsLength'.
---
 23 files changed, 360 insertions(+), 109 deletions(-)

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/WTF/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WTF/wtf/FastMalloc.cpp:4640:  Missing space after ,  [whitespace/comma] [3]
Source/WTF/wtf/text/AtomicString.h:197:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 23 files


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

[EFL EWS Bot]

Comment on attachment 183118 [details]
Patch

Attachment 183118 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15901920

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> Attachment 183118 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
> Source/WTF/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
> Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
> Source/WTF/wtf/FastMalloc.cpp:4640:  Missing space after ,  [whitespace/comma] [3]
> Source/WTF/wtf/text/AtomicString.h:197:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
> Total errors found: 4 in 23 files

Fixed locally for next patch.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> (From update of attachment 183118 [details])
> Attachment 183118 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/15901920

/Volumes/Data/EWS/WebKit/Source/WTF/wtf/dtoa.cpp:1269:30: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'type' (aka 'int') [-Werror,-Wsign-compare]
    if (finalTruncatedLength > std::numeric_limits<int>::max())
        ~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

[Build Bot]

Comment on attachment 183118 [details]
Patch

Attachment 183118 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15907660

[Benjamin Poulain]

Comment on attachment 183118 [details]
Patch

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

Really nice!

Notes:
-The type long is a huge pain in the ass because of Windows. I am not sure we want to use it more.
-Some of the Characters functions could probably use unsigned instead of size_t. This would avoids the conversions 32-64 bits.
-CRASH() is not necessarily the right thing to do for overflow. Sometime we should report the error to JavaScript. We have to be careful we cannot force getting in one of this branch from JS code.
-If you can split the patch in subparts, it will be easier to review :D

> Source/WTF/ChangeLog:33
> +        (WTF::isLeapYear):
> +        - Change types from int to long since most local variables for
> +          'year', 'month', 'day', 'hour' and 'minute' are already type
> +          long.

Why not go the other way around and use int for this variables?
A long (64 bits on x86_64 for anything but Windows) for year, month and day seems overkill.

> Source/WTF/wtf/CryptographicallyRandomNumber.cpp:103
> -    addRandomData(randomness, length);
> +    addRandomData(randomness, static_cast<int>(length));

It would be better to change addRandomData() to take a size_t.

> Source/WTF/wtf/DecimalNumber.cpp:128
> -        return next - buffer;
> +        size_t length = next - buffer;
> +        if (length > std::numeric_limits<unsigned>::max())
> +            CRASH();
> +
> +        return static_cast<unsigned>(length);

That looks wrong for 32bits where sizeof(size_t) == sizeof(unsigned).

I don't think you need to check for overflow (or maybe just assert for it). If next - buffer > bufferLength, something horribly wrong happened :)

> Source/WTF/wtf/DecimalNumber.cpp:148
> -        return next - buffer;
> +        size_t length = next - buffer;
> +        if (length > std::numeric_limits<unsigned>::max())
> +            CRASH();
> +
> +        return static_cast<unsigned>(length);

ditto.

> Source/WTF/wtf/DecimalNumber.cpp:167
> -    return next - buffer;
> +    size_t length = next - buffer;
> +    if (length > std::numeric_limits<unsigned>::max())
> +        CRASH();
> +
> +    return static_cast<unsigned>(length);

Ditto.

> Source/WTF/wtf/DecimalNumber.cpp:212
> -    return next - buffer;
> +    size_t length = next - buffer;
> +    if (length > std::numeric_limits<unsigned>::max())
> +        CRASH();
> +
> +    return static_cast<unsigned>(length);

Ditto.

> Source/WTF/wtf/FastMalloc.cpp:4491
> +        ASSERT(span->length <= std::numeric_limits<int>::max());
> +        return static_cast<int>(span->length);

Would returning size_t from visit() be an option?

> Source/WTF/wtf/MD5.cpp:223
>  void MD5::addBytes(const uint8_t* input, size_t length)
>  {
> +    if (length > std::numeric_limits<uint32_t>::max())
> +        CRASH();
> +    
> +    uint32_t len = static_cast<uint32_t>(length);

This is odd. Which part fails with a 64bits length?
Is it length < t?

> Source/WTF/wtf/text/AtomicString.h:204
> +    if (bLength > std::numeric_limits<unsigned>::max())
> +        CRASH();
> +    return equal(aImpl, b.data(), static_cast<unsigned>(bLength));

I have a bad feeling about this. I think that particular operator only exists for performance reason.

> Source/WTF/wtf/text/StringConcatenate.h:226
> -        , m_length(strlen(buffer))
>      {
> +        size_t bufferLength = strlen(buffer);
> +        if (bufferLength > std::numeric_limits<unsigned>::max())
> +            CRASH();
> +        m_length = static_cast<unsigned>(bufferLength);

Ouch, this one is unfortunate.
I don't see a way around it. We must make a new adapter for literals.

> Source/WTF/wtf/text/StringImpl.cpp:164
> +    if (length > std::numeric_limits<unsigned>::max())
> +        CRASH();

This should be an ASSERT.

This case is already tested at compile time for the template constructor.

Actually, if (!length) should also be an ASSERT. That is probably an accident.

> Source/WTF/wtf/text/StringImpl.cpp:1409
> +    while ((srcSegmentStart = find(pattern, static_cast<unsigned>(srcSegmentStart))) != notFound) {

Hum, maybe change ::find() to return unsigned, and srcSegmentStart to unsigned?

This code can loop if this->length() == numeric_limits<unsigned>::max(). (but so does the original code).

[Build Bot]

Comment on attachment 183118 [details]
Patch

Attachment 183118 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15908989

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> (From update of attachment 183118 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183118&action=review
> 
> Really nice!
> 
> Notes:
> -The type long is a huge pain in the ass because of Windows. I am not sure we want to use it more.
> -Some of the Characters functions could probably use unsigned instead of size_t. This would avoids the conversions 32-64 bits.
> -CRASH() is not necessarily the right thing to do for overflow. Sometime we should report the error to JavaScript. We have to be careful we cannot force getting in one of this branch from JS code.
> -If you can split the patch in subparts, it will be easier to review :D

You're right, I'll do that instead and use this as an umbrella bug.

Thanks for the quick review feedback!

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> (From update of attachment 183118 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183118&action=review
> 
> > Source/WTF/ChangeLog:33
> > +        (WTF::isLeapYear):
> > +        - Change types from int to long since most local variables for
> > +          'year', 'month', 'day', 'hour' and 'minute' are already type
> > +          long.
> 
> Why not go the other way around and use int for this variables?
> A long (64 bits on x86_64 for anything but Windows) for year, month and day seems overkill.

Much of the code uses strtol() to parse the various pieces of the date string.  Changing the values to int would actually increase the amount of static_cast<int>() operators that would be added to the code.  :/

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> (In reply to comment #6)
> > (From update of attachment 183118 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=183118&action=review
> > 
> > > Source/WTF/ChangeLog:33
> > > +        (WTF::isLeapYear):
> > > +        - Change types from int to long since most local variables for
> > > +          'year', 'month', 'day', 'hour' and 'minute' are already type
> > > +          long.
> > 
> > Why not go the other way around and use int for this variables?
> > A long (64 bits on x86_64 for anything but Windows) for year, month and day seems overkill.
> 
> Much of the code uses strtol() to parse the various pieces of the date string.  Changing the values to int would actually increase the amount of static_cast<int>() operators that would be added to the code.  :/

Actually, I can change parseLong() to parseInt() and put the static_cast<int>() in one place, but I'm also concerned about changing this code and making weird corner cases fail due to lack of precision when switching from long to int.

I'll post both patches in another bug.

[Darin Adler]

(In reply to comment #10)
> Actually, I can change parseLong() to parseInt() and put the static_cast<int>() in one place, but I'm also concerned about changing this code and making weird corner cases fail due to lack of precision when switching from long to int.

To be pedantic, there is no lack of precision when switching from long to int anywhere.

On 64-bit there is a lack of range when converting from long to int, but it need not be a deep subject. If the value can be out of a 32-bit number range, then we need code that properly clamps it in range, which is usually not all that challenging to write. If it can’t be out of 32-bit integer range, then there is no problem and no code is needed.

[Oliver Hunt]

I have a patch that does this and adds a checked_cast<> function that will crash on overflow

[Oliver Hunt]

Created attachment 226825 [details]
Patch

[Sam Weinig]

Comment on attachment 226825 [details]
Patch

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

> Source/WTF/Configurations/Base.xcconfig:66
>  // FIXME: <http://webkit.org/b/107093> WTF should build with -Wshorten-64-to-32

You can get rid of this FIXME.

[Oliver Hunt]

(In reply to comment #14)
> (From update of attachment 226825 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226825&action=review
> 
> > Source/WTF/Configurations/Base.xcconfig:66
> >  // FIXME: <http://webkit.org/b/107093> WTF should build with -Wshorten-64-to-32
> 
> You can get rid of this FIXME.

This is a very early run through of WTF -- i've not even run perf tests on it O_o

[Darin Adler]

Comment on attachment 226825 [details]
Patch

Clearing the review flag on this year old patch.