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

[Christian Schoenebeck]

On Montag, 8. März 2021 14:37:44 CET Peter Maydell wrote:
> On Mon, 8 Mar 2021 at 13:32, Christian Schoenebeck
> 
> <qemu_oss@crudebyte.com> wrote:
> > 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.
> 
> The online defect viewer is a bit better for showing why it thought
> something was an issue. In this case we have at the top of the function:

Ah, good to know. Actually never looked into the online viewer. Thanks Peter!

>     trace_gd_switch(vc->label,
>                     surface ? surface_width(surface)  : 0,
>                     surface ? surface_height(surface) : 0);
> 
> which tests whether surface is NULL, implying that sometimes it is.
> 
> Then later we have:
>     if (vc->gfx.ds && surface &&
> 
> also checking surface for NULL-ness.
> 
> Finally we have:
>     if (surface->format == PIXMAN_x8r8g8b8) {
> 
> which dereferences surface without checking if it's NULL.
> 
> So there is definitely a bug here:
> (1) either surface can never be NULL, and all the places where
> the function is testing for NULL-ness are wrong and need to be removed
> (2) or surface can be NULL, and we should check here too
> 
> Coverity can't tell us which of the two possibilities is right, of course.

BTW, there is __nonnull supported by clang, e.g.:

static void foo(void *__nonnull p) {
        ...
}

Maybe as an optionally defined macro (if supported by compiler) this could be 
a useful tool for such intended nonnull designs, as it immediately emits 
compiler errors.

Best regards,
Christian Schoenebeck