Closed Bug 883201 Opened 6 years ago Closed 6 years ago

[Skia] SourceSurfaceSkia::GetData() frees memory before use

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: gal, Assigned: snorp)

Details

Attachments

(1 file, 1 obsolete file)

unsigned char*
SourceSurfaceSkia::GetData()
{
  mBitmap.lockPixels();
  unsigned char *pixels = (unsigned char *)mBitmap.getPixels();
  mBitmap.unlockPixels();
  return pixels;

}

We have to do mBitmap.unlockPixels() in the destructor if we did lockPixels here. I think the current code basically instantly frees the pixels again. This is probably exploitable but since we don't have this enabled in production anywhere I won't hide this bug.
Assignee: nobody → snorp
Comment on attachment 763582 [details] [diff] [review]
Make SourceSurfaceSkia::GetData keep pixels alive as necessary

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

::: gfx/2d/SourceSurfaceSkia.cpp
@@ +85,5 @@
>  
>  unsigned char*
>  SourceSurfaceSkia::GetData()
>  {
> +  if (mDrawTarget && !mLocked) {

This breaks if mDrawTarget == NULL (for example via MarkIndependent). You don't lock pixels and do getPixels(). I don't think you need to check for mDrawTarget here. Just remove that and it should be ok.
(In reply to Andreas Gal :gal from comment #2)
> Comment on attachment 763582 [details] [diff] [review]
> Make SourceSurfaceSkia::GetData keep pixels alive as necessary
> 
> Review of attachment 763582 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/SourceSurfaceSkia.cpp
> @@ +85,5 @@
> >  
> >  unsigned char*
> >  SourceSurfaceSkia::GetData()
> >  {
> > +  if (mDrawTarget && !mLocked) {
> 
> This breaks if mDrawTarget == NULL (for example via MarkIndependent). You
> don't lock pixels and do getPixels(). I don't think you need to check for
> mDrawTarget here. Just remove that and it should be ok.

Oh, hmm. Indeed. Not sure what I was thinking there. Fixed.
Attachment #763582 - Attachment is obsolete: true
Attachment #763582 - Flags: review?(gwright)
Comment on attachment 763763 [details] [diff] [review]
Make SourceSurfaceSkia::GetData keep pixels alive as necessary

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

I'm confused as to how long the GetData() is supposed to last.  I do agree that the original code looked like it had a problem in that it was unlocking, then returning the data, but I'm not sure that we need a separate flag to tell us we added a lock, and to stop us from locking it again, given that this is a counted lock.  Or at least that's what it looks like at a glance.

::: gfx/2d/SourceSurfaceSkia.cpp
@@ +20,5 @@
>  }
>  
>  SourceSurfaceSkia::~SourceSurfaceSkia()
>  {
> +  MaybeUnlock();

Not certain this is necessary, though perhaps it makes the code easier to read and understand.  This should call mBitmap destructor, which will force the "unlock" anyway.

@@ +99,5 @@
>  SourceSurfaceSkia::DrawTargetWillChange()
>  {
>    if (mDrawTarget) {
> +    MaybeUnlock();
> +

mBitmap.reset() will call SkBitmap::freePixels() which will force unlock.
Comment on attachment 763763 [details] [diff] [review]
Make SourceSurfaceSkia::GetData keep pixels alive as necessary

Ok, cool. I have studied this code quite a bit, so I feel lucky enough for r=me here.
Attachment #763763 - Flags: review?(gwright) → review+
Whats the advantage of not having the lock flag?
Comment on attachment 763763 [details] [diff] [review]
Make SourceSurfaceSkia::GetData keep pixels alive as necessary

Lets hold the review pending discussion with Milan.
Attachment #763763 - Flags: review+ → review?
(In reply to Milan Sreckovic [:milan] from comment #5)
> Comment on attachment 763763 [details] [diff] [review]
> Make SourceSurfaceSkia::GetData keep pixels alive as necessary
> 
> Review of attachment 763763 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm confused as to how long the GetData() is supposed to last.  I do agree
> that the original code looked like it had a problem in that it was
> unlocking, then returning the data, but I'm not sure that we need a separate
> flag to tell us we added a lock, and to stop us from locking it again, given
> that this is a counted lock.  Or at least that's what it looks like at a
> glance.
> 

Yeah, looks like SkBitmap::lockPixels() only actually does anything on the first lock. However, I think it would look pretty strange to be calling something that has "lock" in the name without ever unlocking it. Additionally, there is no guarantee that the current behavior will not change. The docs do explicitly state that calls to lockPixels() should be balanced with ones to unlockPixels().

> ::: gfx/2d/SourceSurfaceSkia.cpp
> @@ +20,5 @@
> >  }
> >  
> >  SourceSurfaceSkia::~SourceSurfaceSkia()
> >  {
> > +  MaybeUnlock();
> 
> Not certain this is necessary, though perhaps it makes the code easier to
> read and understand.  This should call mBitmap destructor, which will force
> the "unlock" anyway.

While true, I view it as destroying a resource that is in-use, and that is naughty.

> 
> @@ +99,5 @@
> >  SourceSurfaceSkia::DrawTargetWillChange()
> >  {
> >    if (mDrawTarget) {
> > +    MaybeUnlock();
> > +
> 
> mBitmap.reset() will call SkBitmap::freePixels() which will force unlock.

Same argument as above. Also, this behavior will not be obvious to readers of SourceSurfaceSkia, so I think it's definitely easier to understand this way.
Fair enough. I especially like the "I see lock, but I don't see unlock" argument, and given how this is currently being used, and that the code is solving a problem, I'm all good with the changes.
On a separate note, this whole conversation gets more complicated because SourceSurfaceSkia also lets you access SkBitmap (mBitmap) directly, but I have to form my thoughts better on that and it'd be another bug.
https://hg.mozilla.org/mozilla-central/rev/2a1e3764868a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Attachment #763763 - Flags: review?(gwright) → review+
Attachment #763763 - Flags: review?
You need to log in before you can comment on or make changes to this bug.