1352453 - Crash [@ComputeBorderSpacedRepeatSize] Divide-by-Zero

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P3)

Description

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Testcase found while fuzzing mozilla-central rev 20170330-8df9fabf2587.

[11877] ###!!! ABORT: Divide by zero: file /home/worker/workspace/build/src/toolkit/xre/nsSigHandlers.cpp, line 156
==11877==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffed0dc4000; bottom 0x7f848fc6f000; size: 0x007a41155000 (525077925888)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
[11877] ###!!! ABORT: Divide by zero: file /home/worker/workspace/build/src/toolkit/xre/nsSigHandlers.cpp, line 156
ASAN:DEADLYSIGNAL
=================================================================
==11877==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004ecb07 bp 0x7f848fc70490 sp 0x7f848fc70480 T0)
==11877==The signal is caused by a WRITE memory access.
==11877==Hint: address points to the zero page.
    #0 0x4ecb06 in mozalloc_abort(char const*) /home/worker/workspace/build/src/memory/mozalloc/mozalloc_abort.cpp:33:5
    #1 0x7f847349def5 in Abort(char const*) /home/worker/workspace/build/src/xpcom/base/nsDebugImpl.cpp:441:3
    #2 0x7f847349dce0 in NS_DebugBreak /home/worker/workspace/build/src/xpcom/base/nsDebugImpl.cpp:397:7
    #3 0x7f847cac5c0a in fpehandler(int, siginfo*, void*) /home/worker/workspace/build/src/toolkit/xre/nsSigHandlers.cpp:156:5
    #4 0x7f848f84f3df  (/lib/x86_64-linux-gnu/libpthread.so.0+0x113df)
    #5 0x7f847a56229a in ComputeBorderSpacedRepeatSize /home/worker/workspace/build/src/layout/painting/nsCSSRendering.cpp:3034:35
    #6 0x7f847a56229a in ComputeTile(nsRect&, unsigned char, unsigned char, nsSize const&, nsSize&) /home/worker/workspace/build/src/layout/painting/nsImageRenderer.cpp:729
    #7 0x7f847a4e1ee9 in mozilla::nsImageRenderer::DrawBorderImageComponent(nsPresContext*, nsRenderingContext&, nsRect const&, nsRect const&, mozilla::gfx::IntRectTyped<mozilla::CSSPixel> const&, unsigned char, unsigned char, nsSize const&, unsigned char, mozilla::Maybe<nsSize> const&, bool) /home/worker/workspace/build/src/layout/painting/nsImageRenderer.cpp:890:21
    #8 0x7f847a49656a in nsCSSBorderImageRenderer::DrawBorderImage(nsPresContext*, nsRenderingContext&, nsIFrame*, nsRect const&) /home/worker/workspace/build/src/layout/painting/nsCSSRenderingBorders.cpp:3781:24
    #9 0x7f847a4927e5 in nsCSSRendering::PaintBorderWithStyleBorder(nsPresContext*, nsRenderingContext&, nsIFrame*, nsRect const&, nsRect const&, nsStyleBorder const&, nsStyleContext*, mozilla::PaintBorderFlags, mozilla::Sides) /home/worker/workspace/build/src/layout/painting/nsCSSRendering.cpp:839:22
    #10 0x7f847a49226e in nsCSSRendering::PaintBorder(nsPresContext*, nsRenderingContext&, nsIFrame*, nsRect const&, nsRect const&, nsStyleContext*, mozilla::PaintBorderFlags, mozilla::Sides) /home/worker/workspace/build/src/layout/painting/nsCSSRendering.cpp:647:12
    #11 0x7f847a52c847 in nsDisplayBorder::Paint(nsDisplayListBuilder*, nsRenderingContext*) /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:4784:5
    #12 0x7f847a48cbd8 in mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem>&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, gfxContext*, nsRenderingContext*, nsDisplayListBuilder*, nsPresContext*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, float, float, int) /home/worker/workspace/build/src/layout/painting/FrameLayerBuilder.cpp:6077:21
    #13 0x7f847a48fbd8 in mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*) /home/worker/workspace/build/src/layout/painting/FrameLayerBuilder.cpp:6252:19
    #14 0x7f84754dcd79 in mozilla::layers::ClientPaintedLayer::PaintThebes(nsTArray<mozilla::layers::ReadbackProcessor::Update>*) /home/worker/workspace/build/src/gfx/layers/client/ClientPaintedLayer.cpp:86:5
    #15 0x7f84754dd9a1 in mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) /home/worker/workspace/build/src/gfx/layers/client/ClientPaintedLayer.cpp:140:3
    #16 0x7f847550d43f in mozilla::layers::ClientContainerLayer::RenderLayer() /home/worker/workspace/build/src/gfx/layers/client/ClientContainerLayer.h:57:29
    #17 0x7f847550d43f in mozilla::layers::ClientContainerLayer::RenderLayer() /home/worker/workspace/build/src/gfx/layers/client/ClientContainerLayer.h:57:29
    #18 0x7f84754d825c in mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) /home/worker/workspace/build/src/gfx/layers/client/ClientLayerManager.cpp:358:13
    #19 0x7f84754d8a57 in mozilla::layers::ClientLayerManager::EndTransaction(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) /home/worker/workspace/build/src/gfx/layers/client/ClientLayerManager.cpp:411:3
    #20 0x7f847a5051b7 in nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:2253:17

Comment 1

[Ryan VanderMeulen (PTO, back 6-April)]

Reproduces with or without Stylo enabled.

INFO: Last good revision: 7c669d5d63efceb12696cd65cfa72c296013dafb (2016-07-25)
INFO: First bad revision: ceb63dec9267e9bb62f5e5e1f4c9d32d3ac1fbac (2016-07-26)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7c669d5d63efceb12696cd65cfa72c296013dafb&tochange=ceb63dec9267e9bb62f5e5e1f4c9d32d3ac1fbac

Crashing in code added by bug 720531.

Comment 2

[Ethan Lin[:ethlin]]

Attachment: Bug 1352453 - Check image dimension when computing border space size.

Review commit: https://reviewboard.mozilla.org/r/182920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182920/

Comment 3

[Ethan Lin[:ethlin]]

Attachment: Bug 1352453 - Add crashtest for this bug.

Review commit: https://reviewboard.mozilla.org/r/182922/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182922/

Comment 4

[Cameron McCormack (:heycam)]

Just to confirm, the bug is that we resolve the 1% border-image-width value against the size of the element, which is 0x0 because it has no content?  Do we hit the assertion with an explicit 0px image size here, or do we check for that somewhere else?

I see that we will correctly handle a zero aRepeatSize value down in nsLayoutUtils::DrawBackgroundImage -- it'll think we don't need any repeating, and call DrawImageInternal just once.  That then will return early after ComputeSnappedImageDrawingParameters determines there is nothing to paint.

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8911450 [details]
Bug 1352453 - Check image dimension when computing border space size.

https://reviewboard.mozilla.org/r/182920/#review188130

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8911451 [details]
Bug 1352453 - Add crashtest for this bug.

https://reviewboard.mozilla.org/r/182922/#review188132

Comment 7

[Astley Chen (inactive)]

Please uplift the fix to FF57 after code lands into m-c. Thanks.

Comment 8

[Ethan Lin[:ethlin]]

(In reply to Cameron McCormack (:heycam) from comment #4)
> Just to confirm, the bug is that we resolve the 1% border-image-width value
> against the size of the element, which is 0x0 because it has no content?  Do
> we hit the assertion with an explicit 0px image size here, or do we check
> for that somewhere else?

The size is zero so we get the error of "divide by zero". It looks like we don't check the image size elsewhere until the painting function(e.g., nsImageRenderer::Draw or nsLayoutUtils::DrawBackgroundImage).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6974ff079df1
Check image dimension when computing border space size. r=heycam
https://hg.mozilla.org/integration/autoland/rev/00076807f73d
Add crashtest for this bug. r=heycam

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6974ff079df1
https://hg.mozilla.org/mozilla-central/rev/00076807f73d

Comment 11

[Ryan VanderMeulen (PTO, back 6-April)]

Please request Beta approval on this when you get a chance.

Comment 12

[Ethan Lin[:ethlin]]

Comment on attachment 8911450 [details]
Bug 1352453 - Check image dimension when computing border space size.

Approval Request Comment
[Feature/Bug causing the regression]: bug 720531
[User impact if declined]: certain css style may cause crash 
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: Because I just add some error handling for zero value.
[String changes made/needed]: none

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8911450 [details]
Bug 1352453 - Check image dimension when computing border space size.

Fix a crash, taking it.
Should be in 57b4, gtb later today

Comment 14

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/releases/mozilla-beta/rev/41166905e77a
https://hg.mozilla.org/releases/mozilla-beta/rev/7aab168eac85

Comment 15

[Andrei Vaida [:avaida]]

(In reply to Ethan Lin[:ethlin] from comment #12)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no 

Setting qe-verify- based on Ethan's assessment on manual testing needs and the fact that this fix has automated coverage.