Re: [PATCH] ui/gtk: fix NULL pointer dereference

[Akihiko Odaki]

2021年3月8日(月) 21:42 Christian Schoenebeck <qemu_oss@crudebyte.com>:
>
> Unfortunately Coverity's defects reports are not very verbose. In this case:
>
> *** CID 1448421:    (FORWARD_NULL)
> /qemu/ui/gtk.c: 570 in gd_switch()
> 564             surface_width(vc->gfx.ds) == surface_width(surface) &&
> 565             surface_height(vc->gfx.ds) == surface_height(surface)) {
> 566             resized = false;
> 567         }
> 568         vc->gfx.ds = surface;
> 569
> >>> CID 1448421:    (FORWARD_NULL)
> >>> Dereferencing null pointer "surface".
> 570         if (surface->format == PIXMAN_x8r8g8b8) {
> 571             /*
> 572              * PIXMAN_x8r8g8b8 == CAIRO_FORMAT_RGB24
> 573              *
> 574              * No need to convert, use surface directly.  Should be the
> 575              * common case as this is qemu_default_pixelformat(32) too.
>
> So no detailed path is outlined that may lead to the detected situation (i.e.
> no call stack or conditions like you would get e.g. with clang's static
> analyzer).

Hmm, Coverity should have decided the surface can somehow be NULL. I
hope it is false-positive...

>
> There are false positives sometimes, but they should be silenced in some way.
>
> So as you assume "surface" pointer should never be NULL, why did you remove
> the return statement in gd_switch() with c821a58ee7 then? Redundancy?
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index c32ee34edc..3edaf041de 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -567,10 +567,6 @@ static void gd_switch(DisplayChangeListener *dcl,
>      }
>      vc->gfx.ds = surface;
>
> -    if (!surface) {
> -        return;
> -    }
> -
>      if (surface->format == PIXMAN_x8r8g8b8) {
>          /*
>           * PIXMAN_x8r8g8b8 == CAIRO_FORMAT_RGB24
>
> I was reading your change as you wanted to reach the end of the function in
> case of surface == NULL.
>
> Best regards,
> Christian Schoenebeck
>
>

Redundancy is one reason.

It is also intended to prevent people writing ui/console code from
assuming displays accept NULL as surface. In reality, some other
displays dereferenced surfaces without checking NULL even before this
change. The code checking if the surface is NULL is confusing when
reading the source code. In runtime, pointer dereferences following
the conditional should assert the pointer is not NULL and prevent code
which produces NULL from getting in.

Regards,
Akihiko Odaki