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

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 <[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