967808 - Faulty: crash in free() under _moz_pixman_region32_copy under nsRegion::Copy under LayerTransactionParent::RecvUpdate

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Faulty session

Found by Christoph Diehl's "Faulty" fuzzer, see bug 777067

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

free() crashes are often double frees, but here, see the GDB session attached, this doesn't look like already-freed memory. So I don't know what to make of this.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Note: found while running css-blending reftests.

Comment 3

[Andrew McCreight [:mccr8]]

Sounds memory-corruptiony.  Can you run the Faulty fuzzer with ASAN?

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Should be trivial, since the fuzzer is orthogonal to compiler or memory allocator considerations.

Remind me how to enable ASAN?

Comment 5

[Andrew McCreight [:mccr8]]

There's info about it here: https://developer.mozilla.org/en-US/docs/Building_Firefox_with_Address_Sanitizer

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Thanks; my future reports will be with an ASAN build.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Classification: PLayerTransactionParent, memory corruption, hard.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Another session, with ASan

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Andrew, the new log says it's using ASan at the top, and there is __asan::AsanThread::ThreadStart in the crash callstack, so I assume that ASan is taking effect here. Does this tell you anything? Did I have to do something different?

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

#1  0x00002aaab488c400 in nsRegion::Copy (this=0x617000214cd8, aRegion=...) at ../../dist/include/nsRegion.h:260
260         pixman_region32_copy(&mImpl, aRegion.Impl());
(gdb) p mImpl
$3 = {extents = {x1 = 0, y1 = 0, x2 = 0, y2 = 0}, data = 0x3ff0000000000000}
(gdb) p aRegion.Impl()
$4 = (pixman_region32_t *) 0x62100088138c
(gdb) p *aRegion.Impl()
$5 = {extents = {x1 = 0, y1 = 0, x2 = 0, y2 = 0}, data = 0x2aaad4b87040}

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Er, this looks like a bad pointer:

(gdb) p mImpl.data
$6 = (pixman_region32_data_t *) 0x3ff0000000000000

Looks like either something scribbled a region we're passing to pixman, or we're reinterpreting something else as a region.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Classification: PLayerTransaction, memory corruption, hard

Comment 13

[Andrew McCreight [:mccr8]]

Hmm.  Yeah, that is certainly not producing an ASAN report.  Maybe running it under the debugger interferes with how ASAN deals with a SIGSEV?  Maybe decoder has some idea?

An ASAN report will start out looking more like

==5650==ERROR: AddressSanitizer: heap-use-after-free on address 0xwhatever at pc [...]
READ of size 8 at 0xsomething thread T0

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Ah, sure, GDB is catching the segfault before ASan can.

But the above session still shows that there is no ASan-caught memory corruption before we crash, right?

Comment 15

[Andrew McCreight [:mccr8]]

Yeah, it sounds like it isn't a use-after-free at least, and we're just accessing some kind of garbage.

Comment 16

[Christian Holler (:decoder)]

It's certainly possible to crash without triggering any ASan mechanisms other than ASan's internal SIGSEGV handler (which will not be triggered if GDB catches the signal first). This will for example happen if we directly access a memory address that is not accessible but also has never been accessible before (so ASan doesn't consider it uaf or anything else).

Comment 17

[Andrew McCreight [:mccr8]]

Sounds bad.  Assigning to bjacob as he seems to be looking at the blocking bug already.

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

Fixed by the landing of PLayerTransaction type checks before casting layers, bug 968833.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Benoit, is this one of the fixes that you don't think makes sense to take on Aurora and Beta?

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

Yes, so do generally all blockers (or blockers of blockers) of bug 963790.

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qa-] for verification, lack of test case.