1704176 - addition of unsigned offset overflowed in gfx/wr/swgl/src/gl.cc:540:16

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Tyson Smith [:tsmith]]

Found while fuzzing m-c 20210409-7bc2dd06085f (--enable-address-sanitizer --enable-fuzzing)

This issue is not reproducible with the reported test case. Hopefully the report is enough to go on.

src/gl.cc:540:16: runtime error: addition of unsigned offset to 0x7f35e7c1e800 overflowed to 0x6f35e7c1e800
    #0 0x7f361e1782e7 in Texture::sample_ptr(int, int) const /gecko/gfx/wr/swgl/src/gl.cc:540:16
    #1 0x7f361e65638a in void draw_perspective_spans<unsigned int>(int, glsl::vec4_scalar*, glsl::vec3*, Texture&, Texture&, ClipRect const&) /gecko/gfx/wr/swgl/src/rasterize.h:1143:26
    #2 0x7f361e6553e5 in draw_perspective_clipped(int, glsl::vec4_scalar*, glsl::vec3*, Texture&, Texture&) /gecko/gfx/wr/swgl/src/rasterize.h:1410:5
    #3 0x7f361e18a9a6 in draw_perspective /gecko/gfx/wr/swgl/src/rasterize.h:1453:5
    #4 0x7f361e18a9a6 in draw_quad(int, Texture&, Texture&) /gecko/gfx/wr/swgl/src/rasterize.h:1526:5
    #5 0x7f361e188313 in void draw_elements<unsigned short>(int, int, unsigned long, VertexArray&, Texture&, Texture&) /gecko/gfx/wr/swgl/src/rasterize.h:1622:5
    #6 0x7f361e187fb9 in DrawElementsInstanced /gecko/gfx/wr/swgl/src/gl.cc:2699:7
    #7 0x7f361d5f6095 in webrender::device::gl::Device::draw_indexed_triangles_instanced_u16::h9db85ebc5dd1be98 /gecko/gfx/wr/webrender/src/device/gl.rs:3522:9
    #8 0x7f361d5f6095 in webrender::renderer::Renderer::draw_instanced_batch::h577f8b0a4da3cc1e /gecko/gfx/wr/webrender/src/renderer/mod.rs:2557:17
    #9 0x7f361d5e22fd in webrender::renderer::Renderer::draw_alpha_batch_container::hfa1e675e0906feac /gecko/gfx/wr/webrender/src/renderer/mod.rs:3041:17
    #10 0x7f361d5b3d74 in webrender::renderer::Renderer::draw_color_target::h9fb34d9e1c891ad9 /gecko/gfx/wr/webrender/src/renderer/mod.rs:3710:13
    #11 0x7f361d5b3d74 in webrender::renderer::Renderer::draw_frame::hfb4bc1ed9933ca68 /gecko/gfx/wr/webrender/src/renderer/mod.rs:4700:17
    #12 0x7f361d61c1af in webrender::renderer::Renderer::render_impl::h4449767a88cd8c73 /gecko/gfx/wr/webrender/src/renderer/mod.rs:2155:17
    #13 0x7f361d63da69 in webrender::renderer::Renderer::render::h982b98525d4d797e /gecko/gfx/wr/webrender/src/renderer/mod.rs:1890:30
    #14 0x7f361d8a9eef in wr_renderer_render /gecko/gfx/webrender_bindings/src/bindings.rs:637:11
    #15 0x7f360ee2d5ce 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*) /gecko/gfx/webrender_bindings/RendererOGL.cpp:186:8
    #16 0x7f360ee2bcff 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*) /gecko/gfx/webrender_bindings/RenderThread.cpp:486:31
    #17 0x7f360ee2ae81 in mozilla::wr::RenderThread::HandleFrameOneDoc(mozilla::wr::WrWindowId, bool) /gecko/gfx/webrender_bindings/RenderThread.cpp:341:3
    #18 0x7f360ee43216 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
    #19 0x7f360ee43216 in apply<mozilla::wr::RenderThread, void (mozilla::wr::RenderThread::*)(mozilla::wr::WrWindowId, bool)> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1154:12
    #20 0x7f360ee43216 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
    #21 0x7f360d0e3cc7 in MessageLoop::RunTask(already_AddRefed<nsIRunnable>) /gecko/ipc/chromium/src/base/message_loop.cc:468:11
    #22 0x7f360d0e4a2e in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask&&) /gecko/ipc/chromium/src/base/message_loop.cc:477:5
    #23 0x7f360d0e52cb in MessageLoop::DoWork() /gecko/ipc/chromium/src/base/message_loop.cc:552:13
    #24 0x7f360d0e65c6 in base::MessagePumpDefault::Run(base::MessagePump::Delegate*) /gecko/ipc/chromium/src/base/message_pump_default.cc:35:31
    #25 0x7f360d0e3871 in RunInternal /gecko/ipc/chromium/src/base/message_loop.cc:335:10
    #26 0x7f360d0e3871 in RunHandler /gecko/ipc/chromium/src/base/message_loop.cc:328:3
    #27 0x7f360d0e3871 in MessageLoop::Run() /gecko/ipc/chromium/src/base/message_loop.cc:310:3
    #28 0x7f360d100ef8 in base::Thread::ThreadMain() /gecko/ipc/chromium/src/base/thread.cc:191:16
    #29 0x7f360d0f4aec in ThreadFunc(void*) /gecko/ipc/chromium/src/base/platform_thread_posix.cc:40:13
    #30 0x7f362c99b608 in start_thread /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477:8
    #31 0x7f362c564292 in clone /build/glibc-eX1tMB/glibc-2.31/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Comment 1

[Lee Salzman [:lsalzman]]

Without a test-case, I am not sure this is actionable.

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

This testcase was originally reported in bug 1748499. However, once that issue was addressed, the stack described in comment #0 will trigger occasionally.

Comment 3

[Tyson Smith [:tsmith]]

I am able to consistently reproduce this with the attached test case.

This UBSan check is enabled when building with --enable-undefined-sanitizer.

STR:

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

I'm running Ubuntu 20.04. I've tested on my hardware and in a VM. --xvfb seems to be helpful in some cases (screen res related?).

I am unable to get a Pernosco session because this does not reproduce with a no-opt build, possibly due to the fact that this is UB.

Lee, please let me know if you have any issues reproducing the issue.

Comment 4

[Lee Salzman [:lsalzman]]

This does not seem to reproduce on a normal build at all, no matter what I try, so I am essentially unsure what would be causing it.

Comment 5

[Tyson Smith [:tsmith]]

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

I was only able to reproduce the issue with a build that has an optimization level >= -O1. Hopefully this provides enough information to get an understanding of what is going on.

I will happily test any patches if needed.

Comment 6

[Lee Salzman [:lsalzman]]

The pernosco session would indicate the existing bounds checking is working fine. While it may do the pointer math, that pointer will never be actually sampled from at all and is otherwise innocuous. Since this is in a fast-path/very hot code and benign, it is not imperative that this warning be fixed especially if it would add extra branching/slowness to the fast-path. If I have any idea as to how to work around this, I will address it later, but for now, I am going to think on it.

Comment 7

[Christian Holler (:decoder)]

(In reply to Lee Salzman [:lsalzman] from comment #6)

The pernosco session would indicate the existing bounds checking is working fine. While it may do the pointer math, that pointer will never be actually sampled from at all and is otherwise innocuous. Since this is in a fast-path/very hot code and benign, it is not imperative that this warning be fixed especially if it would add extra branching/slowness to the fast-path. If I have any idea as to how to work around this, I will address it later, but for now, I am going to think on it.

The bug shown in comment 0 is not just a warning but a confirmed instance of undefined behavior.

Per specification (see e.g. C99, Section 6.5.6), pointer arithmetic is not allowed to wrap. There is a number of things that can go wrong here, such as:

• The compiler removing the overflow check now or in the future, based on the assumption that it is dead code.
• Incorrect offset computations.
• Internal corruptions because the pointer is taking special values that are reserved by the compiler/LLVM.

This is not a hypothetical problem, Clang/LLVM is doing all of these things already right now (integer overflow is a prime example, Clang will aggressively optimize under the assumption that no undefined behavior occurs, e.g. for signed integer overflow). And even if the code right now does the right thing (which we would have to validate from the assembly), it might not do so anymore on another platform or another compiler version. We do not want to keep such bugs in our codebase unless there is a way to really ensure that the code cannot be miscompiled.

If you need further information on this, please reach out and I'll be glad to provide some pointers.

Comment 8

[Lee Salzman [:lsalzman]]

This might be resolved by bug 1746545.

Comment 9

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Testcase crashes using the initial build (mozilla-central 20210409092020-7bc2dd06085f) but not with tip (mozilla-central 20220127213627-b9121c1a4330.)
The bug appears to have been fixed in the following build range:

Start: 25ebce40c6543889148a63765a16add6ddbf6721 (20220114220151)
End: 25ebce40c6543889148a63765a16add6ddbf6721 (20220114220151)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=25ebce40c6543889148a63765a16add6ddbf6721&tochange=25ebce40c6543889148a63765a16add6ddbf6721
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 10

[Tyson Smith [:tsmith]]

This bisection range is not what I expected (fixed by bug 1746545). Can you please have a look when you have a chance? Maybe something to do with the fact that this test previously found another issue (comment #2) and is confusing bugmon?

Comment 11

[Jason Kratzer [:jkratzer]]

I'm still investigating what went wrong with the bisection but I can confirm that bug 1746545 did in fact fix this issue.