Possible division by zero in nsIFrame::ComputeBorderRadii()

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: Ehsan, Assigned: dholbert)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(4 attachments)

Reporter

Description

4 months ago

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.

Assignee

Comment 1

4 months ago

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.)

Assignee

Comment 2

4 months ago

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

Assignee

Comment 3

4 months ago

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

Assignee

Updated

4 months ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Priority: -- → P3
Attachment #9047480 - Attachment description: Bug 1531025 part 2: Skip an unnecessary border-radius calculation if the radii are 0 (to avoid division by 0). r?TYLin → Bug 1531025 part 2: Skip an unnecessary border-radius calculation if the radii are 0 (to avoid division by 0).
Attachment #9047479 - Attachment description: Bug 1531025 part 1: Add braces around conditions in nsIFrame::ComputeBorderRadii(). r?TYLin → Bug 1531025 part 1: Add braces around conditions in nsIFrame::ComputeBorderRadii().

Comment 4

4 months ago
Pushed by dholbert@mozilla.com:
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
Assignee

Comment 6

4 months ago

(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).

Flags: needinfo?(dholbert)
Assignee

Comment 7

3 months ago

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.

Assignee

Updated

3 months ago
Attachment #9048241 - Attachment mime type: text/plain → text/html
Assignee

Comment 9

3 months ago

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.

Attachment #9047479 - Attachment description: Bug 1531025 part 1: Add braces around conditions in nsIFrame::ComputeBorderRadii(). → Bug 1531025 part 1: Add braces around conditions in nsIFrame::ComputeBorderRadii(). r=boris
Attachment #9047480 - Attachment description: Bug 1531025 part 2: Skip an unnecessary border-radius calculation if the radii are 0 (to avoid division by 0). → Bug 1531025 part 2: Skip an unnecessary border-radius calculation if the radii are 0 (to avoid division by 0). r=boris
Assignee

Comment 10

3 months ago

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

Assignee

Updated

3 months ago
Flags: needinfo?(boris.chiou)

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.

Assignee

Comment 12

3 months ago

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

(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. :)

Flags: needinfo?(boris.chiou)

Comment 14

3 months ago
Pushed by dholbert@mozilla.com:
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

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.