[cairo] [Fwd: cairo bugs]

[Carl Worth]

On Tue, 06 Sep 2005 16:46:34 -0400, Kristian Høgsberg wrote:
> I got this mail from Steve Grubb, who's been looking over that cairo
> source and found a couple of issues as described below.  The patch looks
> good to me - ok to commit?

Comments below.

> I did a quick run through of my source debuggers and found a couple minor 
> bugs. I'm attaching a patch that fixed 3 memory leaks and points out 
> something really weird.

I'm curious what source debuggers were used to find these?

We've got a few issues in the source code now which are known to be
fragile and that standard compiler checks don't help with,
(floating-point <-> fixed-point conversion being one prominent
issue). We're talking about looking at sparse to help with these, but
if anyone has other static checking tools that would be useful, we'd
love to hear about them.

> In pixman/src/fbpict.c is this code:
> 
> switch(!(long)dst&3)
> 
> What is that "!" doing in the switch? That should reduce the results to either 
> a 0 or 1. That means that case 2 & 3 are not used. I'm not 100% sure if my 
> patch is correct in this particular piece of code. I think someone more 
> familiar with this project should review this section of code. Maybe its 
> supposed to be (!dst)&3. It needs something.

Good catch! The ! is obviously wrong, and my examination of the code
suggests that it is correct to simply remove it as in the patch. Note
that this fix also needs to be applied in the xserver tree which is
pixman's upstream. So the fix should be applied simultaneously or else
a bug should be opened against xserver.

It might be worth cooking up a test case to hit this code just to
understand better what this bug was doing.

> diff -ur cairo-1.0.0.orig/src/cairo-xlib-surface.c cairo-1.0.0/src/cairo-xlib-surface.c
> --- cairo-1.0.0.orig/src/cairo-xlib-surface.c	2005-09-05 16:18:28.000000000 -0400
> +++ cairo-1.0.0/src/cairo-xlib-surface.c	2005-09-05 16:53:48.000000000 -0400
> @@ -2020,8 +2020,10 @@
>  	    unsigned char   *new, *n;
>  	    
>  	    new = malloc (c);
> -	    if (!new)
> +	    if (!new) {
> +		free (entry);
>  		return CAIRO_STATUS_NO_MEMORY;
> +	    }
>  	    n = new;
>  	    d = data;
>  	    while (c--)

This fix, (and the similar one below) will apply to the cairo 1.0
branch, but not to head where the leak has been obviated, (entry no
longer appears in the function). (PS. The "-p" option to diff would
have been helpful in this case.)

The other fixed memory leak looks good and should be committed to both
branches.

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050906/f42a6075/attachment.pgp