845125 - Mac: Crash when printing csptesting.herokuapp.com to PDF w/ heap-use-after-free

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

1. Go to this URL:

http://csptesting.herokuapp.com/

2. Without pressing that button, select Print > PDF > Save as PDF and save.

Result:
Crash, see ASan log below.

Does not appear to crash in daily dbg build, only ASan.

Marking s-s only because it's a crash.



==4374== ERROR: AddressSanitizer: heap-use-after-free on address 0x0001531da040 at pc 0x10000a520 bp 0x7fff5fbfbf50 sp 0x7fff5fbfbf28
WRITE of size 1 at 0x0001531da040 thread T0
    #0 0x10000a51f in 0x20000a51f
    #1 0x7fff8402b99e in CGAccessSessionGetBytes (in CoreGraphics) + 134
    #2 0x17908cdef in unpackImageRow (in libPDFRIP.A.dylib) + 71
    #3 0x17908cd28 in PDFImageEmitData (in libPDFRIP.A.dylib) + 1513
    #4 0x17908daec in PDFImageEmitDefinition (in libPDFRIP.A.dylib) + 428
    #5 0x7fff85ce6e51 in __CFSetApplyFunction_block_invoke_0 (in CoreFoundation) + 17
    #6 0x7fff85bfbd7f in CFBasicHashApply (in CoreFoundation) + 127
    #7 0x7fff85bfbced in CFSetApplyFunction (in CoreFoundation) + 157
    #8 0x17908e7e4 in PDFImageSetEmitDefinitions (in libPDFRIP.A.dylib) + 58
    #9 0x179087ce9 in emit_page_resources(PDFDocument*) (in libPDFRIP.A.dylib) + 103
    #10 0x179087c60 in PDFDocumentEndPage (in libPDFRIP.A.dylib) + 72
    #11 0x1790867f6 in pdf_EndPage (in libPDFRIP.A.dylib) + 16
    #12 0x7fff8b4e3b8a in pdfSpoolingEndPage(void*, void*) (in PrintCore) + 544
    #13 0x7fff8b4dae9e in PJCEndPage(OpaquePMPrintSession*) (in PrintCore) + 42
    #14 0x7fff8b4b8ce0 in PMSessionEndPageNoDialog (in PrintCore) + 79
    #15 0x1062b9e6a in nsDeviceContextSpecX::EndPage() (in XUL) + 42
    #16 0x104270816 in nsDeviceContext::EndPage() (in XUL) + 182
    #17 0x1045b4175 in nsSimplePageSequenceFrame::DoPageEnd() (in XUL) + 181
    #18 0x10571c381 in nsPrintEngine::PrintPage(nsPrintObject*, bool&) (in XUL) + 3201
    #19 0x1057233ca in nsPagePrintTimer::Run() (in XUL) + 186
    #20 0x106e109db in nsThread::ProcessNextEvent(bool, bool*) (in XUL) + 2139
    #21 0x106d51a3e in NS_ProcessPendingEvents_P(nsIThread*, unsigned int) (in XUL) + 254
    #22 0x1062dc1c3 in nsBaseAppShell::NativeEventCallback() (in XUL) + 451
    #23 0x10625901a in nsAppShell::ProcessGeckoEvents(void*) (in XUL) + 490
    #24 0x7fff85bfc100 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (in CoreFoundation) + 16
    #25 0x7fff85bfbaec in __CFRunLoopDoSources0 (in CoreFoundation) + 444
    #26 0x7fff85c1edc4 in __CFRunLoopRun (in CoreFoundation) + 788
    #27 0x7fff85c1e6b1 in CFRunLoopRunSpecific (in CoreFoundation) + 289
    #28 0x7fff8295a0a3 in RunCurrentEventLoopInMode (in HIToolbox) + 208
    #29 0x7fff82959d83 in ReceiveNextEventCommon (in HIToolbox) + 165
    #30 0x7fff82959cd2 in BlockUntilNextEventMatchingListInMode (in HIToolbox) + 61
    #31 0x7fff88131612 in _DPSNextEvent (in AppKit) + 684
    #32 0x7fff88130ed1 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 127
    #33 0x106257865 in -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in XUL) + 245
    #34 0x7fff88128282 in -[NSApplication run] (in AppKit) + 516
    #35 0x106259bf9 in nsAppShell::Run() (in XUL) + 185
    #36 0x105e0c587 in nsAppStartup::Run() (in XUL) + 311
    #37 0x103be309f in XREMain::XRE_mainRun() (in XUL) + 4287
    #38 0x103be4057 in XREMain::XRE_main(int, char**, nsXREAppData const*) (in XUL) + 599
    #39 0x103be4512 in XRE_main (in XUL) + 146
    #40 0x1000028a8 in 0x2000028a8
    #41 0x100001abd in 0x200001abd
    #42 0x100001043 in 0x200001043
    #43 0x0 in 0x0000000100000000 (in firefox-bin)
0x0001531da040 is located 0 bytes inside of 11007-byte region [0x0001531da040,0x0001531dcb3f)
freed by thread T0 here:
    #0 0x10000ef88 in 0x20000ef88
    #1 0x10000e602 in 0x20000e602
    #2 0x106f1a54e in gfxAlphaBoxBlur::~gfxAlphaBoxBlur() (in XUL) + 78
    #3 0x104301c27 in nsCSSRendering::PaintBoxShadowOuter(nsPresContext*, nsRenderingContext&, nsIFrame*, nsRect const&, nsRect const&) (in XUL) + 3911
    #4 0x10435a467 in nsDisplayBoxShadowOuter::Paint(nsDisplayListBuilder*, nsRenderingContext*) (in XUL) + 535
    #5 0x10429904f in mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*) (in XUL) + 3263
    #6 0x106fd17e3 in mozilla::layers::BasicThebesLayer::PaintThebes(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) (in XUL) + 3635
    #7 0x106fb59f6 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintContext&, gfxContext*) (in XUL) + 1142
    #8 0x106fb520e in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) (in XUL) + 3806
    #9 0x106fb5881 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintContext&, gfxContext*) (in XUL) + 769
    #10 0x106fb520e in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) (in XUL) + 3806
    #11 0x106fb5881 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintContext&, gfxContext*) (in XUL) + 769
    #12 0x106fb520e in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) (in XUL) + 3806
    #13 0x106fb33e6 in mozilla::layers::BasicLayerManager::EndTransactionInternal(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) (in XUL) + 2454
    #14 0x104350d66 in nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) const (in XUL) + 2582
previously allocated by thread T0 here:
    #0 0x10000ed7c in 0x20000ed7c
    #1 0x7fff8bc26152 in malloc_zone_malloc (in libsystem_c.dylib) + 70
    #2 0x7fff8bc26ba6 in malloc (in libsystem_c.dylib) + 40
    #3 0x7fff8a04a346 in operator new(unsigned long) (in libstdc++..dylib) + 33
    #4 0x7fff8a04a3da in operator new[](unsigned long) (in libstdc++..dylib) + 8
    #5 0x1077597c7 in mozilla::gfx::AlphaBoxBlur::AlphaBoxBlur(mozilla::gfx::Rect const&, mozilla::gfx::IntSize const&, mozilla::gfx::IntSize const&, mozilla::gfx::Rect const*, mozilla::gfx::Rect const*) (in XUL) + 3223
    #6 0x106f1a9d7 in gfxAlphaBoxBlur::Init(gfxRect const&, nsIntSize const&, nsIntSize const&, gfxRect const*, gfxRect const*) (in XUL) + 1111
Shadow byte and word:
  0x10002a63b408: fd
  0x10002a63b408: fd fd fd fd fd fd fd fd
long double restrictunsigned __int128::* shadow bytes:
  0x10002a63b3e8: fa fa fa fa fa fa fa fa
  0x10002a63b3f0: fa fa fa fa fa fa fa fa
  0x10002a63b3f8: fa fa fa fa fa fa fa fa
  0x10002a63b400: fa fa fa fa fa fa fa fa
=>0x10002a63b408: fd fd fd fd fd fd fd fd
  0x10002a63b410: fd fd fd fd fd fd fd fd
  0x10002a63b418: fd fd fd fd fd fd fd fd
  0x10002a63b420: fd fd fd fd fd fd fd fd
  0x10002a63b428: fd fd fd fd fd fd fd fd
Stats: 710M malloced (502M for red zones) by 1042005 calls
Stats: 117M realloced by 40876 calls
Stats: 659M freed by 805793 calls
Stats: 612M really freed by 715607 calls
Stats: 363M (92982 full pages) mmaped in 633 calls
  mmaps   by size class: 7:274365; 8:106444; 9:20460; 10:9709; 11:9435; 12:3072; 13:2496; 14:1024; 15:896; 16:704; 17:456; 18:38; 19:38; 20:19; 21:4; 22:4; 24:1;
  mallocs by size class: 7:634097; 8:234135; 9:57662; 10:47143; 11:36566; 12:11018; 13:8667; 14:4174; 15:3747; 16:2622; 17:1913; 18:138; 19:72; 20:31; 21:8; 22:6; 24:6;
  frees   by size class: 7:483126; 8:171383; 9:44391; 10:42882; 11:34588; 12:9544; 13:7997; 14:3776; 15:3574; 16:2423; 17:1880; 18:119; 19:65; 20:28; 21:7; 22:6; 24:6;
  rfrees  by size class: 7:421965; 8:151304; 9:40346; 10:40605; 11:33520; 12:8914; 13:7737; 14:3592; 15:3408; 16:2157; 17:1834; 18:115; 19:65; 20:28; 21:6; 22:6; 24:5;
Stats: malloc large: 8644 small slow: 14721
==4374== ABORTING

Comment 1

[Daniel Holbert [:dholbert]]

I believe heap use-after-free is generally sec-critical.

Comment 2

[Mats Palmgren (inactive)]

Attachment: wip

The problem seems to be that there's something else with a reference to
this surface, so even though we own the image data, deleting it makes
the remaining cairo surface reference have a dangling data pointer.

The value of "mImageSurface->CairoSurface()->ref_count" inside the
if-statement is 2.

I'm NOT suggesting this patch is a fix or even a wallpaper - it just
demonstrates the nature of the problem.  (It does fix the crash though.)

Note also that we have some patches on top of cairo in this area:
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/quartz-create-for-data.patch
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/quartz-get-image.patch

Comment 3

[Mats Palmgren (inactive)]

Fwiw, the crash does not occur in an ASan build on Linux.

Comment 4

[Daniel Veditz [:dveditz]]

This is crashing down in Mac OS code (Matt is using 10.8), looks like an Apple bug.

Does this happen on 10.7 also?

Since Apple's not likely to fix this any time soon we should see if we can wallpaper it.

Does this happen when you print to paper also, or only when you print to a PDF? (PDF routines are on the stack)

Comment 5

[Mats Palmgren (inactive)]

I'm on OSX 10.7.5.  I would guess it's a bug in Cairo or in how we use Cairo.
I don't think it's an Apple bug.  Clearly we shouldn't deallocate the surface
data when there's still other references to the surface.

Comment 6

[Jet Villegas (inactive)]

To Milan for reassignment.

Comment 7

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

Remember that this only crashes ASan builds. I made my own for 17+20. This is Mac OS 10.8.2.

Using builds from 2013-03-11:

17.0.4esr: no crash
20.0: no crash
21.0a2: crash
22.0a: crash

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Jeff any hunches WRT Cairo? (see comment 5).

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Mat's patch does stop the crash, and the resulting PDF looks correct.

Comment 10

[Daniel Holbert [:dholbert]]

[though see comment 2 -- Mats doesn't necessarily intend it as an actual fix]

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Right, it's an ownership issue:
* gfxAlphaBoxBlur new's and owns AlphaBoxBlur (in mBlur)
* mBlur in turn new's and owns the buffer
* gfxAlphaBoxBlur also new's and owns gfxImageSurface, created with mBlur's owned buffer
* on creation, gfxImageSurface creates cairo surface and lets it use the buffer in question
* deleting gfxAlphaBoxBlur deletes gfxImageSurface and AlphaBoxBlur
* gfxImageSurface does not delete the cairo surface unless the reference count is down to zero
* AlphaBoxBlur deletes the buffer it allocated and owns
* thus the trouble - cairo surface outlives gfxImageSurface and AlphaBoxBlur, which in turn deletes the buffer while the cairo surface may still want access to it in the case where there was another reference holder to the cairo surface

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Should we consider Mats patch for 21 while the deeper (and pretty awesome) investigation ensues?

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Change the data ownership between gfxImageSurface and AlphaBoxBlur when used by gfxAlphaBoxBlur

This is a bit of a larger change, but perhaps appropriate.
gfxAlphaBoxBlur is the one that takes the data owned by AlphaBoxBlur and lets gfxImageSurface use it, but doesn't make sure the data is still around as long as it is needed.  The approach in this patch is to take the ownership of the data away from AlphaBoxBlur right away, and give it to gfxImageSurface instead, and do it on creation, rather than on deletion.
* Add AlphaBoxBlur::GetDataWithOwnership() method which returns the data and relinquishes the ownership of it.  We still point to it, but we don't own it anymore.  Mats' approach is absolutely valid, with an explicit "own / don't own", but this one felt a bit more explicit and perhaps more difficult to abuse.  Would like to know what you think.
* Add an extra parameter to the gfxImageSurface constructor (with the data) which explicitly specifies who owns the data passed in the constructor.  The default is false (the caller still owns it), but we now have a way of passing the data ownership at construction time.
* Have gfxAlphaBoxBlur put it all together.

I have not reviewed the rest of the users of gfxImageSurface to see if we're getting into this kind of trouble in other places.

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 727849 [details] [diff] [review]
Change the data ownership between gfxImageSurface and AlphaBoxBlur when used by gfxAlphaBoxBlur

I think this is the right approach.

> gfx/2d/Blur.cpp
>+AlphaBoxBlur::GetDataWithOwnership()
>+{
>+  if (mFreeData) {
>+      mFreeData = false;

Indentation in this file is 2 spaces.

Please add mode-lines to the files that miss them while you're here.
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line


>+	/**
>+   * As GetData() but only return the pointer if this class owns the data.
>+   * At the same time, reset the ownership - the caller is now responsible

indentation


>gfx/thebes/gfxBlur.cpp
> gfxAlphaBoxBlur::~gfxAlphaBoxBlur()
> {
>   // Drop references to mContext and mImageSurface before we delete mBlur,
>   // because the image surface points to data in mBlur.
>   mContext = nullptr;
>+  delete mBlur;
>   mImageSurface = nullptr;
>-  delete mBlur;
> }

Is there a reason for this change?  I suspect that if there are cases
where mBlur owns the data we want to keep the current order (but see
below). (and note the comment is wrong with your change)


>     mBlur = new AlphaBoxBlur(rect, spreadRadius, blurRadius, dirtyRect, skipRect);
> 
>-    unsigned char* data = mBlur->GetData();
>+    // We will give the ownership of the mBlur data to mImageSurface
>+    unsigned char* data = mBlur->GetDataWithOwnership();

It's OK, but I wonder why the AlphaBoxBlur constructor should do the
allocation since we immediately take ownership.  Are there any situations
where AlphaBoxBlur actually owns the data after this change?  If not,
then we might as well remove mFreeData and associated code if add a new
AlphaBoxBlur method that allocates the data and use that here.


>gfx/thebes/gfxImageSurface.h
>     /**
>      * Construct an image surface around an existing buffer of image data.
>      * @param aData A buffer containing the image data
>      * @param aSize The size of the buffer
>      * @param aStride The stride of the buffer
>      * @param format Format of the data
>+     * @param aOwnsData If true, assume aData ownership passes to this class.
>      *
>      * @see gfxImageFormat
>      */
>     gfxImageSurface(unsigned char *aData, const gfxIntSize& aSize,
>-                    long aStride, gfxImageFormat aFormat);
>+                    long aStride, gfxImageFormat aFormat,
>+                    bool aOwnsData=false);

I don't like bool params because it makes the call sites hard to read
and error prone.  Please change it to "uint32_t aFlags" and add a enum
or const value for the flag.  (spaces around the '=' please)


>     void InitWithData(unsigned char *aData, const gfxIntSize& aSize,
>-                      long aStride, gfxImageFormat aFormat);
>+                      long aStride, gfxImageFormat aFormat, bool aOwnsData);

same

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Mats Palmgren [:mats] from comment #14)
> Comment on attachment 727849 [details] [diff] [review]
> ...
> 
> >gfx/thebes/gfxBlur.cpp
> > gfxAlphaBoxBlur::~gfxAlphaBoxBlur()
> > {
> >   // Drop references to mContext and mImageSurface before we delete mBlur,
> >   // because the image surface points to data in mBlur.
> >   mContext = nullptr;
> >+  delete mBlur;
> >   mImageSurface = nullptr;
> >-  delete mBlur;
> > }
> 
> Is there a reason for this change?  I suspect that if there are cases
> where mBlur owns the data we want to keep the current order (but see
> below). (and note the comment is wrong with your change)

Will change the comment for sure.  In the gfxAlphaBoxBlur scenario, mBlur never owns the data, and it doesn't really matter which one goes away first, but it felt more comfortable to not have mBlur around for an extra line of code pointing to that (potentially) deleted data, even though it wouldn't do anything with it.

> 
> >     mBlur = new AlphaBoxBlur(rect, spreadRadius, blurRadius, dirtyRect, skipRect);
> > 
> >-    unsigned char* data = mBlur->GetData();
> >+    // We will give the ownership of the mBlur data to mImageSurface
> >+    unsigned char* data = mBlur->GetDataWithOwnership();
> 
> It's OK, but I wonder why the AlphaBoxBlur constructor should do the
> allocation since we immediately take ownership.  Are there any situations
> where AlphaBoxBlur actually owns the data after this change?  If not,
> then we might as well remove mFreeData and associated code if add a new
> AlphaBoxBlur method that allocates the data and use that here.

You're right.  The cleanest thing here would be for us to allocate the data and pass it to AlphaBoxBlur in a another constructor.  That way it's clear where the ownership of the data lies and we don't even need the "give me the data ownership" method.  There is a logic in figuring out the size of the data that's currently buried in the existing constructor, but that may be nicely wrapped in a static method.

The other comments are clear, thanks.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Mats Palmgren [:mats] from comment #14)
> Comment on attachment 727849 [details] [diff] [review]
> 
> >gfx/thebes/gfxImageSurface.h
> ...
> >     gfxImageSurface(unsigned char *aData, const gfxIntSize& aSize,
> >-                    long aStride, gfxImageFormat aFormat);
> >+                    long aStride, gfxImageFormat aFormat,
> >+                    bool aOwnsData=false);
> 
> I don't like bool params because it makes the call sites hard to read
> and error prone.  Please change it to "uint32_t aFlags" and add a enum
> or const value for the flag.  (spaces around the '=' please)

This class internally holds a bool for "own data" - would you change that to an enum as well?  Or just as the method parameter?

Comment 17

[Mats Palmgren (inactive)]

No, just the method parameter.

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

AlphaBoxBlur uses uint8_t array, gfxImageSurface uses unsigned char array.  I will not attempt to resolve that inconsistency.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: gfxImageSurface constructor lets us specify if it should own the data buffer specified.

I think there are still inconsistencies on how the mData is handled in gfxImageSurface, but that's probably a separate bug. For example, MakeInvalid(), and tracking the memory usage when the data is owned.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 2/2: AlphaBoxBlur does not hold to the data at all, but expects the callers to manage it and specify it on each Blur() call.

AlphaBoxBlur does not hold the data, gfxAlphaBoxBlur allocates it, and passes it to gfxImageSurface to manage. 
There is still a bit a annoyance of of a shared assumption for the extra 3 bytes required by blur that nobody tracks.  Perhaps gfxImageSurface should track the "capacity" in addition to the "size".

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 729230 [details] [diff] [review]
gfxImageSurface constructor lets us specify if it should own the data buffer specified.

Looks good to me, r=mats.  I'm not a gfx peer though so you need an additional r+ from someone who is.

Comment 22

[Mats Palmgren (inactive)]

Comment on attachment 729235 [details] [diff] [review]
Part 2/2: AlphaBoxBlur does not hold to the data at all, but expects the callers to manage it and specify it on each Blur() call.

>gfx/2d/Blur.cpp
>+   mMinDataSize(-1)

You forgot to declare mMinDataSize in the header?


>+    mMinDataSize(aRect.width*aRect.height)

I think you need to use CheckedInt<int32_t> for the result of that
multiplication and if it's !isValid() assign -1.


>+int32_t
>+AlphaBoxBlur::GetMinDataSize() const
>+{
>+  return mMinDataSize;
>+}

I think you should put this method in the header, unless there's
a reason not to.


> void
>+AlphaBoxBlur::Blur(uint8_t* aData)
> [...]
>+  return true;
> }

Stray statement? (this method is void)


>gfx/2d/Blur.h
>+   * Return the minimum buffer size that should be given to Blur() method.  If
>+   * negative, the class is not properly setup for blurring.  Note that this
>+   * includes the extra three bytes on top of the stride*width, where something
>+   * like gfxImageSurface::GetDataSize() would report without it, even if it 
>+   * happens to have the extra bytes.
>    */
>+  int32_t GetMinDataSize() const;

Perhaps naming it GetSurfaceAllocationSize or something to make it
distinct from "DataSize" would be clearer?


>+  /**
>+   * Perform the blur in-place on the surface backed by specified 8-bit
>+   * alpha surface data. The size must be at least that returned by
>+   * GetMinDataSize() or bad things will happen.
>+   */
>+  void Blur(uint8_t* aData);

Also mention that aData is expected to be zero-filled by the caller?


>gfx/thebes/gfxBlur.cpp
>     mBlur = new AlphaBoxBlur(rect, spreadRadius, blurRadius, dirtyRect, skipRect);
>+    int32_t blurDataSize = mBlur->GetMinDataSize();
> 
>-    unsigned char* data = mBlur->GetData();
>+    // Allocate and null out the buffer for the data.
>+    uint8_t* data = nullptr;
>+    if (blurDataSize > 0) {
>+        data = new uint8_t[blurDataSize];
>+    }
>     if (!data)
>-      return nullptr;
>+        return nullptr;
>+    memset(data, 0, blurDataSize);

Yeah, that works, but would be simpler as:

    int32_t blurDataSize = mBlur->GetMinDataSize();
    if (blurDataSize < 0)
        return nullptr;
    data = new uint8_t[blurDataSize];
    memset(data, 0, blurDataSize);

since the 'new' allocation is infallible.


>+    // Need to pass the data buffer to mBlur
>+    mBlur->Blur(mImageSurface->Data());

No need for that comment IMO, the code is obvious.


r=mats with those nits fixed, but please also get r+ from a gfx peer.

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Mats Palmgren [:mats] from comment #22)
> Comment on attachment 729235 [details] [diff] [review]
> ...
> 
> >+int32_t
> >+AlphaBoxBlur::GetMinDataSize() const
> >+{
> >+  return mMinDataSize;
> >+}
> 
> I think you should put this method in the header, unless there's
> a reason not to.

I put in in C++ as that seems to be the prevalent approach in this class for all the other accessors.


> ...
> >+  /**
> >+   * Perform the blur in-place on the surface backed by specified 8-bit
> >+   * alpha surface data. The size must be at least that returned by
> >+   * GetMinDataSize() or bad things will happen.
> >+   */
> >+  void Blur(uint8_t* aData);
> 
> Also mention that aData is expected to be zero-filled by the caller?

I know this was being done in the constructor, but was Blur only "called once"?  At the first glance I thought we were zeroing it out mostly for the extra bits that the Cairo surface wouldn't fill, but I only glanced at it once :-)

I'll fix the rest of the stuff and resubmit to a gfx peer.

Comment 24

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 2/2: AlphaBoxBlur does not hold to the data at all, but expects the callers to manage it and specify it on each Blur() call.

Addressed Mats comments.

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 729621 [details] [diff] [review]
Part 2/2: AlphaBoxBlur does not hold to the data at all, but expects the callers to manage it and specify it on each Blur() call.

Already had r=mpalmgren

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: gfxImageSurface constructor lets us specify if it should own the data buffer specified.

Fixed an issue in Windows build, otherwise r=matspal.

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

The try run for it: https://tbpl.mozilla.org/?tree=Try&rev=97552c0d9c86

Comment 28

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Add another constructor instead, allowing for the extra memory allocation

This would obsolete the previous patch, but I won't remove it so that we can compare the two.  Jeff, is this a preferable way?  This way we allocate the memory inside of the gfxImageSurface, it has the ownership, no passing of the allocated memory ownership between classes.  If so, I'll prepare the second part that matches.

Comment 29

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 734011 [details] [diff] [review]
Add another constructor instead, allowing for the extra memory allocation

It's really feedback, not review at this point.

Comment 30

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Add gfxImageSurface constructor that allocates more space than needed, change AlphaBoxBlur to not hold on to the data and let the callers manage it.

Go with the approach where gfxImageSurface allocates the data it owns, as hinted by Jeff. AlphaBoxBlur operates on the data, doesn't hold on to it.

Comment 31

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Add gfxImageSurface constructor that allocates more space than needed, change AlphaBoxBlur to not hold on to the data and let the callers manage it.

The try run https://tbpl.mozilla.org/?tree=Try&rev=a4f07b5db07e

Comment 32

[bhavana bajaj [:bajaj]]

I see this is marked as a regression but does not block anything.Do we know the feature or bug# that regressed this ? Adding :mwobensmith as QA to help with that.

:Milan : Given we have gone to build with beta 4 for Fx21, what are the next best steps here  ? If you are already aware of the regressing bug , please let us know if a backout is possible as Fx 20 is unaffected ?

Comment 33

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

Unfortunately, because it's an ASan-only crash - for me - finding a regression window is extremely hard, as I don't have the range of ASan builds that I'd have with opt/debug builds.

How important is an exact regression window? Or can the questions you need answered be satisfied by doing something else?

Comment 34

[Daniel Holbert [:dholbert]]

(In reply to Matt Wobensmith from comment #33)
> I don't have the range of ASan
> builds that I'd have with opt/debug builds.

I think we publish nightlies built with ASAN available somewhere (decoder makes them, IIRC). I don't recall where they live, though.

Comment 35

[Daniel Holbert [:dholbert]]

Sorry, never mind -- currently our nightly mac ASAN builds are here:
   http://people.mozilla.org/~choller/firefox/asan/
and unfortunately we only have a few weeks' worth of them. (Nothing from around when/before this bug was filed.)

So: disregard comment 34.

Comment 36

[Mats Palmgren (inactive)]

I don't think the exact regression window is essential.
We just need to get this reviewed and landed.

Comment 37

[Milan Sreckovic [:milan] (needinfo for best results)]

Jeff'll get the review done today.

Comment 38

[Alex Keybl [:akeybl]]

(In reply to Milan Sreckovic [:milan] from comment #37)
> Jeff'll get the review done today.

Awesome! Would be great to get this landed on m-c (and then uplifted Monday of next week) to prevent a sec-critical regression in FF21.

Comment 39

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 739507 [details] [diff] [review]
Add gfxImageSurface constructor that allocates more space than needed, change AlphaBoxBlur to not hold on to the data and let the callers manage it.

Review of attachment 739507 [details] [diff] [review]:
-----------------------------------------------------------------

It's not beautiful, but it's better to be secure. I do like that Blur doesn't have a member for the data anymore.

Comment 40

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 739507 [details] [diff] [review]
Add gfxImageSurface constructor that allocates more space than needed, change AlphaBoxBlur to not hold on to the data and let the callers manage it.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I don't think easily.  If anything, the patch may point to buffer overflow issue, where it's really use after free.

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?

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be very similar if not exactly the same.  They haven't been prepared pending decision to use.

How likely is this patch to cause regressions; how much testing does it need?
Should be safe.

Comment 41

[Milan Sreckovic [:milan] (needinfo for best results)]

I added checkin-needed, but note that it should wait for sec-approval first.

Comment 42

[Milan Sreckovic [:milan] (needinfo for best results)]

Just in case, remove checkin-needed until sec-approval is granted.  If granted, whoever does that, could you set checkin-needed to expedite the process?

Comment 43

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

Comment on attachment 739507 [details] [diff] [review]
Add gfxImageSurface constructor that allocates more space than needed, change AlphaBoxBlur to not hold on to the data and let the callers manage it.

Sec-approval+ (which applies to m-c). Please nominate the patch for Aurora and Beta after/while you checkin so we can not ship this security problem publicly.

Comment 44

[Daniel Holbert [:dholbert]]

Adding back checkin-needed, & needinfo on milan for aurora/beta approval nominations.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c37b537e562

Comment 46

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 739507 [details] [diff] [review]
Add gfxImageSurface constructor that allocates more space than needed, change AlphaBoxBlur to not hold on to the data and let the callers manage it.

Should this be uplifted to aurora and beta?  The patches apply with some trivial modifications.

Comment 47

[bhavana bajaj [:bajaj]]

(In reply to Milan Sreckovic [:milan] from comment #46)
> Comment on attachment 739507 [details] [diff] [review]
> Add gfxImageSurface constructor that allocates more space than needed,
> change AlphaBoxBlur to not hold on to the data and let the callers manage it.
> 
> Should this be uplifted to aurora and beta?  The patches apply with some
> trivial modifications.

I am approving this on branches given comment# 38 &#43 and keeping in mind the patch is safe.If there is a concern on landing this in our second last beta we should discuss that and see how we can mitigate it.

Comment 48

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0c37b537e562

Comment 49

[Robert Kaiser]

Just out of interest, does this have any influence on bug 829954?

Comment 50

[Milan Sreckovic [:milan] (needinfo for best results)]

Check-in needed on the aurora and beta uplift.  The patches for the trunk should apply cleanly, but let me know if you get into trouble.

Comment 51

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Robert Kaiser (:[e-mail]) from comment #49)
> Just out of interest, does this have any influence on bug 829954?

This bug is "early release of allocated memory" where bug 829954 is "can't allocate memory", so probably not.

Comment 52

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #50)
> Check-in needed on the aurora and beta uplift.  The patches for the trunk
> should apply cleanly, but let me know if you get into trouble.

Hold off on uplift, there is an overflow protection that wasn't working properly here.

Comment 53

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Correct way to use checked int

The original code didn't have the overflow protection, the new code looked like it did, but it really didn't, because of how the code was written.  This fixes that.

Comment 54

[Robert Kaiser]

(In reply to Milan Sreckovic [:milan] from comment #51)
> (In reply to Robert Kaiser (:[e-mail]) from comment #49)
> > Just out of interest, does this have any influence on bug 829954?
> 
> This bug is "early release of allocated memory" where bug 829954 is "can't
> allocate memory", so probably not.

OK, just wondered because this patch contains "allocate" and "AlphaBoxBlur" in the description and that other bug talks about using fallible instead of infallible allocation in that area. If it's not related, we're all fine I guess. :)

Comment 55

[Jeff Muizelaar [:jrmuizel]]

(In reply to Milan Sreckovic [:milan] from comment #53)
> Created attachment 743790 [details] [diff] [review]
> Correct way to use checked int
> 
> The original code didn't have the overflow protection, the new code looked
> like it did, but it really didn't, because of how the code was written. 
> This fixes that.

I've landed this as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa4d8053055d

Comment 56

[Jeff Walden [:Waldo]]

I saw attachment 743790 [details] [diff] [review]'s push in IRC.  This error looks like a super-easy case for static analysis to detect and flag; I filed bug 867348 on adding such an analysis, hidden for now as this bug's hidden.

Comment 57

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Correct way to use checked int

Here's the patch that Jeff landed.

Comment 58

[Milan Sreckovic [:milan] (needinfo for best results)]

Please check this into aurora and beta, making sure both patches are taken (the second one goes on top of the first one).

Comment 59

[Joe Drew (not getting mail)]

Note also that there's a typo in the patch, so it might be best to just transplant from mozilla-central.

Comment 60

[Joe Drew (not getting mail)]

Oh, ignore me: comment 57 handles this.

Comment 61

[bhavana bajaj [:bajaj]]

Given :RyanVm is afk, any volunteers who can help land this asap on branches(mozilla-beta as priority) as we need to go-to-build ?  Please.. O:)

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e0f84d427e27
https://hg.mozilla.org/releases/mozilla-aurora/rev/840ca31cb3df

https://hg.mozilla.org/releases/mozilla-beta/rev/201dab279504
https://hg.mozilla.org/releases/mozilla-beta/rev/c58832c7069e

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa4d8053055d