933826 - IPC: crash with PLayerTransaction::Msg_Update [@nsRegion::SetToElements]

Status: RESOLVED INCOMPLETE

(Core :: IPC, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: fuzzing-session

Tested with an opt/non-debug build of https://github.com/posidron/mozilla-central/commit/26121cb

Comment 1

[:gerard-majax]

Okay so,

#0  nsRegion::SetToElements (this=0xaf227208, aCount=<optimized out>) at ../../../../mozilla-central/gfx/src/nsRegion.cpp:305
#1  0xb4fd56da in nsRegion::SetEmpty (this=0xaf227208) at ../../../../mozilla-central/gfx/src/nsRegion.h:148

We can see that nsRegion::SetEmpty is defined as:

  146   void SetEmpty ()
  147   {
  148     SetToElements (0);
  149     mBoundRect.SetRect (0, 0, 0, 0);
  150   }

Considering that the code of SetToElements() that is called is:

   273 // Adjust the number of rectangles in region.
   274 // Content of rectangles should be changed by caller.
   275 
   276 void nsRegion::SetToElements (uint32_t aCount)
   277 {
   278   if (mRectCount < aCount)        // Add missing rectangles
   279   {
   280     uint32_t InsertCount = aCount - mRectCount;
   281     mRectCount = aCount;
   282     RgnRect* pPrev = &mRectListHead;
   283     RgnRect* pNext = mRectListHead.next;
   284 
   285     while (InsertCount--)
   286     {
   287       mCurRect = new RgnRect;
   288       mCurRect->prev = pPrev;
   289       pPrev->next = mCurRect;
   290       pPrev = mCurRect;
   291     }
   292 
   293     pPrev->next = pNext;
   294     pNext->prev = pPrev;
   295   } else
   296   if (mRectCount > aCount)        // Remove unnecessary rectangles
   297   {
   298     uint32_t RemoveCount = mRectCount - aCount;
   299     mRectCount = aCount;
   300     mCurRect = mRectListHead.next;
   301 
   302     while (RemoveCount--)
   303     {
   304       RgnRect* tmp = mCurRect;
   305       mCurRect = mCurRect->next;
   306       delete tmp;
   307     }
   308 
   309     mRectListHead.next = mCurRect;
   310     mCurRect->prev = &mRectListHead;
   311   }
   312 }

So from the header file we have aCount = 0 ; and the crash happens at line 305, meaning mRectCount was at least >= 1.

It would mean that the list mRectListHead is not in sync somehow: either its content gets outdated, or it had garbage inside.

Comment 2

[:gerard-majax]

However, the stack mentions:

#2  0xb4fd57ac in nsRegion::Copy (this=0xaf227208, aRegion=...) at ../../../../mozilla-central/gfx/src/nsRegion.cpp:585

Which, is:

   579 nsRegion& nsRegion::Copy (const nsRegion& aRegion)
   580 {
   581   if (&aRegion == this)
   582     return *this;
   583 
   584   if (aRegion.mRectCount == 0)
   585     SetEmpty ();
   586   else
   587   {
   588     SetToElements (aRegion.mRectCount);
   589 
   590     const RgnRect* pSrc = aRegion.mRectListHead.next;
   591     RgnRect* pDest = mRectListHead.next;
   592 
   593     while (pSrc != &aRegion.mRectListHead)
   594     {
   595       *pDest = *pSrc;
   596 
   597       pSrc  = pSrc->next;
   598       pDest = pDest->next;
   599     }
   600 
   601     mCurRect = mRectListHead.next;
   602     mBoundRect = aRegion.mBoundRect;
   603   }
   604 
   605   return *this;
   606 }

So I'm not sure to correctly understand what happens here ; the region that we are copying is with mRectCount = 0, and we call SetEmpty(0), but mRectCount of the nsRegion object is different.

Comment 3

[Benoit Girard (:BenWa)]

(In reply to Alexandre LISSY :gerard-majax from comment #2)
> So I'm not sure to correctly understand what happens here ; the region that
> we are copying is with mRectCount = 0, and we call SetEmpty(0), but
> mRectCount of the nsRegion object is different.

aRegion.mRectCount is zero when we're inside Copy but that doesn't mean that this->mRectCount was zero then.

Comment 4

[:gerard-majax]

(In reply to Benoit Girard (:BenWa) from comment #3)
> (In reply to Alexandre LISSY :gerard-majax from comment #2)
> > So I'm not sure to correctly understand what happens here ; the region that
> > we are copying is with mRectCount = 0, and we call SetEmpty(0), but
> > mRectCount of the nsRegion object is different.
> 
> aRegion.mRectCount is zero when we're inside Copy but that doesn't mean that
> this->mRectCount was zero then.

So it's legit ? And then it would mean the issue lies in the list.

Comment 5

[:gerard-majax]

(gdb) bt full
#0  nsRegion::SetToElements (this=0xaf227208, aCount=<optimized out>) at ../../../../mozilla-central/gfx/src/nsRegion.cpp:305
        tmp = 0x0
        RemoveCount = <optimized out>

So the list is out of sync ...

Comment 6

[Andrei Purice]

Hey Alexandre,
Does this issue still reproduce for you or can it be closed?

Comment 7

[:gerard-majax]

I have no idea, it was found by fuzzing IPC and I was not the one doing it.

Comment 8

[Andrew McCreight [:mccr8]]

I think we can close this. It is an old fuzz bug that will be hard to run again, and it looks like the prior investigating didn't come up with a concrete fix.