1706441 - crash in [@ blend_span]

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

First found while fuzzing m-c 20210418-78764895b2a6 (--enable-coverage --enable-fuzzing)

I can only reproduce this with a fuzzing-ccov build. No idea why. No repro on fuzzing-asan-opt, fuzzing-debug, ccov-opt or opt.

I'm happy to test patches if you have guesses.

==179379==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x7f9fe27a2460 (pc 0x7fa156e36cc8 bp 0x000000000000 sp 0x7fa0b7e96830 T179430)
==179379==The signal is caused by a READ memory access.
    #0 0x7fa156e36cc8 in memcpy /builds/worker/fetches/sysroot/usr/include/x86_64-linux-gnu/bits/string3.h:51:10
    #1 0x7fa156e36cc8 in load<unsigned int> src/gfx/wr/swgl/src/vector_type.h:503:5
    #2 0x7fa156e36cc8 in unaligned_load<unsigned char __attribute__((ext_vector_type(16))), unsigned int> src/gfx/wr/swgl/src/vector_type.h:532:10
    #3 0x7fa156e36cc8 in blend_span src/gfx/wr/swgl/src/blend.h:732:28
    #4 0x7fa156e36cc8 in commit_blend_span<true, unsigned int, unsigned short __attribute__((ext_vector_type(16)))> src/gfx/wr/swgl/src/blend.h:792:22
    #5 0x7fa156e36cc8 in unsigned int* blendTextureLinearFallback<true, glsl::sampler2D_impl*, NoColor, unsigned int>(glsl::sampler2D_impl*, glsl::vec2, int, glsl::vec2_scalar, glsl::vec2_scalar, glsl::vec2_scalar, NoColor, unsigned int*) src/gfx/wr/swgl/src/swgl_ext.h:177:5
    #6 0x7fa156f0906a in int blendTextureLinear<true, glsl::sampler2D_impl*, NoColor, unsigned int>(glsl::sampler2D_impl*, glsl::vec2, int, glsl::vec4_scalar const&, NoColor, unsigned int*, LinearFilter) src/gfx/wr/swgl/src/swgl_ext.h:456:3
    #7 0x7fa156f83f3f in brush_image_ALPHA_PASS_TEXTURE_2D_frag::swgl_drawSpanRGBA8() /builds/worker/workspace/obj-build/x86_64-unknown-linux-gnu/release/build/swgl-c0a134755c64a46a/out/brush_image_ALPHA_PASS_TEXTURE_2D.h:895:2
    #8 0x7fa156f7f8fb in brush_image_ALPHA_PASS_TEXTURE_2D_frag::draw_span_RGBA8(brush_image_ALPHA_PASS_TEXTURE_2D_frag*) /builds/worker/workspace/obj-build/x86_64-unknown-linux-gnu/release/build/swgl-c0a134755c64a46a/out/brush_image_ALPHA_PASS_TEXTURE_2D.h:938:42
    #9 0x7fa157144c48 in draw_span src/gfx/wr/swgl/src/program.h:149:12
    #10 0x7fa157144c48 in draw_depth_span<unsigned int> src/gfx/wr/swgl/src/rasterize.h:597:38
    #11 0x7fa157144c48 in void draw_quad_spans<unsigned int>(int, glsl::vec2_scalar*, unsigned int, glsl::vec3*, Texture&, Texture&, ClipRect const&) src/gfx/wr/swgl/src/rasterize.h:999:13
    #12 0x7fa156da9dcf in draw_quad(int, Texture&, Texture&) src/gfx/wr/swgl/src/rasterize.h:1592:5
    #13 0x7fa156da6e93 in void draw_elements<unsigned short>(int, int, unsigned long, VertexArray&, Texture&, Texture&) src/gfx/wr/swgl/src/rasterize.h:1622:5
    #14 0x7fa156da6c68 in DrawElementsInstanced src/gfx/wr/swgl/src/gl.cc:2699:7
    #15 0x7fa155a2ce24 in webrender::device::gl::Device::draw_indexed_triangles_instanced_u16::he541a299179565db src/gfx/wr/webrender/src/device/gl.rs:3556:9
    #16 0x7fa155a2ce24 in webrender::renderer::Renderer::draw_instanced_batch::h36364c6060e0f56b src/gfx/wr/webrender/src/renderer/mod.rs:2561:17
    #17 0x7fa155a2a967 in webrender::renderer::Renderer::draw_alpha_batch_container::h196dfcc56eac88f7 src/gfx/wr/webrender/src/renderer/mod.rs:3045:17
    #18 0x7fa155a2415e in webrender::renderer::Renderer::draw_picture_cache_target::ha4153bd6409179a8 src/gfx/wr/webrender/src/renderer/mod.rs:2868:9
    #19 0x7fa155a20936 in webrender::renderer::Renderer::draw_frame::h406fc00bfb194484 src/gfx/wr/webrender/src/renderer/mod.rs:4683:21
    #20 0x7fa155a57937 in webrender::renderer::Renderer::render_impl::h0f98e3578feb63ca src/gfx/wr/webrender/src/renderer/mod.rs:2159:17
    #21 0x7fa155a64899 in webrender::renderer::Renderer::render::h3e3d3b3a9b7c29e2 src/gfx/wr/webrender/src/renderer/mod.rs:1894:30
    #22 0x7fa156188f7c in wr_renderer_render src/gfx/webrender_bindings/src/bindings.rs:636:11
    #23 0x7fa148d49881 in mozilla::wr::RendererOGL::UpdateAndRender(mozilla::Maybe<mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> > const&, mozilla::Maybe<mozilla::wr::ImageFormat> const&, mozilla::Maybe<mozilla::Range<unsigned char> > const&, bool*, mozilla::wr::RendererStats*) src/gfx/webrender_bindings/RendererOGL.cpp:186:8
    #24 0x7fa148d4861c in mozilla::wr::RenderThread::UpdateAndRender(mozilla::wr::WrWindowId, mozilla::layers::BaseTransactionId<mozilla::VsyncIdType> const&, mozilla::TimeStamp const&, bool, mozilla::Maybe<mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> > const&, mozilla::Maybe<mozilla::wr::ImageFormat> const&, mozilla::Maybe<mozilla::Range<unsigned char> > const&, bool*) src/gfx/webrender_bindings/RenderThread.cpp:486:31
    #25 0x7fa148d47dfe in mozilla::wr::RenderThread::HandleFrameOneDoc(mozilla::wr::WrWindowId, bool) src/gfx/webrender_bindings/RenderThread.cpp:341:3
    #26 0x7fa148d547ae in applyImpl<mozilla::wr::RenderThread, void (mozilla::wr::RenderThread::*)(mozilla::wr::WrWindowId, bool), StoreCopyPassByConstLRef<mozilla::wr::WrWindowId>, StoreCopyPassByConstLRef<bool> , 0, 1> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1148:12
    #27 0x7fa148d547ae in apply<mozilla::wr::RenderThread, void (mozilla::wr::RenderThread::*)(mozilla::wr::WrWindowId, bool)> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1154:12
    #28 0x7fa148d547ae in mozilla::detail::RunnableMethodImpl<mozilla::wr::RenderThread*, void (mozilla::wr::RenderThread::*)(mozilla::wr::WrWindowId, bool), true, (mozilla::RunnableKind)0, mozilla::wr::WrWindowId, bool>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1201:13
    #29 0x7fa1476f7df5 in MessageLoop::RunTask(already_AddRefed<nsIRunnable>) src/ipc/chromium/src/base/message_loop.cc:468:11
    #30 0x7fa1476f8514 in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask&&) src/ipc/chromium/src/base/message_loop.cc:477:5
    #31 0x7fa1476f880a in MessageLoop::DoWork() src/ipc/chromium/src/base/message_loop.cc:552:13
    #32 0x7fa1476f9450 in base::MessagePumpDefault::Run(base::MessagePump::Delegate*) src/ipc/chromium/src/base/message_pump_default.cc:35:31
    #33 0x7fa1476f7bbf in RunInternal src/ipc/chromium/src/base/message_loop.cc:335:10
    #34 0x7fa1476f7bbf in RunHandler src/ipc/chromium/src/base/message_loop.cc:328:3
    #35 0x7fa1476f7bbf in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:310:3
    #36 0x7fa14770bea1 in base::Thread::ThreadMain() src/ipc/chromium/src/base/thread.cc:191:16
    #37 0x7fa1477022e1 in ThreadFunc(void*) src/ipc/chromium/src/base/platform_thread_posix.cc:40:13
    #38 0x7fa16b594608 in start_thread /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477:8
    #39 0x7fa16b15d292 in clone /build/glibc-eX1tMB/glibc-2.31/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Comment 1

[Tyson Smith [:tsmith]]

Attachment: prefs.js

Comment 2

[Lee Salzman [:lsalzman]]

Nothing I can really do here without being able to reliably catch this in the act, and the stack trace isn't saying much about what is really going on. At a minimum a pernosco session would be helpful.

Comment 3

[Tyson Smith [:tsmith]]

No problem. I made a ccov-fuzzing-noopt build to run under rr and of course it does not repro :(

I'll keep an eye on our buckets and hopefully a more reliable test case comes in.

Comment 4

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20210430214504-b417d526e5fc.
The bug appears to have been introduced in the following build range:

Start: 6f3a9007793c5f356522b8d283e9e2bc4a4f6b4f (20210402094809)
End: c2f0b7fb408a5b29c04d68b633a8ccbc3e19a425 (20210402214149)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6f3a9007793c5f356522b8d283e9e2bc4a4f6b4f&tochange=c2f0b7fb408a5b29c04d68b633a8ccbc3e19a425

Comment 5

[Daniel Veditz [:dveditz]]

just guessing, but the fix for bug 1701975 is in this range and touches this code.

CCov builds are slow so maybe that compile option makes some races more likely?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Waiting on a more reliable reproduction so that it can be debugged.

Comment 7

[Lee Salzman [:lsalzman]]

Still waiting on a better repro here...

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S4. However, the bug is flagged with the sec-high keyword.
:bhood, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 9

[Lee Salzman [:lsalzman]]

Tyson, does this still reproduce at all?

Comment 10

[Tyson Smith [:tsmith]]

I was able to reproduce the issue with the attached testcase and prefs.js with m-c 20220328-eaf3521f3b37.

On Ubuntu 20.04 (and 18.04) I used Grizzly Replay:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch --cov --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html --xvfb

A Pernosco session is available here: https://pernos.co/debug/zHlyVfNAfxnCSxAEbyg6iQ/index.html

Please let me know if you'd like any patches tested.

Comment 11

[Lee Salzman [:lsalzman]]

Without debugging information it is hard to see much of anything about what's going on in that trace...

Comment 12

[Tyson Smith [:tsmith]]

Here is another attempt: https://pernos.co/debug/UnanKBsMaXGNfZEa6fR9Mg/index.html

This is tricky because for some reason this crash requires a ccov build. So I need to create a ccov build with --enable-debug and with the lowest debug level possible. Of course when I build with --disable-optimize I am no longer able to reproduce the issue. The build used to collect this trace is using -O1. In the trace some information seems to be available in shallower frames but not deeper frames, I'm not sure if there is enough to help you.

If this is potentially due to a compiler option like bug 1746545 send me a list of options to try and I'd be happy to test.

Comment 13

[Lee Salzman [:lsalzman]]

Attachment: Bug 1706441 - Check for non-empty span. r?jrmuizel

Comment 14

[Lee Salzman [:lsalzman]]

It appears what's going on here is clang is being a bit too smart on its optimizations. Within the branch uv_step.x > 0.0f && insideDist >= uv_step.x it infers that both are non-zero, and further that int(insideDist / uv_step.x) * swgl_StepSize is >= 0, and so it elides the check for max(that, 0) = that > 0 ? that : 0 within the clamp since it determines that >= 0. Where we fall afoul of things is that the * swgl_StepSize operation overflows, which falls into "undefined behavior" land, and the compiler pretends it will never overflow as a result and that the operation is always predictably >= 0. This patch restructures the check so that inside is compared > 0, which the compiler can't guarantee or elide, so that the undefined behavior no longer matters...

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

:lsalzman, since this bug contains a bisection range, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 16

[Lee Salzman [:lsalzman]]

Comment on attachment 9269994 [details]
Bug 1706441 - Check for non-empty span. r?jrmuizel

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily. Only seems to reproduce in fuzzing coverage builds.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 91+
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Not hard or risky.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1692731

Comment 18

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9269994 [details]
Bug 1706441 - Check for non-empty span. r?jrmuizel

Approved to land and uplift

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Check for non-empty span. r=gfx-reviewers,aosmond
https://hg.mozilla.org/integration/autoland/rev/88cc45d69235ef56bbd5a3f6cac4b09ec083b25f
https://hg.mozilla.org/mozilla-central/rev/88cc45d69235

Comment 20

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20220407092959-6ff2b7d52aa3.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:lsalzman, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 22

[Lee Salzman [:lsalzman]]

Comment on attachment 9269994 [details]
Bug 1706441 - Check for non-empty span. r?jrmuizel

Beta/Release Uplift Approval Request

• User impact if declined: Potential buffer underread leading to crashes. Unknown if this affects release builds.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just fixes a correctness issue related to compositing empty areas - it just skips them now. Risk should be very minimal.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):

Comment 23

[Dianna Smith [:diannaS]]

Comment on attachment 9269994 [details]
Bug 1706441 - Check for non-empty span. r?jrmuizel

Approved for 100.0b5

Comment 24

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/70afa20133bc

Comment 25

[Dianna Smith [:diannaS]]

Comment on attachment 9269994 [details]
Bug 1706441 - Check for non-empty span. r?jrmuizel

Approved for 91.9esr

Comment 26

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/608b8cfa8ae8