Bug 163762 – IntSize::area() should used checked arithmetic

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[David Kilzer (:ddkilzer)]

InSize::area() should used checked arithmetic since m_width and m_height are integers, and area() currently returns an unsigned value, and area() is frequently multiplied by 4 when used.

[David Kilzer (:ddkilzer)]

Created attachment 292317 [details]
Patch v1

[Build Bot]

Comment on attachment 292317 [details]
Patch v1

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

New failing tests:
plugins/large-plugin-crash.html

[Build Bot]

Created attachment 292321 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Radar WebKit Bug Importer]

<rdar://problem/28885875>

[David Kilzer (:ddkilzer)]

Created attachment 292327 [details]
Patch v2

[Brent Fulgham]

Comment on attachment 292327 [details]
Patch v2

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

I think these changes look good. We need a WK2 reviewer to complete. r=me for non-WK2 parts.

> Source/WebCore/html/ImageData.cpp:116
> +    , m_data(Uint8ClampedArray::createUninitialized((size.area() * 4).unsafeGet()))

I wonder if it would be better to say "sizeof(RGBA32)" instead of "4" in all of these places.

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:87
> +    memcpy(result->data(), pixels, area);

Funny that we were doing this calculation twice before!

[Darin Adler]

Comment on attachment 292327 [details]
Patch v2

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

> Source/WebCore/html/MediaElementSession.cpp:681
> +    return elementRectInMainFrame.area().unsafeGet() > totalElementArea / 2;

Looking at all this code using Checked makes it clear that unsafeGet is the wrong name for that function. That function terminates the process if there was overflow. That’s not “unsafe” in the sense we normally mean it. In fact, we recently named a function safeCast and we gave it the same behavior as unsafeGet, abort the process if something is wrong.

> Source/WebCore/platform/graphics/IntSize.h:134
> +    Checked<unsigned> area() const

Using checked arithmetic to compute the area is clearly the right thing to do. Having a function that is named simply area, and returns a Checked<>, when we have almost no other functions like that in all of WebKit, is not an obvious pattern. I would suggest renaming it checkedArea instead.

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:50
> -    Checked<unsigned, RecordOverflow> area = 4 * rect.area();
> -    if (area.hasOverflowed())
> +    if (rect.unclampedArea() > std::numeric_limits<unsigned>::max() / 4)
>          return nullptr;
> +    unsigned area = (rect.area() * 4).unsafeGet();

This is inelegant. Checked is doing overflow checking, and here we do the same overflow checking a second time so that we can detect that case and return nullptr. Is there a way to write this without having to do our own hand check that exactly matches what Checked is doing?

> Source/WebKit2/WebProcess/Plugins/PluginProxy.cpp:613
> +    if (backingStoreSize.unclampedArea() > std::numeric_limits<unsigned>::max() / 4) {
> +        m_backingStore = nullptr;
> +        return false;
> +    }

Same comment as above. Can we find a way to use Checked to do this rather than writing code to do same check? Also, I think this code needs a comment. It’s not at all clear why this check is the correct one.

[David Kilzer (:ddkilzer)]

Comment on attachment 292327 [details]
Patch v2

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

>> Source/WebCore/html/ImageData.cpp:116
>> +    , m_data(Uint8ClampedArray::createUninitialized((size.area() * 4).unsafeGet()))
> 
> I wonder if it would be better to say "sizeof(RGBA32)" instead of "4" in all of these places.

I inferred from Simon's review of the first patch that these "4" values aren't all necessarily used for the same reason.  Is that not the case?

Also, "typedef unsigned RGBA32;" has a comment that's it's deprecated in Source/WebCore/platform/graphics/Color.h, so I'm not sure if we should be adding more uses of it?

>> Source/WebCore/html/MediaElementSession.cpp:681
>> +    return elementRectInMainFrame.area().unsafeGet() > totalElementArea / 2;
> 
> Looking at all this code using Checked makes it clear that unsafeGet is the wrong name for that function. That function terminates the process if there was overflow. That’s not “unsafe” in the sense we normally mean it. In fact, we recently named a function safeCast and we gave it the same behavior as unsafeGet, abort the process if something is wrong.

I think the mismatch comes from the difference between Checked<unsigned, CrashOnOverflow> (which is the default for Checked<unsigned>) and Checked<unsigned, RecordOverflow>.

The Checked<unsigned>)::unsafeGet() method is poorly named since it's always safe (since we would have crashed before it was called if there was overflow), but Checked<unsigned, RecordOverflow>::unsafeGet() might actually crash if one didn't check the value of Checked<unsigned, RecordOverflow>::hasOverflowed() first.

Do you have a proposal for fixing this mismatch?

>> Source/WebCore/platform/graphics/IntSize.h:134
>> +    Checked<unsigned> area() const
> 
> Using checked arithmetic to compute the area is clearly the right thing to do. Having a function that is named simply area, and returns a Checked<>, when we have almost no other functions like that in all of WebKit, is not an obvious pattern. I would suggest renaming it checkedArea instead.

I will rename it in a new patch, but I can see three reasons for not naming it differently:

1. You can't make a mistake because the compiler will tell you that you didn't call a method to get the value out of the Checked<> object.
2. I foresee a future where we have many more methods that return Checked<>, and so adding a "checked" prefix won't be that useful.
3. It forces the caller to think about overflow if they hit a compiler error with #1.  (This is the weakest argument because the "checked" prefix would do the same.)

>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:50
>> +    unsigned area = (rect.area() * 4).unsafeGet();
> 
> This is inelegant. Checked is doing overflow checking, and here we do the same overflow checking a second time so that we can detect that case and return nullptr. Is there a way to write this without having to do our own hand check that exactly matches what Checked is doing?

I think the way to do this is to templatize checkedArea() to handle both <unsigned, CrashOnOverflow> and Checked<unsigned, RecordOverflow> variants, then chose the variant at the call site.

I will do this in the next patch.

>> Source/WebKit2/WebProcess/Plugins/PluginProxy.cpp:613
>> +    }
> 
> Same comment as above. Can we find a way to use Checked to do this rather than writing code to do same check? Also, I think this code needs a comment. It’s not at all clear why this check is the correct one.

Ditto.

[Simon Fraser (smfr)]

(In reply to comment #8)
> Comment on attachment 292327 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292327&action=review
> 
> >> Source/WebCore/html/ImageData.cpp:116
> >> +    , m_data(Uint8ClampedArray::createUninitialized((size.area() * 4).unsafeGet()))
> > 
> > I wonder if it would be better to say "sizeof(RGBA32)" instead of "4" in all of these places.
> 
> I inferred from Simon's review of the first patch that these "4" values
> aren't all necessarily used for the same reason.  Is that not the case?

Right now that's true, but soon we'll be making ImageBuffers with different pixel sizes, so you'll have to ask the image buffer for its "bytes per pixel".

[Darin Adler]

Comment on attachment 292327 [details]
Patch v2

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

>>> Source/WebCore/html/MediaElementSession.cpp:681
>>> +    return elementRectInMainFrame.area().unsafeGet() > totalElementArea / 2;
>> 
>> Looking at all this code using Checked makes it clear that unsafeGet is the wrong name for that function. That function terminates the process if there was overflow. That’s not “unsafe” in the sense we normally mean it. In fact, we recently named a function safeCast and we gave it the same behavior as unsafeGet, abort the process if something is wrong.
> 
> I think the mismatch comes from the difference between Checked<unsigned, CrashOnOverflow> (which is the default for Checked<unsigned>) and Checked<unsigned, RecordOverflow>.
> 
> The Checked<unsigned>)::unsafeGet() method is poorly named since it's always safe (since we would have crashed before it was called if there was overflow), but Checked<unsigned, RecordOverflow>::unsafeGet() might actually crash if one didn't check the value of Checked<unsigned, RecordOverflow>::hasOverflowed() first.
> 
> Do you have a proposal for fixing this mismatch?

No, no proposal right now.

>>> Source/WebCore/platform/graphics/IntSize.h:134
>>> +    Checked<unsigned> area() const
>> 
>> Using checked arithmetic to compute the area is clearly the right thing to do. Having a function that is named simply area, and returns a Checked<>, when we have almost no other functions like that in all of WebKit, is not an obvious pattern. I would suggest renaming it checkedArea instead.
> 
> I will rename it in a new patch, but I can see three reasons for not naming it differently:
> 
> 1. You can't make a mistake because the compiler will tell you that you didn't call a method to get the value out of the Checked<> object.
> 2. I foresee a future where we have many more methods that return Checked<>, and so adding a "checked" prefix won't be that useful.
> 3. It forces the caller to think about overflow if they hit a compiler error with #1.  (This is the weakest argument because the "checked" prefix would do the same.)

The value of the name with "checked" in it is primarily for people reading code, not writing code. I agree that when writing code the compiler will help you understand it’s checked.

Your argument that we will return checked from lots more functions in the future is an interesting one. I still think there will be *many* functions that return values that are not checked since the values are not the result of a computation. I think we need to discuss how good a pattern that is. It’s good to return a checked value if the caller is highly likely to do additional calculations based on that value. Otherwise the use of Checked can be inside the function and need not affect the return type.

[David Kilzer (:ddkilzer)]

Comment on attachment 292327 [details]
Patch v2

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

>>>> Source/WebCore/platform/graphics/IntSize.h:134
>>>> +    Checked<unsigned> area() const
>>> 
>>> Using checked arithmetic to compute the area is clearly the right thing to do. Having a function that is named simply area, and returns a Checked<>, when we have almost no other functions like that in all of WebKit, is not an obvious pattern. I would suggest renaming it checkedArea instead.
>> 
>> I will rename it in a new patch, but I can see three reasons for not naming it differently:
>> 
>> 1. You can't make a mistake because the compiler will tell you that you didn't call a method to get the value out of the Checked<> object.
>> 2. I foresee a future where we have many more methods that return Checked<>, and so adding a "checked" prefix won't be that useful.
>> 3. It forces the caller to think about overflow if they hit a compiler error with #1.  (This is the weakest argument because the "checked" prefix would do the same.)
> 
> The value of the name with "checked" in it is primarily for people reading code, not writing code. I agree that when writing code the compiler will help you understand it’s checked.
> 
> Your argument that we will return checked from lots more functions in the future is an interesting one. I still think there will be *many* functions that return values that are not checked since the values are not the result of a computation. I think we need to discuss how good a pattern that is. It’s good to return a checked value if the caller is highly likely to do additional calculations based on that value. Otherwise the use of Checked can be inside the function and need not affect the return type.

Actually, what I wanted to write instead of #3 above (but forgot while typing this up waiting outside the new Firenza Pizza place in Los Gatos for lunch) is:

#4. I don't think we want to ever provide an IntSize::area() method that doesn't use checked arithmetic due to the possibility of overflow, so this will become the "default" method for getting the area of an IntSize, so why not call it just area()?

I understand the argument to make it easier for the reader, although it's also obvious where the object is used later that's it's Checked<> with methods like unsafeGet() being used.  (In a similar way, the use of 'auto' obscures the exact type of the returned value, but using clues later in the method usually make it obvious whether it's a RetainPtr<> or a Ref<> or a RefPtr<>, etc.)

Hmm...maybe change unsafeGet() into checkedGet() or getChecked() or getCheckedValue() or checkedValue()?  (I like the last suggestion the best.)

Then expressions like (size.area() * 4).checkedValue() would make it obvious you're dealing with Checked<> objects without having to rename area() to checkedArea().

[David Kilzer (:ddkilzer)]

Created attachment 292422 [details]
Patch v3

[Darin Adler]

Comment on attachment 292422 [details]
Patch v3

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

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:48
> +    Checked<unsigned, RecordOverflow> numBytes = rect.area<RecordOverflow>() * 4;

This should use auto, I think, rather than writing out the type.

> Source/WebKit2/Shared/ShareableBitmap.cpp:69
> +    Checked<unsigned, RecordOverflow> numBytes = numBytesForSize(size);

Ditto.

> Source/WebKit2/Shared/ShareableBitmap.cpp:82
> +    Checked<unsigned, RecordOverflow> numBytes = numBytesForSize(size);

Ditto.

> Source/WebKit2/Shared/ShareableBitmap.cpp:97
> +    Checked<unsigned, RecordOverflow> numBytes = numBytesForSize(size);

Ditto.

[David Kilzer (:ddkilzer)]

Committed r207708: <http://trac.webkit.org/changeset/207708>