Bug 209485

Summary:

Division by zero in RenderBlockFlow::computeColumnCountAndWidth

Product:

WebKit

Reporter:

Jack <shihchieh_lee>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, ews-feeder, product-security, rniwa, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Jack

Reported 2020-03-24 11:39:17 PDT

<rdar://60746109> #0 0x11f4297bd in WebCore::operator/(WebCore::LayoutUnit const&, WebCore::LayoutUnit const&) (Safari_ASAN_258820_08056b77ac1d7bd944690afb1f182d3dda421853.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x2a477bd) #1 0x121505658 in WebCore::RenderBlockFlow::computeColumnCountAndWidth() (Safari_ASAN_258820_08056b77ac1d7bd944690afb1f182d3dda421853.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x4b23658) #2 0x12150513b in WebCore::RenderBlockFlow::recomputeLogicalWidthAndColumnWidth() (Safari_ASAN_258820_08056b77ac1d7bd944690afb1f182d3dda421853.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x4b2313b) #3 0x121506349 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) (Safari_ASAN_258820_08056b77ac1d7bd944690afb1f182d3dda421853.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x4b24349) #4 0x1214ccc74 in WebCore::RenderBlock::layout() (Safari_ASAN_258820_08056b77ac1d7bd944690afb1f182d3dda421853.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x4aeac74)

Attachments
Patch (4.01 KB, patch) 2020-03-24 12:29 PDT, Jack	no flags	Details Formatted Diff Diff
Patch (3.54 KB, patch) 2020-03-24 14:13 PDT, Jack	no flags	Details Formatted Diff Diff
Patch (6.14 KB, patch) 2020-03-27 12:16 PDT, Jack	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Jack

Comment 1 2020-03-24 11:51:59 PDT

Root cause: In function computeColumnCountAndWidth, the computed colGap is -1 px, and colWidth is 1px. And next (colWidth + colGap) is used as denominator to get desiredColumnCount, resulting in division by zero. All the pixel settings seem normal, but function contentWidth() gives a negative value. contentWidth = paddingBoxWidth - paddingLeft - paddingRight where paddingBoxWidth=0, paddingLeft=0, paddingRight=64 And paddingBoxWidth = std::max(0_lu, width() - borderLeft() - borderRight() - verticalScrollbarWidth()); where width=192, borderLeft=64, borderRight=64, verticalScrollbarWidth=15

Jack

Comment 2 2020-03-24 11:52:41 PDT

Division by zero so should not have security implications.

Jack

Comment 3 2020-03-24 12:29:03 PDT

Created attachment 394395 [details] Patch

Jack

Comment 4 2020-03-24 14:13:58 PDT

Created attachment 394408 [details] Patch

Jack

Comment 5 2020-03-24 14:20:35 PDT

Limit the minimum to 0 in function contentWidth() and contentHeight() breaks expected result of some tests. (In reply to Jack from comment #3) > Created attachment 394395 [details] > Patch

Jack

Comment 6 2020-03-24 14:24:42 PDT

Limit the minimum of column gap instead. However, this patch would still show error message below when the grid-gap is set to 50%. There is no error when grid-gap is 100%. ERROR: !(m_value >= 0) /Users/jacklee/browser2/OpenSource/Source/WebCore/platform/LayoutUnit.h(114) : unsigned int WebCore::LayoutUnit::toUnsigned() const <style> #TEXTAREA { grid-gap: 50%; -webkit-logical-width: 0px; } </style> <script> window.onload = () => { TEXTAREA.style.setProperty("column-width", "1px"); TEXTAREA.style.setProperty("padding", "0px 1px 0px 0px"); } </script> <body><textarea id=TEXTAREA><span></span></textarea> (In reply to Jack from comment #4) > Created attachment 394408 [details] > Patch

alan

Comment 7 2020-03-26 09:35:14 PDT

the first patch looks correct, you should check the nature of the different geometry and why we end up producing it (it might just be correct).

Jack

Comment 8 2020-03-26 10:48:48 PDT

Alan, thanks! But a lot of tests fail in first patch. I guess the content width/height were negative in those tests. (In reply to zalan from comment #7) > the first patch looks correct, you should check the nature of the different > geometry and why we end up producing it (it might just be correct).

alan

Comment 9 2020-03-27 09:35:17 PDT

(In reply to Jack from comment #8) > Alan, thanks! But a lot of tests fail in first patch. I guess the content > width/height were negative in those tests. I see only one failure. https://ews-build.webkit.org/results/macOS-Mojave-Release-WK1-Tests-EWS/r394395-5787/results.html Am I missing something here?

Jack

Comment 10 2020-03-27 10:50:36 PDT

Thanks for checking out the patch. There is one crash and one failed case in Mac-wk1, two failures in Mac-wk2 and one failure in ios-wk2 and mac-debug-wk1 when I obsolete the change. Most of them show different pixel sizes (almost for every element), so I think the test cases probably expect negative content width/height. :-( layer at (0,0) size 800x251 RenderBlock {HTML} at (0,0) size 800x251 RenderBody {BODY} at (8,8) size 784x235 layer at (0,0) size 800x249 RenderBlock {HTML} at (0,0) size 800x249 RenderBody {BODY} at (8,8) size 784x233 (In reply to zalan from comment #9) > (In reply to Jack from comment #8) > > Alan, thanks! But a lot of tests fail in first patch. I guess the content > > width/height were negative in those tests. > I see only one failure. > https://ews-build.webkit.org/results/macOS-Mojave-Release-WK1-Tests-EWS/ > r394395-5787/results.html > Am I missing something here?

alan

Comment 11 2020-03-27 10:56:19 PDT

(In reply to Jack from comment #10) > Thanks for checking out the patch. There is one crash and one failed case in > Mac-wk1, two failures in Mac-wk2 and one failure in ios-wk2 and > mac-debug-wk1 when I obsolete the change. I am so confused. Are we looking at the same test output? e.g. I don't see a crash at all. Could you include a link to these failures?

alan

Comment 12 2020-03-27 11:03:47 PDT

(In reply to zalan from comment #11) > (In reply to Jack from comment #10) > > Thanks for checking out the patch. There is one crash and one failed case in > > Mac-wk1, two failures in Mac-wk2 and one failure in ios-wk2 and > > mac-debug-wk1 when I obsolete the change. > I am so confused. Are we looking at the same test output? e.g. I don't see a > crash at all. Could you include a link to these failures? I guess you meant "open-after-abort.htm". that must be unrelated. please check locally.

Jack

Comment 13 2020-03-27 11:16:38 PDT

I see. Thanks! Other links are attached below. There are five cases failing, but button.html fail on three MAC platforms. Maybe the width/height is negative only on MAC? And I am not sure if the other two are related to the code change. I will run the test to verify. css3/flexbox/button.html fast/hidpi/image-srcset-relative-svg-canvas-2x.html imported/w3c/web-platform-tests/content-security-policy/reporting/report-uri-from-child-frame.html https://ews-build.webkit.org/results/iOS-13-Simulator-WK2-Tests-EWS/r394395-13750/results.html https://ews-build.webkit.org/results/macOS-Mojave-Release-WK1-Tests-EWS/r394395-5787/results.html https://ews-build.webkit.org/results/macOS-Mojave-Release-WK2-Tests-EWS/r394395-5865/results.html https://ews-build.webkit.org/results/macOS-Mojave-Debug-WK1-Tests-EWS/r394395-5855/results.html (In reply to zalan from comment #12) > I guess you meant "open-after-abort.htm". that must be unrelated. please > check locally.

alan

Comment 14 2020-03-27 11:23:15 PDT

(In reply to Jack from comment #13) > I see. Thanks! Other links are attached below. There are five cases failing, > but button.html fail on three MAC platforms. Maybe the width/height is > negative only on MAC? And I am not sure if the other two are related to the > code change. I will run the test to verify. > > fast/hidpi/image-srcset-relative-svg-canvas-2x.html This is most likely unrelated. If you look at the diff, you can see that this is a circle drawing mismatch. > imported/w3c/web-platform-tests/content-security-policy/reporting/report-uri- > from-child-frame.html There's no diff to it's hard to tell but I'd be very surprised if it was caused by this change. > css3/flexbox/button.html I think this is the only failure caused by this change.

Jack

Comment 15 2020-03-27 11:25:41 PDT

Thanks! Got it. I will submit the first patch and rerun the test. Hopefully the other two will disappear this time. But are they flaky cases? Since they don't fail in the second patch somehow. (In reply to zalan from comment #14) > (In reply to Jack from comment #13) > > I see. Thanks! Other links are attached below. There are five cases failing, > > but button.html fail on three MAC platforms. Maybe the width/height is > > negative only on MAC? And I am not sure if the other two are related to the > > code change. I will run the test to verify. > > > > fast/hidpi/image-srcset-relative-svg-canvas-2x.html > This is most likely unrelated. If you look at the diff, you can see that > this is a circle drawing mismatch. > > > > imported/w3c/web-platform-tests/content-security-policy/reporting/report-uri- > > from-child-frame.html > There's no diff to it's hard to tell but I'd be very surprised if it was > caused by this change. > > > css3/flexbox/button.html > I think this is the only failure caused by this change.

Jack

Comment 16 2020-03-27 12:16:38 PDT

Created attachment 394745 [details] Patch

Jack

Comment 17 2020-03-27 12:19:54 PDT

Thanks Alan for discussion. We are going to limit minimal value of content width and height to 0, since there is no concept of negative content size. Run with modified expected output for button.html (only for MAC platform). (In reply to Jack from comment #16) > Created attachment 394745 [details] > Patch

EWS

Comment 18 2020-03-30 10:43:28 PDT

Committed r259210: <https://trac.webkit.org/changeset/259210> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394745 [details].

Ryosuke Niwa

Comment 19 2020-03-30 11:24:39 PDT

There is no security implication here.

Note You need to log in before you can comment on or make changes to this bug.