[2/3] bo: Check null return from _cairo_malloc_ab() (CID #1159556)

Submitted by Bryce Harrington on June 9, 2018, 5:34 a.m.

Details

Message ID 1528522467-8873-3-git-send-email-bryce@bryceharrington.org
State New
Series "Coverity fixes"
Headers show

Commit Message

Bryce Harrington June 9, 2018, 5:34 a.m.
_cairo_malloc_ab() can return NULL under some circumstances, and all
other callers of this routine in the Cairo codebase check its return, so
do so here as well.

(I'm not sure that cairo-bentley-ottmann.c is actually plugged in
anywhere for actual use, so this change may be more to quell Coverity
than fix an actual likely bug.  However, the bo code has been used as a
starting point when writing compositors so perhaps is a useful thing to
cleanup.)

Coverity ID: #1159556

Signed-off-by: Bryce Harrington <bryce@bryceharrington.org>
---
 src/cairo-bentley-ottmann.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/cairo-bentley-ottmann.c b/src/cairo-bentley-ottmann.c
index 91e41f9..afe3a63 100644
--- a/src/cairo-bentley-ottmann.c
+++ b/src/cairo-bentley-ottmann.c
@@ -1484,10 +1484,13 @@  _cairo_bentley_ottmann_tessellate_polygon (cairo_traps_t	 *traps,
 	ymin = _cairo_fixed_integer_floor (polygon->limit.p1.y);
 	ymax = _cairo_fixed_integer_ceil (polygon->limit.p2.y) - ymin;
 
-	if (ymax > 64)
+	if (ymax > 64) {
 	    event_y = _cairo_malloc_ab(sizeof (cairo_bo_event_t*), ymax);
-	else
+	    if (unlikely (event_y == NULL))
+		return _cairo_error (CAIRO_STATUS_NO_MEMORY);
+	} else {
 	    event_y = stack_event_y;
+	}
 	memset (event_y, 0, ymax * sizeof(cairo_bo_event_t *));
     }
 

Comments

Uli Schlachter June 9, 2018, 6:58 a.m.
Reviewed-by: Uli Schlachter <psychon@znc.in>

(Sigh, that code leaks memory; there is a following call to
_cairo_malloc_ab_plus_c() in this function and if that fails, it returns
an error without first freeing event_y (after checking if it has to be
freed))

On 09.06.2018 07:34, Bryce Harrington wrote:
> _cairo_malloc_ab() can return NULL under some circumstances, and all
> other callers of this routine in the Cairo codebase check its return, so
> do so here as well.
> 
> (I'm not sure that cairo-bentley-ottmann.c is actually plugged in
> anywhere for actual use, so this change may be more to quell Coverity
> than fix an actual likely bug.  However, the bo code has been used as a
> starting point when writing compositors so perhaps is a useful thing to
> cleanup.)
> 
> Coverity ID: #1159556
> 
> Signed-off-by: Bryce Harrington <bryce@bryceharrington.org>
> ---
>  src/cairo-bentley-ottmann.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/cairo-bentley-ottmann.c b/src/cairo-bentley-ottmann.c
> index 91e41f9..afe3a63 100644
> --- a/src/cairo-bentley-ottmann.c
> +++ b/src/cairo-bentley-ottmann.c
> @@ -1484,10 +1484,13 @@ _cairo_bentley_ottmann_tessellate_polygon (cairo_traps_t	 *traps,
>  	ymin = _cairo_fixed_integer_floor (polygon->limit.p1.y);
>  	ymax = _cairo_fixed_integer_ceil (polygon->limit.p2.y) - ymin;
>  
> -	if (ymax > 64)
> +	if (ymax > 64) {
>  	    event_y = _cairo_malloc_ab(sizeof (cairo_bo_event_t*), ymax);
> -	else
> +	    if (unlikely (event_y == NULL))
> +		return _cairo_error (CAIRO_STATUS_NO_MEMORY);
> +	} else {
>  	    event_y = stack_event_y;
> +	}
>  	memset (event_y, 0, ymax * sizeof(cairo_bo_event_t *));
>      }
>  
>
Bryce Harrington June 12, 2018, 9:55 p.m.
On Sat, Jun 09, 2018 at 08:58:29AM +0200, Uli Schlachter wrote:
> Reviewed-by: Uli Schlachter <psychon@znc.in>

Thanks.

> (Sigh, that code leaks memory; there is a following call to
> _cairo_malloc_ab_plus_c() in this function and if that fails, it returns
> an error without first freeing event_y (after checking if it has to be
> freed))

Indeed, and Coverity has this logged as CID #1160682.  I'll spin up a
fix for that one as well.

Bryce

> On 09.06.2018 07:34, Bryce Harrington wrote:
> > _cairo_malloc_ab() can return NULL under some circumstances, and all
> > other callers of this routine in the Cairo codebase check its return, so
> > do so here as well.
> > 
> > (I'm not sure that cairo-bentley-ottmann.c is actually plugged in
> > anywhere for actual use, so this change may be more to quell Coverity
> > than fix an actual likely bug.  However, the bo code has been used as a
> > starting point when writing compositors so perhaps is a useful thing to
> > cleanup.)
> > 
> > Coverity ID: #1159556
> > 
> > Signed-off-by: Bryce Harrington <bryce@bryceharrington.org>
> > ---
> >  src/cairo-bentley-ottmann.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/cairo-bentley-ottmann.c b/src/cairo-bentley-ottmann.c
> > index 91e41f9..afe3a63 100644
> > --- a/src/cairo-bentley-ottmann.c
> > +++ b/src/cairo-bentley-ottmann.c
> > @@ -1484,10 +1484,13 @@ _cairo_bentley_ottmann_tessellate_polygon (cairo_traps_t	 *traps,
> >  	ymin = _cairo_fixed_integer_floor (polygon->limit.p1.y);
> >  	ymax = _cairo_fixed_integer_ceil (polygon->limit.p2.y) - ymin;
> >  
> > -	if (ymax > 64)
> > +	if (ymax > 64) {
> >  	    event_y = _cairo_malloc_ab(sizeof (cairo_bo_event_t*), ymax);
> > -	else
> > +	    if (unlikely (event_y == NULL))
> > +		return _cairo_error (CAIRO_STATUS_NO_MEMORY);
> > +	} else {
> >  	    event_y = stack_event_y;
> > +	}
> >  	memset (event_y, 0, ymax * sizeof(cairo_bo_event_t *));
> >      }
> >  
> > 
> 
> 
> -- 
> Bruce Schneier can read and understand Perl programs.