1824800 - Use-after-free crash in [@ _cups_strcasecmp] called from nsPrinterCUPS::TryEnsurePrinterInfo

Status: RESOLVED FIXED

(Core :: Printing: Setup, defect, P2)

Description

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/c323cd55-eef2-4699-84f6-826680230324

Reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 10 frames of crashing thread:

0  libcups.2.dylib  _cups_strcasecmp  
1  libcups.2.dylib  cupsRemoveOption  
2  libcups.2.dylib  cupsGetOption  
3  libcups.2.dylib  _cupsGetDestResource  
4  libcups.2.dylib  cupsCopyDestInfo  
5  XUL  nsPrinterCUPS::TryEnsurePrinterInfo const  widget/nsPrinterCUPS.cpp:444
6  XUL  nsPrinterCUPS::Supports const  widget/nsPrinterCUPS.cpp:171
7  XUL  mozilla::SpawnPrintBackgroundTask<nsPrinterBase, bool, > const  widget/PrintBackgroundTask.h:56
7  XUL  std::__1::__invoke_constexpr<mozilla::SpawnPrintBackgroundTask<nsPrinterBase, bool, >  /builds/worker/fetches/MacOSX13.0.sdk/usr/include/c++/v1/type_traits:3924
7  XUL  std::__1::__apply_tuple_impl<mozilla::SpawnPrintBackgroundTask<nsPrinterBase, bool, >  /builds/worker/fetches/MacOSX13.0.sdk/usr/include/c++/v1/tuple:1536

This looks like the same basic issue as bug 1746956, but that bug was closed so I figured I'd filed a new bug. A decent number of these are use-after-free crashes. I see 29 of those in the last two weeks on recent versions. I'll mark this sec-moderate because it requires the user to print, I assume.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

So nsPrinterCUPS::Connection::GetConnection can be called from multiple threads, but it seems cupsConnectDest isn't thread-safe to me... Emily, do you have cycles to look? It seems this could explain some of the related crashes we've been seeing, and this is all the preliminary connection we started doing in bug 1662946...

Comment 2

[Emily McDonough [:alaskanemily]]

If this is the same as Bug 1746956, then this will still happen with older versions of CUPS. It requires either the patch I sent to OpenPrinting CUPS, or the update Apple did for CUPS (depending on platform).
However, this was supposed to be fixed in OS X 12.4 with Apple's cups-499.4, so the linked crash should have the fix already.

I wonder if this is related to bug 1823565? Also given all three of these are related to CUPS strings, it might be that the fix for OpenPrinting CUPS/Apple's CUPS for bug 1746956, and that I couldn't seem to fix bug 1746956 even when explicitly testing for what should have been triggering the crash, this is probably all one memory corruption or CUPS state clobbering issue.

After looking at bug 1823565, I am tempted to wrap nsPrinterCUPS::mPrinter in a mutex even though it really looks like it shouldn't be necessary. I'm not really sure where else threading issues could be coming into play, and unless we're getting memory corruption outside our usage of CUPS that somehow just happens to mostly cause problems when we call into CUPS, I don't have any idea what could be causing these crashes.

Comment 3

[Emily McDonough [:alaskanemily]]

I've created bug 1826872 which moves the cups_dest_t behind the same mutex as the cups_dinfo_t. I suspect what is happening is that some of the code that uses the cups_dest_t to get data actually modifies the cups_dest_t, and/or that the code that uses the cups_dinfo_t can also modify the original cups_dest_t.

That change will lock both at once, ensuring both that the cups_dest_t is always locked when accessed, and that any cups_dinfo_t cannot be used without also locking the cups_dest_t. I'm not sure if the latter is actually necessary or not, but this is a simple way to do it that covers both cases.

Comment 4

[Daniel Holbert [:dholbert]]

(Assigning to make it clear that this is being worked on, and is hopefully-fixed, per comment 3.)

Comment 5

[Daniel Holbert [:dholbert]]

(Also: there's still no new crash data here in versions 114 or newer, which lends credibility to the theory that bug 1826872 has indeed fixed this.)

Worth double-checking that this is still the case after 114 hits release.

Comment 6

[Emily McDonough [:alaskanemily]]

Yeah, I don't think we can really tell much about how effective the change is until version 114 is release. The stacktraces imply it should be effective, though.

Comment 7

[Daniel Holbert [:dholbert]]

2 months have passed, we're releasing 116 tomorrowish, and there are no crashes here since 114.

Per comment 5 - comment 6, I think we can declare victory here.