Closed Bug 1293598 Opened 5 years ago Closed 5 years ago

clean up pointer after free in pixman_image_fini

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: pchang, Assigned: pchang)

References

Details

Attachments

(1 file, 1 obsolete file)

There are some random address crashes from bug 1268202, some of them should be related to dereferencing the freed memory.

Set pointer as NULL in _pixman_image_fini after free would be easier to handle the "dereferencing freed memory".
Assignee: nobody → howareyou322
Attachment #8779273 - Flags: review?(jmuizelaar)
Comment on attachment 8779273 [details]
Bug 1293598 - clean up pointers after free inside pixman_image,

https://reviewboard.mozilla.org/r/70300/#review67722

Perhaps wrap these in a macro like PIXMAN_POISON and a comment about the bug that they are trying to catch so that we can easily remove them once we've caught the bug.
Attachment #8779273 - Flags: review?(jmuizelaar) → review-
Comment on attachment 8779273 [details]
Bug 1293598 - clean up pointers after free inside pixman_image,

https://reviewboard.mozilla.org/r/70300/#review68132

::: gfx/cairo/libpixman/src/pixman-image.c:50
(Diff revision 2)
> +#ifdef PIXMAN_POISON
> +    if (p) {
> +#endif
> +        free (p + offset);
> +#ifdef PIXMAN_POISON
> +        p = NULL;

This just sets the argument to NULL not the copy in the caller. I think you want to either use a macro or pass in a pointer to a pointer.

::: gfx/cairo/libpixman/src/pixman-image.c:199
(Diff revision 2)
>  		image->common.property_changed == gradient_property_changed);
>  	}
>  
> -	if (image->type == BITS && image->bits.free_me)
> -	    free (image->bits.free_me);
> +	if (image->type == BITS && image->bits.free_me) {
> +	    free_memory (image->bits.free_me);
> +	    free_memory (image->bits.bits);

Where is this extra free coming from?
Attachment #8779273 - Flags: review?(jmuizelaar) → review-
Comment on attachment 8779273 [details]
Bug 1293598 - clean up pointers after free inside pixman_image,

https://reviewboard.mozilla.org/r/70300/#review68132

> This just sets the argument to NULL not the copy in the caller. I think you want to either use a macro or pass in a pointer to a pointer.

Do you suggest to use memory poison here, just like jemalloc? Original code didn't reset the pointer as NULL after free, it should cause trobule if somewhere uses it after free. Setting the pointers to null could simpify the crash problem instead of getting random crash address or unexpected callstack. Therefore, I think it is safe to keep my first patch. How do you think?

> Where is this extra free coming from?

bits.free_me and bits.bits[1] will share the same address. Therefore, I passed it into free_memory too.
[1]https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/pixman-bits-image.c#1765
ni for comment 5.
Flags: needinfo?(jmuizelaar)
(In reply to Peter Chang[:pchang] from comment #5)
> Comment on attachment 8779273 [details]
> Bug 1293598 - clean up pointers after free inside pixman_image,
> 
> https://reviewboard.mozilla.org/r/70300/#review68132
> 
> > This just sets the argument to NULL not the copy in the caller. I think you want to either use a macro or pass in a pointer to a pointer.
> 
> Do you suggest to use memory poison here, just like jemalloc? Original code
> didn't reset the pointer as NULL after free, it should cause trobule if
> somewhere uses it after free. Setting the pointers to null could simpify the
> crash problem instead of getting random crash address or unexpected
> callstack. Therefore, I think it is safe to keep my first patch. How do you
> think?

Sorry, for the delay. I was away on vacation.

Your patch doesn't work:

free_memory (common->transform);

will not set common->transform to null.
Flags: needinfo?(jmuizelaar)
Comment on attachment 8779273 [details]
Bug 1293598 - clean up pointers after free inside pixman_image,

https://reviewboard.mozilla.org/r/70300/#review76828

::: gfx/cairo/libpixman/src/pixman-image.c:56
(Diff revision 2)
> +    }
> +#endif
> +}
> +
> +static void
> +free_memory (void*p)

If you pass common->transform via free_memory, it will be set as NULL if it contains value.
(In reply to Peter Chang[:pchang] from comment #8)
> Comment on attachment 8779273 [details]
> Bug 1293598 - clean up pointers after free inside pixman_image,
> 
> https://reviewboard.mozilla.org/r/70300/#review76828
> 
> ::: gfx/cairo/libpixman/src/pixman-image.c:56
> (Diff revision 2)
> > +    }
> > +#endif
> > +}
> > +
> > +static void
> > +free_memory (void*p)
> 
> If you pass common->transform via free_memory, it will be set as NULL if it
> contains value.

I don't understand what you mean here.

Imagine:
 common->transform = 0xababab;

calling free_memory(common->transform);

is equivalent to:
free_memory(0xababab);

inside free_memory the local variable p is assigned 0xababab
p is then made NULL.

The value of common->transform never changes.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> (In reply to Peter Chang[:pchang] from comment #8)
> > Comment on attachment 8779273 [details]
> > Bug 1293598 - clean up pointers after free inside pixman_image,
> > 
> > https://reviewboard.mozilla.org/r/70300/#review76828
> > 
> > ::: gfx/cairo/libpixman/src/pixman-image.c:56
> > (Diff revision 2)
> > > +    }
> > > +#endif
> > > +}
> > > +
> > > +static void
> > > +free_memory (void*p)
> > 
> > If you pass common->transform via free_memory, it will be set as NULL if it
> > contains value.
> 
> I don't understand what you mean here.
> 
> Imagine:
>  common->transform = 0xababab;
> 
> calling free_memory(common->transform);
> 
> is equivalent to:
> free_memory(0xababab);
> 
> inside free_memory the local variable p is assigned 0xababab
> p is then made NULL.
> 
> The value of common->transform never changes.
Oh...you are right. I should use "pass by pointer to pointer".
Is 50 also affected? If it is you may want to request uplift.
Flags: needinfo?(howareyou322)
Comment on attachment 8779273 [details]
Bug 1293598 - clean up pointers after free inside pixman_image,

https://reviewboard.mozilla.org/r/70300/#review77392

::: gfx/cairo/libpixman/src/pixman-image.c:199
(Diff revision 3)
>  		image->common.property_changed == gradient_property_changed);
>  	}
>  
> -	if (image->type == BITS && image->bits.free_me)
> -	    free (image->bits.free_me);
> +	if (image->type == BITS && image->bits.free_me) {
> +	    free_memory (&image->bits.free_me);
> +	    free_memory (&image->bits.bits);

This will free the memory twice. You should just set bits to NULL after calling free_memory()
Attachment #8779273 - Flags: review?(jmuizelaar) → review-
Comment on attachment 8779273 [details]
Bug 1293598 - clean up pointers after free inside pixman_image,

https://reviewboard.mozilla.org/r/70300/#review77394
Attachment #8779273 - Flags: review- → review+
Pushed by pchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/854e74094d8c
clean up pointers after free inside pixman_image, r=jrmuizel
(In reply to Phil Ringnalda (:philor) from comment #17)
> Backed out in https://hg.mozilla.org/integration/autoland/rev/2c7ba40559c6
> 
> Windows build bustage:
> https://treeherder.mozilla.org/logviewer.html#?job_id=3583663&repo=autoland
> 
Try to fix this build error.
> Anything that could still build SVG jemalloc crashes:
> https://treeherder.mozilla.org/logviewer.html#?job_id=3587348&repo=autoland
> https://treeherder.mozilla.org/logviewer.html#?job_id=3587318&repo=autoland
Wow...my patch hit the jemalloc crash on linux because the arena magic number[1] was mismatch

[1]https://dxr.mozilla.org/mozilla-central/source/memory/mozjemalloc/jemalloc.c#4737
Flags: needinfo?(howareyou322)
My patch had problem because it tried to do pointer arithmetic for void pointer and it was not allowed. I should just pass the address to free_memory API after calculation in [1].

[1]https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/pixman-image.c#161
Submit another patch and wait for try result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6ba33f790c0
Fix the incorrect address calculation and passed the reftest in my local machine.
Attachment #8779273 - Attachment is obsolete: true
Attachment #8796408 - Flags: review+
Pushed by pchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fc3826c761e
clean up pointers after free inside pixman_image, r=jrmuizel
Comment on attachment 8796408 [details] [diff] [review]
Bug 1293598 - clean up pointers after free inside pixman_image

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

I realize this already landed...

::: gfx/cairo/libpixman/src/pixman-image.c
@@ +42,5 @@
> +static void
> +free_memory (void** p)
> +{
> +#ifdef PIXMAN_POISON
> +    if (*p) {

free(NULL) is explicitly safe, so there's no need to test for it here as well
Comment on attachment 8796408 [details] [diff] [review]
Bug 1293598 - clean up pointers after free inside pixman_image

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

::: gfx/cairo/libpixman/src/pixman-image.c
@@ +42,5 @@
> +static void
> +free_memory (void** p)
> +{
> +#ifdef PIXMAN_POISON
> +    if (*p) {

The major part of this patch is tried to reset pointer back to NULL. I think we can watch the crashes first and remove the pointer checking later.
https://hg.mozilla.org/mozilla-central/rev/3fc3826c761e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Attachment #8796408 - Flags: approval-mozilla-aurora?
This patch fixed the potential problem of "dereferencing freed memory". I would like to uplift this to watch the new crashes after the build with this patch.
Comment on attachment 8796408 [details] [diff] [review]
Bug 1293598 - clean up pointers after free inside pixman_image

Approval Request Comment
[Feature/regressing bug #]:none
[User impact if declined]:Users could hit the random crash problem because of "dereferencing freed memory".
[Describe test coverage new/current, TreeHerder]: got passed in try and local testing
[Risks and why]: low risk since it only cleanup free pointer as NULL 
[String/UUID change made/needed]:none
Comment on attachment 8796408 [details] [diff] [review]
Bug 1293598 - clean up pointers after free inside pixman_image

Fix a possible crash. Take it in 51 aurora.
Attachment #8796408 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Gerry Chang [:gchang] from comment #28)
> Comment on attachment 8796408 [details] [diff] [review]
> Bug 1293598 - clean up pointers after free inside pixman_image
> 
> Fix a possible crash. Take it in 51 aurora.

FWIW, this doesn't fix the crash just turns it in a null deref
Blocks: 1268202
(In reply to Jeff Muizelaar [:jrmuizel] from comment #30)
> (In reply to Gerry Chang [:gchang] from comment #28)
> > Comment on attachment 8796408 [details] [diff] [review]
> > Bug 1293598 - clean up pointers after free inside pixman_image
> > 
> > Fix a possible crash. Take it in 51 aurora.
> 
> FWIW, this doesn't fix the crash just turns it in a null deref

Yes, that's right. I will monitor the crashes in bug 1268202.
Blocks: 1258230
You need to log in before you can comment on or make changes to this bug.