1531025 - Possible division by zero in nsIFrame::ComputeBorderRadii()

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[(no longer active)]

https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/layout/generic/nsFrame.cpp#1709

The sum variable is tested against zero on line 1706, which implies it can be zero, so the division here can be divide by zero.

Comment 1

[Daniel Holbert [:dholbert]]

This probably isn't an issue in practice, but sure, we should probably guard the division with a check against zero.

I think it's not an issue in practice (except maybe in fuzzer testcases), because the potential divide-by-zero happens here:

     if (length < sum) ratio = std::min(ratio, double(length) / sum);

Looking at that code: if we get to the the division, then it means sum is strictly greater than length; and I'm pretty sure that length itself can be expected to be nonnegative. So, if we get to the division, it means sum is itself strictly larger than zero, since we've checked that it's strictly larger than another value that is at least zero.

(length is one of the dimensions of nsSize& aBorderArea, and I'm pretty sure it's not expected to be be negative (aside from cases where we've had integer overflow due to absurdly large numbers). It just doesn't make sense to have a negatively-sized border area.)

Having said that, I can imagine this causing problems if integer overflow gives us a length that is unexpectedly negative (in which case we could get to the division with a zero value for sum.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1531025 part 1: Add braces around conditions in nsIFrame::ComputeBorderRadii(). r=boris

This patch doesn't change any logic/behavior -- it's just adding braces for clarity.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1531025 part 2: Skip an unnecessary border-radius calculation if the radii are 0 (to avoid division by 0). r=boris

This patch avoids a potential division by 0 (one that's unlikely to be
triggered by real content), for correctness and robustness.

This patch isn't really changing the logic, because the newly-guarded code is
already guarded by a "length < sum" check, and "length" is expected to be
nonnegative [1], which means "sum" would already have been nonzero in cases
where the existing strict less-than comparison returned true.

[1] except for integer overflow or other bizarreness.

Depends on D21578

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4028e1ca6c7
part 1: Add braces around conditions in nsIFrame::ComputeBorderRadii(). r=boris
https://hg.mozilla.org/integration/autoland/rev/fd71937c6f25
part 2: Skip an unnecessary border-radius calculation if the radii are 0 (to avoid division by 0). r=boris

Comment 5

[Narcis Beleuzu [:NarcisB]]

Backed out for WR failures on outline-013.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/1003ed8243c5633f9a67a0e1323f6faf0abd067a
Push link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=fd71937c6f25db7365001a491e32f283a41397e7&selectedJob=231187758
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=231187758&repo=autoland&lineNumber=14038

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #3)

"length" is expected to be nonnegative

Apparently this was wrong! My expectation was based on frame sizes (which are nonnegative), but it seems that outline-offset: -150px; in this testcase does indeed create a negative length value of -6000.

So current mozilla-central proceeds to the ratio = std::min(ratio, double(length) / sum); expression, with length=-6000 and sum=0, and it produces -inf. Then we enter this block:

  if (ratio < 1.0) {
    NS_FOR_CSS_HALF_CORNERS(corner) { aRadii[corner] *= ratio; }
  }

...which ends up storing 0 * (double)-inf for every aRadii[corner] component, which we evaluate to -2147483648, which is INT_MIN.

In contrast: with my patch, we end up leaving the aRadii[corner] all at 0, and apparently that produces the wrong result (where INT_MIN produces the correct result).

Comment 7

[Daniel Holbert [:dholbert]]

Notably, Firefox is the only engine to "pass" that outline-013.html test:
https://wpt.fyi/results/css/css-ui/outline-013.html

The spec text in question is:

Negative values must cause the outline to shrink into the border box.
Both the height and the width of outside of the shape drawn by the
outline should not become smaller than twice the computed value of
the outline-width property, to make sure that an outline can be
rendered even with large negative values.
https://drafts.csswg.org/css-ui-3/#outline-props

In other words, the spec is saying that our drawn "outline" rectangle must be at least 100px tall (2 x 50px, the outline width), regardless of the outline-offset value.

We pass the test by accident right now, as a special case, for exactly the scenario that the testcase happens to exercise (outline-offset: -150px). However, if I reduce outline-offset a bit to 140px, then we fail the test (both strictly and per the letter of the spec -- we render an outline rect that is smaller than 100px).

So we're only "barely" passing the test as it is, and the tested behavior isn't implemented by any other engine anyway, so I don't think we should worry about it to much. I think we should proceed with the patches here (avoid dividing by 0 and awkwardly depending on the resulting bogus behavior), and just mark the test as failing.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (whose behavior changes if we stop dividing by zero here)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase 1 in Edge, Chrome, Firefox Nightly, & patched Firefox nightly

Here are screenshots of...

 Edge        Chrome
 FF nightly  Patched FF Nightly

In every case here where does not entirely surround the red (i.e. the entire 2nd row, and entire 3rd row in all browsers besides FF Nightly), the browser is technically not compliant with the spec text that I quoted above.

In patched Nightly, our behavior throughout the 2nd row (small green rect migrating slowly towards bottom right corner) just continues in the 3rd row (small green rect continues migrating outside the outlined element's border-box). That behavior is arguably a bit bogus, but we'll now be more consistent at least, and we won't be dividing by 0 and then multiplying the resulting infinite value by 0, which is good for well-defined-ness.

Comment 10

[Daniel Holbert [:dholbert]]

boris: are you OK with me carrying forward your r+, with the patch amended to indicate that the test from comment 5 is expected-fail?

Per comment 8 - 9, we're the only UA that passes the test right now, and we only barely pass it (e.g. if I reduce the magnitude of the test's outline-offset value by 1px, it already would fail in Nightly fail the test, though the spec says it should not).

Also note that the spec text here uses "should" terminology (should not become smaller), which is less binding than "must" terminology, per http://www.rfc-editor.org/rfc/rfc2119.txt

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd note that the interaction of negative outline-offset with rounded borders is somewhat widely known to produce (differently) broken behavior in all (?) browser engines.

Comment 12

[Daniel Holbert [:dholbert]]

Try run with updated patches (with the WPT test outline-013.html now annotated as expected-fail):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=781d220b740c29e51a27d5a7ac63886f925e0c6d

Comment 13

[Boris Chiou [:boris]]

(In reply to Daniel Holbert [:dholbert] from comment #10)

boris: are you OK with me carrying forward your r+, with the patch amended to indicate that the test from comment 5 is expected-fail?

Per comment 8 - 9, we're the only UA that passes the test right now, and we only barely pass it (e.g. if I reduce the magnitude of the test's outline-offset value by 1px, it already would fail in Nightly fail the test, though the spec says it should not).

Just read your comments and updates in the patch. It's ok to carry forward my r+. Thanks for updating this. :)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/818af1db79a2
part 1: Add braces around conditions in nsIFrame::ComputeBorderRadii(). r=boris
https://hg.mozilla.org/integration/autoland/rev/bfb349870bbf
part 2: Skip an unnecessary border-radius calculation if the radii are 0 (to avoid division by 0). r=boris

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/818af1db79a2
https://hg.mozilla.org/mozilla-central/rev/bfb349870bbf