Bug 223697 – Source/WebCore/page/FrameView.h:990:50: runtime error: signed integer overflow: 65537 * 65537 cannot be represented in type 'int'

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Chris Dumez]

Fix bug found by UBSan:
- Source/WebCore/page/FrameView.h:990:50: runtime error: signed integer overflow: 65537 * 65537 cannot be represented in type 'int'
- Source/WebCore/page/FrameView.h:990:50: runtime error: signed integer overflow: 65536 * 65536 cannot be represented in type 'int'
- Source/WebCore/page/FrameView.h:990:50: runtime error: signed integer overflow: 1116300 * 558150 cannot be represented in type 'int'
- Source/WebCore/page/FrameView.h:990:50: runtime error: signed integer overflow: -33554432 * -33554432 cannot be represented in type 'int'

[Chris Dumez]

Created attachment 424150 [details]
Patch

[Darin Adler]

Comment on attachment 424150 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        - Source/WebCore/page/FrameView.h:990:50: runtime error: signed integer overflow: -33554432 * -33554432 cannot be represented in type 'int'

Surprised that we are computing area of sizes that have negative width or height.

> Source/WebCore/page/FrameView.h:994
> +    if (UNLIKELY(area.hasOverflowed()))

Makes me wish Checked had a "saturation" mode so we didn’t have to write such extensive code.

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 424150 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424150&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        - Source/WebCore/page/FrameView.h:990:50: runtime error: signed integer overflow: -33554432 * -33554432 cannot be represented in type 'int'
> 
> Surprised that we are computing area of sizes that have negative width or
> height.

I suspect the values may already overflowed before this function call.

> 
> > Source/WebCore/page/FrameView.h:994
> > +    if (UNLIKELY(area.hasOverflowed()))
> 
> Makes me wish Checked had a "saturation" mode so we didn’t have to write
> such extensive code.

[zalan]

no test case? I am curios how we end up with negative size here.

[Darin Adler]

I believe the test case is "our entire regression test suite run when WebKit is compiled with UBSan".

[Chris Dumez]

(In reply to zalan from comment #4)
> no test case? I am curios how we end up with negative size here.

The UBSan warnings are triggered by our test suite so the values showed in the errors should already be covered by our test suite.

[Chris Dumez]

(In reply to zalan from comment #4)
> no test case? I am curios how we end up with negative size here.

I believe you should be able to add assertions then run the test suite and hopefully find out :)

[zalan]

(In reply to Chris Dumez from comment #6)
> (In reply to zalan from comment #4)
> > no test case? I am curios how we end up with negative size here.
> 
> The UBSan warnings are triggered by our test suite so the values showed in
> the errors should already be covered by our test suite.
Can we figure out what test triggered this? This may have correctness implications as well.

[zalan]

(In reply to Chris Dumez from comment #7)
> (In reply to zalan from comment #4)
> > no test case? I am curios how we end up with negative size here.
> 
> I believe you should be able to add assertions then run the test suite and
> hopefully find out :)
ok

[EWS]

Committed r274958: <https://commits.webkit.org/r274958>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424150 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75799187>