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

[Christian Schoenebeck]

On Montag, 8. März 2021 12:31:33 CET Akihiko Odaki wrote:
> 2021年3月8日(月) 19:39 Christian Schoenebeck <qemu_oss@crudebyte.com>:
> > This was just about silencing the mentioned automated Coverity defects
> > report. If you have a better solution, then just ignore this patch.
> > 
> > Best regards,
> > Christian Schoenebeck
> 
> I do not have an access to Coverity defects report. I'd appreciate the
> details if you provide one. I suspect I made a mistake somewhere else
> ui/gtk.c in c821a58ee7 ("ui/console: Pass placeholder surface to
> display").

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).

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