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

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

Details

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

Commit Message

Bryce Harrington June 9, 2018, 5:34 a.m.
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 <bryce@bryceharrington.org>
---
 src/cairo-script-surface.c | 3 ++-
 util/cairo-trace/trace.c   | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

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;

Comments

Uli Schlachter June 9, 2018, 6:53 a.m.
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);

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 <bryce@bryceharrington.org>
> ---
>  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;
>
Bryce Harrington June 12, 2018, 9:18 p.m.
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 <bryce@bryceharrington.org>
> > ---
> >  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.