[cairo] 1.4.2 release planned for today, 1.4.4 in a few weeks

[Carl Worth]

On Fri, 16 Mar 2007 16:40:45 +0000, Chris Wilson wrote:
> Following Behad's suggestion that Cairo really should suffer some
> malloc-failure testing, I wrote a simple valgrind skin to randomly
> return NULL.
>
> The effort was justified as it's already pinpointed a few places that
> needed some error checking.

Hi Chris,

This looks like great work, and I'm excited about getting fixes like
this into cairo. But I think we'll need a bit more work on these
specific patches before they go into cairo, (that is, I'll probably go
ahead and release 1.4.2 without these today, unless there's some
really fast turnaround on these).

> @@ -187,6 +187,8 @@ _pixman_create_source_image (void)
>      pixman_image_t *image;
>
>      image = (pixman_image_t *) malloc (sizeof (pixman_image_t));
> +    if (!image)
> +	return;

That's introducing the failure to return a value from a non-void
function.

[And as a style point, I prefer "if (image == NULL)" for testing
against NULL. If you do insist on treating a pointer as a Boolean,
(something not unheard of in the cairo source code), I prefer to see a
space character used to call attention to the !  operator. That is:
"if (!  condition)"]

> diff --git a/src/cairo-skiplist.c b/src/cairo-skiplist.c
> index 451ecb0..9f27ffa 100644
> --- a/src/cairo-skiplist.c
> +++ b/src/cairo-skiplist.c
> @@ -349,6 +349,8 @@ _cairo_skip_list_insert (cairo_skip_list_t *list, void *data, int unique)
>      }
>
>      data_and_elt = alloc_node_for_level (list, level);
> +    if (!data_and_elt)
> +	return NULL;

A fix like this doesn't actually help unless we also propagate the
checks all the way up, (the current callers of _cairo_skip_list_insert
are calling this function without checking). And the easiest time to
catch those callers is when introducing this fix. Otherwise, we're
basically just moving a bug from one location to many locations---not
actually improving cairo at all.

> Subject: [PATCH] Handle and return out-of-memory condition.
>
> ---
>  src/cairo-bentley-ottmann.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)

This patch at least looks functionally complete. The patch description
should be more descriptive though, (specifically mentioning
_cairo_bo_sweep_line_insert at least), particularly since we
anticipate a whole slew of similar fixes as testing reveals similar
failures to handle out-of-memory conditions.

Also:

>      edge->sweep_line_elt = sweep_line_elt;
> +    return CAIRO_STATUS_SUCCESS;
...
> -	    _cairo_bo_sweep_line_insert (&sweep_line, edge);
> +	    status = _cairo_bo_sweep_line_insert (&sweep_line, edge);
> +	    if (status)
> +		goto unwind;
>  	    /* Cache the insert position for use in pass 2.

There are missing newlines in the above, (before the return value and
after the goto). Statements that change the control flow really need
to be visually called out somehow compared to other sequences of
statements that are always executed together as a basic block.

-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/20070319/08d96ad8/attachment.pgp