Closed Bug 1897561 Opened 1 month ago Closed 1 month ago

/src/gfx/cairo/cairo/src/cairo-recording-surface.c:488:54: runtime error: applying zero offset to null pointer

Categories

(Core :: Printing: Output, defect, P3)

Product:
Core
Component:
Printing: Output
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
128 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox126 --- unaffected
firefox127 --- fixed
firefox128 --- verified

People

(Reporter: tsmith, Assigned: jfkthame)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: pernosco, regression, testcase, Whiteboard: [bugmon:bisected,confirmed])

Attachments

(4 files)

testcase.html
348 bytes, text/html
Details
Bug 1897561 - Avoid trying to access region elements that didn't get set up (in case of an error status on the surface). r=#gfx-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Bug 1897561 followup - Use the min (not max) of the array lengths. r=#gfx-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Bug 1897561 - [beta] Avoid trying to access region elements that didn't get set up (in case of an error status on the surface).
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-beta+
Details | Review
Attached file testcase.html —

Found while fuzzing m-c 20240507-8d47b53b92aa (--enable-address-sanitizer --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework --upgrade
$ python -m fuzzfetch -a --fuzzing -n firefox
$ python -m grizzly.replay.bugzilla ./firefox/firefox <bugid>

/src/gfx/cairo/cairo/src/cairo-recording-surface.c:488:54: runtime error: applying zero offset to null pointer
    #0 0x724fbc09d47e in _cairo_recording_surface_region_array_destroy /src/gfx/cairo/cairo/src/cairo-recording-surface.c:488:54
    #1 0x724fbc06f0d8 in _paint_page /src/gfx/cairo/cairo/src/cairo-paginated-surface.c:545:9
    #2 0x724fbc06e888 in _cairo_paginated_surface_show_page /src/gfx/cairo/cairo/src/cairo-paginated-surface.c:602:14
    #3 0x724fbc06dec9 in _cairo_paginated_surface_finish /src/gfx/cairo/cairo/src/cairo-paginated-surface.c:206:11
    #4 0x724fbc0d6430 in _cairo_surface_finish /src/gfx/cairo/cairo/src/cairo-surface.c:1046:11
    #5 0x724fbc0d6430 in _moz_cairo_surface_finish /src/gfx/cairo/cairo/src/cairo-surface.c:1095:5
    #6 0x724fb329ef03 in mozilla::gfx::PrintTargetPDF::Finish() /src/gfx/thebes/PrintTargetPDF.cpp:103:16
    #7 0x724fb2a98634 in nsDeviceContext::EndDocument() /src/gfx/src/nsDeviceContext.cpp:319:19
    #8 0x724fbb284ef2 in mozilla::layout::RemotePrintJobParent::RecvFinalizePrint() /src/layout/printing/ipc/RemotePrintJobParent.cpp:256:26
    #9 0x724fba76a4a4 in mozilla::layout::PRemotePrintJobParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PRemotePrintJobParent.cpp:395:52
    #10 0x724fb94f7bb8 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PContentParent.cpp:6436:32
    #11 0x724fb20615c5 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /src/ipc/glue/MessageChannel.cpp:1820:25
    #12 0x724fb205d3bf in mozilla::ipc::MessageChannel::DispatchMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::UniquePtr<IPC::Message, mozilla::DefaultDelete<IPC::Message>>) /src/ipc/glue/MessageChannel.cpp:1739:9
    #13 0x724fb205e491 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::ipc::MessageChannel::MessageTask&) /src/ipc/glue/MessageChannel.cpp:1530:3
    #14 0x724fb205f9e3 in mozilla::ipc::MessageChannel::MessageTask::Run() /src/ipc/glue/MessageChannel.cpp:1630:14
    #15 0x724fb073e2da in mozilla::RunnableTask::Run() /src/xpcom/threads/TaskController.cpp:580:16
    #16 0x724fb072998d in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /src/xpcom/threads/TaskController.cpp:907:26
    #17 0x724fb0726f68 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /src/xpcom/threads/TaskController.cpp:730:15
    #18 0x724fb0727586 in mozilla::TaskController::ProcessPendingMTTask(bool) /src/xpcom/threads/TaskController.cpp:516:36
    #19 0x724fb0745951 in operator() /src/xpcom/threads/TaskController.cpp:234:37
    #20 0x724fb0745951 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_0>::Run() /src/xpcom/threads/nsThreadUtils.h:548:5
    #21 0x724fb0768574 in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1199:16
    #22 0x724fb0773ae8 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:480:10
    #23 0x724fb2069f4e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:85:21
    #24 0x724fb1eb6884 in RunInternal /src/ipc/chromium/src/base/message_loop.cc:370:10
    #25 0x724fb1eb6884 in RunHandler /src/ipc/chromium/src/base/message_loop.cc:363:3
    #26 0x724fb1eb6884 in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:345:3
    #27 0x724fba1b44f9 in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:148:27
    #28 0x724fba36efca in nsAppShell::Run() /src/widget/gtk/nsAppShell.cpp:469:33
    #29 0x724fbe426eb8 in nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:296:30
    #30 0x724fbe6d9ae0 in XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:5770:22
    #31 0x724fbe6db1e9 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:5982:8
    #32 0x724fbe6dc021 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:6039:21
    #33 0x571839b4d727 in do_main /src/browser/app/nsBrowserApp.cpp:230:22
    #34 0x571839b4d727 in main /src/browser/app/nsBrowserApp.cpp:448:16
    #35 0x724fd4029d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #36 0x724fd4029e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #37 0x571839a74898 in _start (/home/user/workspace/browsers/m-c-20240517162708-fuzzing-asan-opt/firefox+0xda898) (BuildId: 91dc42aa0c8c221b4849a774131bcba4130edb67)

SUMMARY: UndefinedBehaviorSanitizer: nullptr-with-offset /src/gfx/cairo/cairo/src/cairo-recording-surface.c:488:54
Flags: in-testsuite?

Verified bug as reproducible on mozilla-central 20240517215359-f35859c2fd56.
Unable to bisect testcase (Unable to launch the start build!):

Start: 8e49356191a8cf73811adf08cba1dd44b2276c7c (20230520092140)
End: 8d47b53b92aab287aee923b25739e74f89cc323f (20240507091307)
BuildFlags: BuildFlags(asan=True, tsan=False, debug=False, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False, nyx=False)

Whiteboard: [bugmon:bisected,confirmed]

Unable to reproduce.

Yeah, same, it doesn't repro here, but maybe a dupe / papered by bug 1896173? In any case it being UBSan means that it doesn't crash for users...

Severity: -- → S3
Priority: -- → P3
See Also: → 1896173

Successfully recorded a pernosco session. A link to the pernosco session will be added here shortly.

Keywords: pernosco-wanted → pernosco

A pernosco session for this bug can be found here.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Yeah, same, it doesn't repro here, but maybe a dupe / papered by bug 1896173? In any case it being UBSan means that it doesn't crash for users...

Unfortunately, I think users could still be affected. The UBSan error happens on the array indexing operation here, because region_elements is null. For users, without ubsan stepping in there, we'll proceed with region_element pointing at or near null, and then try to dereference it a few lines later. That might crash for users? If it doesn't, then we'll be passing garbage in to destroy_pattern_region_array, and I doubt that's going to end well.

The code in _cairo_recording_surface_region_array_destroy clearly expects that region_elements will be parallel to the elements array, with the same number of entries. So why is it actually null here? It looks like the proximate cause is that when _paint_page called _cairo_recording_surface_replay_and_create_regions, the recording surface was already in an error state with status=CAIRO_STATUS_FREETYPE_ERROR, and this means that _cairo_recording_surface_replay_internal bailed out here and the regions array didn't get populated.

It looks like the underlying FreeType error occurs during FT_Set_Char_Size, which gets called by _cairo_ft_unscaled_font_set_scale with huge scaling values; here I'm seeing sf.x_scale as 131351.23297119141.

The huge value will be a result of the testcase having

* {
  transform: scale(9, 1);
}

which recursively scales elements, so that by the time we're 3 elements deep, we're x-scaling by 729 times. Combine that with the 77em font-size in the testcase, and it's not surprising something in FreeType breaks.

The question, then, is where this failure should have been handled safely. Probably one of our cairo calls earlier returned an error status, and we didn't handle it but continued to try and render.

But it also seems like cairo should perhaps be more careful about its cleanup when the surface is in an error state; I'm concerned that even if we'd detected the error earlier and bailed out of printing, we'd have hit this issue during teardown of the cairo context. (But I haven't confirmed that would happen.)

@ajohnson, am I right in suspecting that once the recording surface is in such an error state, it may not be possible to safely destroy it, so _cairo_recording_surface_region_array_destroy should be treading more carefully here?

Flags: needinfo?(ajohnson)

The problem appears to be the assumption that the size of elements and region_elements are the same fails when on the error path. I think setting num_elements in _cairo_recording_surface_region_array_destroy() to MAX(surface->commands.num_elements, _cairo_array_num_elements(&region_array->regions)) would fix this.

The font error should have been picked up earlier. _cairo_recording_surface_show_text_glyphs() should be checking the scaled_font status.

Flags: needinfo?(ajohnson)

(In reply to Jonathan Kew [:jfkthame] from comment #6)

Unfortunately, I think users could still be affected. The UBSan error happens on the array indexing operation here, because region_elements is null.

Yeah, you're 100% right. I was assuming when reading comment 0 something like "region_elements" is null, but it's not used because all the operators are TAG, which don't use the region_element, or something along those lines. But the pernosco recording shows that that's not what's going on at all.

Keywords: regression
Regressed by: 1892913

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

status-firefox126: --- → unaffected
status-firefox127: --- → affected
status-firefox-esr115: --- → unaffected

(In reply to ajohnson@redneon.com from comment #7)

The problem appears to be the assumption that the size of elements and region_elements are the same fails when on the error path. I think setting num_elements in _cairo_recording_surface_region_array_destroy() to MAX(surface->commands.num_elements, _cairo_array_num_elements(&region_array->regions)) would fix this.

Agreed, this should avoid the UBSan error and potential crash. I'll post a patch for Firefox here. Assume you'll deal with it upstream in cairo?

The font error should have been picked up earlier. _cairo_recording_surface_show_text_glyphs() should be checking the scaled_font status.

Yeah, it does check the scaled_font status and sets status=CAIRO_STATUS_FREETYPE_ERROR on the recording surface, which means that subsequent operations on the surface will just bail out. But when we call cairo_surface_finish to clean up at the end of the job (which AIUI is the right thing to do, even if the surface is in an error state), we reach this code.

Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7cef5b4e2052
Avoid trying to access region elements that didn't get set up (in case of an error status on the surface). r=gfx-reviewers,lsalzman

https://hg.mozilla.org/mozilla-central/rev/7cef5b4e2052

Status: ASSIGNED → RESOLVED
Closed: 1 month ago
status-firefox128: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 128 Branch

Bug marked as FIXED but still reproduces on mozilla-central 20240520214633-0e8cd4eeebfd. If you believe this to be incorrect, please remove the bugmon keyword to prevent further analysis.

Status: RESOLVED → REOPENED
status-firefox128: fixed → affected
Resolution: FIXED → ---

Argh, silly error -- the patch should be getting the MIN (not MAX) of the two array lengths! Followup fix coming.

Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/005a4acbead5
followup - Use the min (not max) of the array lengths. r=gfx-reviewers,nical

https://hg.mozilla.org/mozilla-central/rev/005a4acbead5

Status: REOPENED → RESOLVED
Closed: 1 month ago → 1 month ago
status-firefox128: affected → fixed
Resolution: --- → FIXED

Verified bug as fixed on rev mozilla-central 20240521213834-d70074e160c5.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Status: RESOLVED → VERIFIED
status-firefox128: fixed → verified
Keywords: bugmon
Attachment #9403166 - Flags: approval-mozilla-beta?

beta Uplift Approval Request

  • User impact if declined: possible out-of-bounds access/crash after an error occurs during pdf generation
  • Code covered by automated testing: no
  • Fix verified in Nightly: yes
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing: n/a
  • Risk associated with taking this patch: minimal
  • Explanation of risk level: trivial patch to avoid assuming region_array was fully populated
  • String changes made/needed: none
  • Is Android affected?: yes
Attachment #9403166 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: