[cairo] Consistent state during error recovery

[Kristian Høgsberg]

Mike Owens wrote:
> I believe I made an assumption about realloc's return value that doesn't hold 
> true on all platforms, updated patch attached.

I've committed the memory leak stuffing parts of your patch (the 
cairo_path.c and cairo_png_surface.c bits). However, I don't know that 
we want to guarantee that the cairo_t is consistent and generally useful 
in the face of CAIRO_STATUS_NO_MEMORY.  It's a given that we shouldn't 
leak in those cases and that a subsequent cairo_destroy() should 
properly destroy the inconsistent cairo_t.

There is quite a few other entry points in the API that doesn't give you 
this behavior, for example all the path construction operators can leave 
the path inconsistent if they fail.  This mean that you won't be able to 
do fine grained OOOM handling by e.g. repeatedly setting the dash until 
it succeeds.  However, it will be possible to handle OOM at a higher 
level, by e.g. creating a new cairo_t and retrying the entire drawing 
sequence.

cheers,
Kristian

> ------------------------------------------------------------------------
> 
> ? consistent_recovery.patch
> Index: src/cairo_gstate.c
> ===================================================================
> RCS file: /cvs/cairo/cairo/src/cairo_gstate.c,v
> retrieving revision 1.83
> diff -u -3 -p -r1.83 cairo_gstate.c
> --- src/cairo_gstate.c	13 Feb 2005 02:23:04 -0000	1.83
> +++ src/cairo_gstate.c	14 Feb 2005 17:38:40 -0000
> @@ -531,22 +531,21 @@ _cairo_gstate_current_line_join (cairo_g
>  cairo_status_t
>  _cairo_gstate_set_dash (cairo_gstate_t *gstate, double *dash, int num_dashes, double offset)
>  {
> -    if (gstate->dash) {
> -	free (gstate->dash);
> -	gstate->dash = NULL;
> -    }
> -    
> -    gstate->num_dashes = num_dashes;
> -    if (gstate->num_dashes) {
> -	gstate->dash = malloc (gstate->num_dashes * sizeof (double));
> -	if (gstate->dash == NULL) {
> -	    gstate->num_dashes = 0;
> -	    return CAIRO_STATUS_NO_MEMORY;
> +	double *repl = gstate->dash;
> +
> +	if(num_dashes != 0) {
> +		repl = realloc (repl, (num_dashes * sizeof (double)));
> +		if(repl == NULL)
> +			return CAIRO_STATUS_NO_MEMORY;
> +	} else {
> +		free (repl);
> +		repl = NULL;
>  	}
> -    }
>  
> -    memcpy (gstate->dash, dash, gstate->num_dashes * sizeof (double));
> -    gstate->dash_offset = offset;
> +	gstate->dash = repl;
> +	gstate->num_dashes = num_dashes;
> +	memcpy (gstate->dash, dash, gstate->num_dashes * sizeof (double));
> +	gstate->dash_offset = offset;
>  
>      return CAIRO_STATUS_SUCCESS;
>  }
> Index: src/cairo_path.c
> ===================================================================
> RCS file: /cvs/cairo/cairo/src/cairo_path.c,v
> retrieving revision 1.17
> diff -u -3 -p -r1.17 cairo_path.c
> --- src/cairo_path.c	22 Oct 2004 01:40:50 -0000	1.17
> +++ src/cairo_path.c	14 Feb 2005 17:38:40 -0000
> @@ -100,7 +100,8 @@ _cairo_path_init_copy (cairo_path_t *pat
>      for (other_op = other->op_head; other_op; other_op = other_op->next) {
>  	op = _cairo_path_op_buf_create ();
>  	if (op == NULL) {
> -	    return CAIRO_STATUS_NO_MEMORY;
> +		_cairo_path_fini(path);
> +		return CAIRO_STATUS_NO_MEMORY;
>  	}
>  	*op = *other_op;
>  	_cairo_path_add_op_buf (path, op);
> @@ -109,7 +110,8 @@ _cairo_path_init_copy (cairo_path_t *pat
>      for (other_arg = other->arg_head; other_arg; other_arg = other_arg->next) {
>  	arg = _cairo_path_arg_buf_create ();
>  	if (arg == NULL) {
> -	    return CAIRO_STATUS_NO_MEMORY;
> +		_cairo_path_fini(path);
> +		return CAIRO_STATUS_NO_MEMORY;
>  	}
>  	*arg = *other_arg;
>  	_cairo_path_add_arg_buf (path, arg);
> Index: src/cairo_pattern.c
> ===================================================================
> RCS file: /cvs/cairo/cairo/src/cairo_pattern.c,v
> retrieving revision 1.18
> diff -u -3 -p -r1.18 cairo_pattern.c
> --- src/cairo_pattern.c	31 Jan 2005 16:50:22 -0000	1.18
> +++ src/cairo_pattern.c	14 Feb 2005 17:38:40 -0000
> @@ -223,14 +223,13 @@ cairo_pattern_add_color_stop (cairo_patt
>      _cairo_restrict_value (&green, 0.0, 1.0);
>      _cairo_restrict_value (&blue, 0.0, 1.0);
>  
> -    pattern->n_stops++;
> -    pattern->stops = realloc (pattern->stops,
> -			      sizeof (cairo_color_stop_t) * pattern->n_stops);
> -    if (pattern->stops == NULL) {
> -	pattern->n_stops = 0;
> -    
> -	return CAIRO_STATUS_NO_MEMORY;
> -    }
> +    stop = realloc(pattern->stops, 
> +                   sizeof (cairo_color_stop_t) * (pattern->n_stops + 1)); 
> +    if (stop == NULL) 
> +        return CAIRO_STATUS_NO_MEMORY;
> +
> +    pattern->stops = stop;
> +    ++pattern->n_stops;
>  
>      stop = &pattern->stops[pattern->n_stops - 1];
>  
> Index: src/cairo_png_surface.c
> ===================================================================
> RCS file: /cvs/cairo/cairo/src/cairo_png_surface.c,v
> retrieving revision 1.14
> diff -u -3 -p -r1.14 cairo_png_surface.c
> --- src/cairo_png_surface.c	31 Jan 2005 16:50:22 -0000	1.14
> +++ src/cairo_png_surface.c	14 Feb 2005 17:38:40 -0000
> @@ -331,13 +331,15 @@ _cairo_png_surface_copy_page (void *abst
>  	rows[i] = surface->image->data + i * surface->image->stride;
>  
>      png = png_create_write_struct (PNG_LIBPNG_VER_STRING, NULL, NULL, NULL);
> -    if (png == NULL)
> -	return CAIRO_STATUS_NO_MEMORY;
> +    if (png == NULL) {
> +        free(rows);
> +        return CAIRO_STATUS_NO_MEMORY;
> +	}
>  
>      info = png_create_info_struct (png);
>      if (info == NULL) {
> -	png_destroy_write_struct (&png, NULL);
> -	return CAIRO_STATUS_NO_MEMORY;
> +        status = CAIRO_STATUS_NO_MEMORY;
> +		goto BAIL;
>      }
>  
>      if (setjmp (png_jmpbuf (png))) {
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> cairo mailing list
> cairo at cairographics.org
> http://lists.freedesktop.org/mailman/listinfo/cairo