1293598 - clean up pointer after free in pixman_image_fini

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Peter Chang[:pchang]]

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".

Comment 1

[Peter Chang[:pchang]]

Attachment: Bug 1293598 - clean up pointers after free inside pixman_image,

Review commit: https://reviewboard.mozilla.org/r/70300/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70300/

Comment 2

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 3

[Peter Chang[:pchang]]

Comment on attachment 8779273 [details]
Bug 1293598 - clean up pointers after free inside pixman_image,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70300/diff/1-2/

Comment 4

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 5

[Peter Chang[:pchang]]

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

Comment 6

[Peter Chang[:pchang]]

ni for comment 5.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 8

[Peter Chang[:pchang]]

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.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 10

[Peter Chang[:pchang]]

(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".

Comment 11

[Peter Chang[:pchang]]

Comment on attachment 8779273 [details]
Bug 1293598 - clean up pointers after free inside pixman_image,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70300/diff/2-3/

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Is 50 also affected? If it is you may want to request uplift.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

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()

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8779273 [details]
Bug 1293598 - clean up pointers after free inside pixman_image,

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

Comment 15

[Peter Chang[:pchang]]

Comment on attachment 8779273 [details]
Bug 1293598 - clean up pointers after free inside pixman_image,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70300/diff/3-4/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/854e74094d8c
clean up pointers after free inside pixman_image, r=jrmuizel

Comment 17

[Phil Ringnalda (:philor)]

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

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

Comment 18

[Peter Chang[:pchang]]

(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

Comment 19

[Peter Chang[:pchang]]

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

Comment 20

[Peter Chang[:pchang]]

Submit another patch and wait for try result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6ba33f790c0

Comment 21

[Peter Chang[:pchang]]

Attachment: Bug 1293598 - clean up pointers after free inside pixman_image

Fix the incorrect address calculation and passed the reftest in my local machine.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fc3826c761e
clean up pointers after free inside pixman_image, r=jrmuizel

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

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 24

[Peter Chang[:pchang]]

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.

Comment 25

[Phil Ringnalda (:philor)]

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

Comment 26

[Peter Chang[:pchang]]

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 27

[Peter Chang[:pchang]]

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 28

[Gerry Chang [:gchang]]

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.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c8073dd59c81

Comment 30

[Jeff Muizelaar [:jrmuizel]]

(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

Comment 31

[Peter Chang[:pchang]]

(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.