Closed Bug 1419250 Opened 2 years ago Closed 2 years ago

UBSan: division by zero in [@ nsSVGLength2::GetUnitScaleFactor]

Categories

(Core :: SVG, defect, P3)

59 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: tsmith, Assigned: longsonr)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-undefined, testcase)

Attachments

(2 files, 2 obsolete files)

Attached file testcase.html
This was found with a Firefox build built with -fsanitize=float-divide-by-zero,integer-divide-by-zero

/dom/svg/nsSVGLength2.cpp:257:14: runtime error: division by zero
    #0 0x7f2adb6d118c in nsSVGLength2::GetUnitScaleFactor(mozilla::dom::UserSpaceMetrics const&, unsigned char) const /dom/svg/nsSVGLength2.cpp
    #1 0x7f2adb6d12ae in nsSVGLength2::GetUnitScaleFactor(mozilla::dom::SVGViewportElement*, unsigned char) const /dom/svg/nsSVGLength2.cpp:224:10
    #2 0x7f2adb644417 in nsSVGLength2::GetAnimValue(mozilla::dom::SVGViewportElement*) const /dom/svg/nsSVGLength2.h:132:25
    #3 0x7f2adccde81c in nsSVGOuterSVGFrame::GetIntrinsicSize() /layout/svg/nsSVGOuterSVGFrame.cpp:228:61
    #4 0x7f2adccdee35 in nsSVGOuterSVGFrame::ComputeSize(gfxContext*, mozilla::WritingMode, mozilla::LogicalSize const&, int, mozilla::LogicalSize const&, mozilla::LogicalSize const&, mozilla::LogicalSize const&, nsIFrame::ComputeSizeFlags) /layout/svg/nsSVGOuterSVGFrame.cpp:309:33
    #5 0x7f2adc8f03ee in mozilla::ReflowInput::InitConstraints(nsPresContext*, mozilla::LogicalSize const&, nsMargin const*, nsMargin const*, mozilla::LayoutFrameType) /layout/generic/ReflowInput.cpp:2538:17
    #6 0x7f2adc8e83b5 in mozilla::ReflowInput::Init(nsPresContext*, mozilla::LogicalSize const*, nsMargin const*, nsMargin const*) /layout/generic/ReflowInput.cpp:426:3
    #7 0x7f2adc8eb0d3 in mozilla::ReflowInput::ReflowInput(nsPresContext*, mozilla::ReflowInput const&, nsIFrame*, mozilla::LogicalSize const&, mozilla::LogicalSize const*, unsigned int) /layout/generic/ReflowInput.cpp:258:5
    #8 0x7f2adcaec060 in void mozilla::Maybe<mozilla::ReflowInput>::emplace<nsPresContext*&, mozilla::ReflowInput const&, nsIFrame*&, mozilla::LogicalSize&>(nsPresContext*&, mozilla::ReflowInput const&, nsIFrame*&, mozilla::LogicalSize&) /objdir-ff-ubsan/dist/include/mozilla/Maybe.h:459:34
    #9 0x7f2adcaa6b40 in nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) /layout/generic/nsLineLayout.cpp:862:23
    #10 0x7f2adc94f00b in nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /layout/generic/nsBlockFrame.cpp:4173:15
    #11 0x7f2adc94ddb8 in nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /layout/generic/nsBlockFrame.cpp:3969:5
    #12 0x7f2adc947f12 in nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /layout/generic/nsBlockFrame.cpp:3843:9
    #13 0x7f2adc94341f in nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /layout/generic/nsBlockFrame.cpp:2827:5
    #14 0x7f2adc93bd46 in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /layout/generic/nsBlockFrame.cpp:2363:7
    #15 0x7f2adc936b99 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /layout/generic/nsBlockFrame.cpp:1236:3
    #16 0x7f2adc94c097 in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, mozilla::ReflowInput&, nsReflowStatus&, mozilla::BlockReflowInput&) /layout/generic/nsBlockReflowContext.cpp:306:11
    #17 0x7f2adc94685b in nsBlockFrame::ReflowBlockFrame(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /layout/generic/nsBlockFrame.cpp:3474:11
    #18 0x7f2adc9433d3 in nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /layout/generic/nsBlockFrame.cpp:2824:5
    #19 0x7f2adc93bd46 in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /layout/generic/nsBlockFrame.cpp:2363:7
    #20 0x7f2adc936b99 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /layout/generic/nsBlockFrame.cpp:1236:3
    #21 0x7f2adc97224e in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /layout/generic/nsContainerFrame.cpp:934:14
    #22 0x7f2adc971654 in nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /layout/generic/nsCanvasFrame.cpp:757:5
    #23 0x7f2adc97224e in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /layout/generic/nsContainerFrame.cpp:934:14
    #24 0x7f2adca39dbd in nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput*, bool, bool, mozilla::ReflowOutput*, bool) /layout/generic/nsGfxScrollFrame.cpp:552:3
    #25 0x7f2adca3b630 in nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput*, mozilla::ReflowOutput const&) /layout/generic/nsGfxScrollFrame.cpp:664:3
    #26 0x7f2adca3e3f7 in nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /layout/generic/nsGfxScrollFrame.cpp:1041:3
    #27 0x7f2adc928cc6 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /layout/generic/nsContainerFrame.cpp:978:14
    #28 0x7f2adc92840d in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /layout/generic/ViewportFrame.cpp:336:7
    #29 0x7f2adc73e3bc in mozilla::PresShell::DoReflow(nsIFrame*, bool) /layout/base/PresShell.cpp:9028:11
    #30 0x7f2adc74d67a in mozilla::PresShell::ProcessReflowCommands(bool) /layout/base/PresShell.cpp:9201:24
    #31 0x7f2adc74c9ba in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /layout/base/PresShell.cpp:4274:11
    #32 0x7f2adc6d9a72 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /layout/base/nsRefreshDriver.cpp:1901:16
    #33 0x7f2adc6e5003 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /layout/base/nsRefreshDriver.cpp:306:7
    #34 0x7f2adc6e4d3a in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /layout/base/nsRefreshDriver.cpp:327:5
    #35 0x7f2adc6e92ca in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) /layout/base/nsRefreshDriver.cpp:769:5
    #36 0x7f2adc6e7d20 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /layout/base/nsRefreshDriver.cpp:682:35
    #37 0x7f2adc6e31dc in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() /layout/base/nsRefreshDriver.cpp:528:20
    #38 0x7f2ad515cdb9 in nsThread::ProcessNextEvent(bool, bool*) /xpcom/threads/nsThread.cpp:1037:14
    #39 0x7f2ad5195ed1 in NS_ProcessNextEvent(nsIThread*, bool) /xpcom/threads/nsThreadUtils.cpp:513:10
    #40 0x7f2ad62c7e31 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /ipc/glue/MessagePump.cpp:97:21
    #41 0x7f2ad6149d50 in MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:299:3
    #42 0x7f2adc0d70a4 in nsBaseAppShell::Run() /widget/nsBaseAppShell.cpp:159:27
    #43 0x7f2ae0a268d9 in nsAppStartup::Run() /toolkit/components/startup/nsAppStartup.cpp:288:30
    #44 0x7f2ae0bedafb in XREMain::XRE_mainRun() /toolkit/xre/nsAppRunner.cpp:4685:22
    #45 0x7f2ae0bef95c in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /toolkit/xre/nsAppRunner.cpp:4847:8
    #46 0x7f2ae0bf0651 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /toolkit/xre/nsAppRunner.cpp:4942:21
    #47 0x518238 in do_main(int, char**, char**) /browser/app/nsBrowserApp.cpp:231:22
    #48 0x517aba in main /browser/app/nsBrowserApp.cpp:304:16
    #49 0x7f2b0a1021c0 in __libc_start_main /build/glibc-CxtIbX/glibc-2.26/csu/../csu/libc-start.c:308
    #50 0x420589 in _start (firefox+0x420589)
Flags: in-testsuite?
Attached patch WIP (obsolete) — Splinter Review
Assignee: nobody → longsonr
Is there any way to indicate to the tool that a divide by zero is OK because we're going to immediately call IsFinite and deal with it?
Flags: needinfo?(twsmith)
(In reply to Robert Longson [:longsonr] from comment #2)
> Is there any way to indicate to the tool that a divide by zero is OK because
> we're going to immediately call IsFinite and deal with it?

Short answer is we can ignore it but that doesn't solve the problem.

When dealing with undefined behavior we need to take a step back and remember the code we are writing may not be what is generated by the compiler. The compiler is free to make assumptions about the code (in this case divide by zero can't happen) to create optimizations. So that being the case we can't make assumptions about the results at run time because the compiler may optimize away any checks after the UB because it is free to do so, or something else unexpected. It may work or seem to work but any change in compilers, compiler version, optimization level, target architecture, etc... and all bets are off.
Flags: needinfo?(twsmith)
Attached patch patch (obsolete) — Splinter Review
The idea is to avoid generating infinities in GetUnitScaleFactor by changing it to a method that returns the multiplicative inverse of that (which I've called GetPixelsPerUnit). So the new function returns 0 where we would previously have generated INF.

We then need to replace all the multiplications with divisions and vice versa and finally take care with the new divisions as we might produce infinities. Note that there are far fewer divisions now than there were before.
Attachment #8931138 - Attachment is obsolete: true
Attachment #8931419 - Flags: review?(dholbert)
Comment on attachment 8931419 [details] [diff] [review]
patch

Review of attachment 8931419 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay - I was largely AFK on vacation for a little while. Back now!

Marking r- for now, since I think the divide-by-zero issue is still here (as noted in "(1)" below).

::: dom/svg/nsSVGLength2.cpp
@@ +236,3 @@
>  }
>  
> +static const float DPI = 96.0f;

This might need a clearer name (or brief explanatory comment), perhaps?

Right now, its name ("DPI") makes me wonder whether it should really be querying some nsDeviceContext.cpp DPI function or something like that. (And really, maybe it should? But I think any scaling from that gets handled elsewhere, so it can be named something that's clearer about why it's appropriate for this to be a constant...)

@@ +305,5 @@
>  
> +  float valueInUserUnits =
> +    mBaseVal * GetPixelsPerUnit(aSVGElement, mSpecifiedUnitType);
> +  float valueInSpecifiedUnits = valueInUserUnits /
> +          GetPixelsPerUnit(aSVGElement, unitType);

Two issues here:
(1) This line still has the core problem of this bug, I think, right?  It's just pushed up one level of abstraction -- but we'll still potentially be dividing by 0 to produce some quantity (if GetPixelsPerUnit returns 0 here and we divide by that). And then we're testing whether that quantity is infinite (on the next line), but per comment 3, we can't rely on compilers producing the correct answer for that sequence of steps.

Suggestion: I think we probably need to store GetPixelsPerUnit(aSVGElement, unitType) in a local variable & return early if that variable is 0, *before* we use it as a divisor.  We might still *also* need your patch's IsFinite() check as well (if we divide two extreme nonzero finite values and that produces something infinite), though I'm not sure how much of a concern that is in practice or if there'd be any undefined behavior around that (once we've removed the divide-by-zero possibility).


(2) Stylistic nit: This line is overindented, per Mozilla coding style - it should only be indented 2 spaces more than the previous line (vs. right now, it's indented 8 spaces more).  Though maybe after you address (1), there won't be a linebreak anymore and this nit will be obsolete.

(The style you're using here, "indent 2 spaces beyond the start of some arbitrary token on previous line", is something that I think we only use when the "correct" indentation-style would push us over 80 characters and there's no good way to avoid it, so we deindent to some compromise point that feels sensible.  That might make sense here, if we were linebreaking inside of a parenthesized expression, because the parenthesis would impose a new indentation point -- but an arithmetic operator like "/" does not have that same effect.)

@@ +432,5 @@
>  nsSVGLength2::SetBaseValue(float aValue, nsSVGElement *aSVGElement,
>                             bool aDoSetAttr)
>  {
> +  float valueInSpecifiedUnits = aValue /
> +          GetPixelsPerUnit(aSVGElement, mSpecifiedUnitType);

Nit: as above, this can still be a divide-by-zero, and this line is overindented. Please address both of those issues here as well.

::: dom/svg/nsSVGLength2.h
@@ +168,5 @@
>    bool mIsAnimated:1;
>    bool mIsBaseSet:1;
>  
> +  float GetPixelsPerUnit(nsIFrame* aFrame, uint8_t aUnitType) const;
> +  float GetPixelsPerUnit(const UserSpaceMetrics& aMetrics, uint8_t aUnitType) const;

I think "Pixels" here are "user unit" pixels, right?

Since the comment below this (for SetBaseValue/SetAnimValue) mention that term ("user units"), could you add a comment here that mentions it as well, to make it clear that this is dealing with the same unit-space? Maybe something like this:
  // These APIs returns the number of user-unit pixels per unit of the
  // given type, in a given context (frame/element/etc).

@@ +175,4 @@
>  
>    // SetBaseValue and SetAnimValue set the value in user units
> +  nsresult SetBaseValue(float aValue, nsSVGElement *aSVGElement,
> +                        bool aDoSetAttr);

Please update the documentation before this block of 4 functions, so that it mentions why two of the functions (and only those two) are now fallible, vs. why the other two aren't.

(i.e. What sorts of conditions trigger failure & why aren't those possible in the Set***InSpecifiedUnits() functions)
Attachment #8931419 - Flags: review?(dholbert) → review-
Priority: -- → P3
Attachment #8931419 - Attachment is obsolete: true
Attachment #8936469 - Flags: review?(dholbert)
Comment on attachment 8936469 [details] [diff] [review]
address review comments

Review of attachment 8936469 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks -- this looks great!

Looks like this needs a commit message -- r=me assuming you add a sane one that explains the change. :)
Attachment #8936469 - Flags: review?(dholbert) → review+
Also: probably worth adding the attached testcase as a crashtest.

I see that you're tweaking a manifest for SVGLength-px-with-context.html [1], but it doesn't seem like the crashtest would be redundant on top of that web-platform-test, because I don't see any zero font-sizes in that web-platform-test.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/svg/types/scripted/SVGLength-px-with-context.html
I think there's some kind of rounding error that SVGLength-px-with-context.html exposes that's gone with the patch as it does fewer floating point calculations.
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/522e9e2a6c82
check for division by zero in length conversions and fail the conversion in such cases r=dholbert
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/522e9e2a6c82
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.