794463 - Crash [@ nsCanvasRenderingContext2DAzure::FillRect ]

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Bob Clary [:bc] (inactive)]

1. http://mob.novaskin.me/editor
2. Crash Nightly all platforms.

nsCanvasRenderingContext2DAzure::FillRect(double, double, double, double) mozilla::dom::CanvasRenderingContext2DBinding::fillRect mozilla::dom::CanvasRenderingContext2DBinding::genericMethod js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct)

On Windows, breakpad's exploitable does not rate this as exploitable but Microsoft's !exploitable rates this as probably exploitable since the data from the faulting address is used as the target for a branch, .i.e. the address is under the control of the attacker.

Found regression between 20120910004817-20120910183953
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1cb30394aa56&tochange=96287ad60bef
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/09/2012-09-10-mozilla-central-debug/firefox-18.0a1.en-US.debug-linux-i686.tar.bz2
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/09/2012-09-11-mozilla-central-debug/firefox-18.0a1.en-US.debug-linux-i686.tar.bz2

more ionmonkey? should be in js engine?

Comment 1

[Daniel Veditz [:dveditz]]

Would be helpful to include "!exploitable -v" output in the bug if you have it.

Don't really see any obvious IonMonkey connection in that regression range (beyond turning it off on mobile/ARM). Would probably be helpful to narrow it further on the inbound branch before those builds go away.

I don't see them in the push, but given Azure and DOM bindings on the stack I'm cc'ing Bas and bz.

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: exploitable -v output

Comment 3

[Boris Zbarsky [:bzbarsky]]

The regression range I get here (with Mac m-c nightlies) is: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=96287ad60bef&tochange=6e78c3efd115

This range in fact includes the ion merge.

It also includes at least three DOM binding changes: bug 787543, bug 787554 and bug 789636.

The actual crash is in nsCanvasRenderingContext2DAzure::FillRect because state.patternStyles[STYLE_FILL].mRawPtr->mSurface is null.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Also a canvas change in bug 723853.  And another in bug 673378.

Comment 5

[Boris Zbarsky [:bzbarsky]]

(gdb) p *state.patternStyles[STYLE_FILL].mRawPtr
$3 = (nsCanvasPatternAzure) {
  <nsIDOMCanvasPattern> = {
    <nsISupports> = {
      _vptr$nsISupports = 0x1065d8c80
    }, <No data fields>}, 
  members of nsCanvasPatternAzure: 
  mRefCnt = {
    mValue = 2
  }, 
  _mOwningThread = {
    mThread = 0x100563070
  }, 
  mSurface = {
    ptr = 0x0
  }, 
  mRepeat = nsCanvasPatternAzure::REPEAT, 
  mPrincipal = {
    mRawPtr = 0x11ae19e80
  }, 
  mForceWriteOnly = false, 
  mCORSUsed = false
}

Comment 6

[Boris Zbarsky [:bzbarsky]]

Turning off ion does NOT help.  I still get the crash.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I haven't finished bisecting, but the small range I'm down to includes bug 591358.  And I think that bug could well have caused this problem.  It makes mTarget be null in all sorts of cases when it didn't use to be before.  And the code in nsCanvasRenderingContext2DAzure::CreatePattern looks like this:

    if (srcCanvas) {
      // This might not be an Azure canvas!
      RefPtr<SourceSurface> srcSurf = srcCanvas->GetSurfaceSnapshot();
      nsRefPtr<nsCanvasPatternAzure> pat =
        new nsCanvasPatternAzure(srcSurf, repeatMode, htmlElement->NodePrincipal(), 
                                 canvas->IsWriteOnly(), false);

and the implementation of GetSurfaceSnapshot() is:

  mozilla::TemporaryRef<mozilla::gfx::SourceSurface> GetSurfaceSnapshot()
  { return mTarget ? mTarget->Snapshot() : nullptr; }

So if mTarget is null, we create a pattern with a null surface, and later crash on that null deref.

We need to either EnsureTarget() or something in GetSurfaceSnapshot() or allow mSurface to be null in patterns and fix pattern consumers.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, bisect done.  As expected:
The first bad revision is:
changeset:   104754:71d1333ca77f
user:        Anthony Jones <[e-mail]>
date:        Thu May 31 12:47:27 2012 +1200
summary:     Bug 591358 - Part 3: Lazy creation of the draw target in order to save memory and improve performance. r=roc

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fixed nullptr problem

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Crash test for clean canvas pattern

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f5390b6970e

(I folded the test in with the patch)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3f5390b6970e