Re: [PATCH cairo 1/3] script-surface: Check for invalid ids (CID #1159557, 1159558)

On Sat, Jun 09, 2018 at 08:53:30AM +0200, Uli Schlachter wrote:
> Well... this looks like a linked list of "struct _bitmap"s to somehow
> give out some kind of bits... but I do not really understand this.
> 
> Anyway, with your patch, the code malloc()s a new "struct _bitmap" and
> then immediately leaks it. That's not good (and might even be worse than
> the old NULL pointer dereference, because there it is a lot more obvious
> that something is going wrong while the other case would likely be a
> nightmare to debug).
> 
> I *guess* that the case that you are fixing can never occur, because
> some invariant guarantees that the code only exits the do-while-loop
> after having done at least one iteration.
> 
> Thus, I *guess* this is a false-positive and, if you really want to fix
> it, is best fixed / papered over by doing assert (prev != NULL);

Sounds good, I had wondered if an assert might be more appropriate,
thanks for pointing out the potential leak.

Bryce
 
> On 09.06.2018 07:34, Bryce Harrington wrote:
> > When the bitmap's min is non-zero, _bitmap_next_id() may break out of
> > its loop early, before initializing the prev variable.  prev is then
> > dereferenced without a null ptr check.
> > 
> > Same issue is present in trace.c.
> > 
> > Coverity IDs: #1159557, #1159558
> > 
> > Signed-off-by: Bryce Harrington <[email protected]>
> > ---
> >  src/cairo-script-surface.c | 3 ++-
> >  util/cairo-trace/trace.c   | 4 +++-
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/cairo-script-surface.c b/src/cairo-script-surface.c
> > index e715cae..228d60e 100644
> > --- a/src/cairo-script-surface.c
> > +++ b/src/cairo-script-surface.c
> > @@ -267,7 +267,8 @@ _bitmap_next_id (struct _bitmap *b,
> >      if (unlikely (bb == NULL))
> >  	return _cairo_error (CAIRO_STATUS_NO_MEMORY);
> >  
> > -    *prev = bb;
> > +    if (prev != NULL)
> > +	*prev = bb;
> >      bb->next = b;
> >      bb->min = min;
> >      bb->count = 1;
> > diff --git a/util/cairo-trace/trace.c b/util/cairo-trace/trace.c
> > index 3c05613..269e876 100644
> > --- a/util/cairo-trace/trace.c
> > +++ b/util/cairo-trace/trace.c
> > @@ -301,7 +301,9 @@ _type_next_token (Type *t)
> >      }
> >  
> >      bb = malloc (sizeof (struct _bitmap));
> > -    *prev = bb;
> > +
> > +    if (prev != NULL)
> > +	*prev = bb;
> >      bb->next = b;
> >      bb->min = min;
> >      bb->count = 1;
> > 
> 
> 
> -- 
> Bruce Schneier can read and understand Perl programs.
-- 
cairo mailing list
[email protected]
https://lists.cairographics.org/mailman/listinfo/cairo