Re: [Qemu-devel] [PATCH] console: Avoid segfault in screendump

[Thomas Huth <thuth@redhat.com>]

On 17.05.2018 17:00, Michal Privoznik wrote:
> After f771c5440e04626f1 it is possible to select device and
> head which to take screendump from. And even though we check if
> provided head number falls within range, it may still happen that
> the console has no surface yet leading to SIGSEGV:
> 
>   qemu.git $ ./x86_64-softmmu/qemu-system-x86_64 \
>     -qmp stdio \
>     -device virtio-vga,id=video0,max_outputs=4
> 
>   {"execute":"qmp_capabilities"}
>   {"execute":"screendump", "arguments":{"filename":"/tmp/screen.ppm", "device":"video0", "head":1}}
>   Segmentation fault
> 
>  #0  0x00005628249dda88 in ppm_save (filename=0x56282826cbc0 "/tmp/screen.ppm", ds=0x0, errp=0x7fff52a6fae0) at ui/console.c:304
>  #1  0x00005628249ddd9b in qmp_screendump (filename=0x56282826cbc0 "/tmp/screen.ppm", has_device=true, device=0x5628276902d0 "video0", has_head=true, head=1, errp=0x7fff52a6fae0) at ui/console.c:375
>  #2  0x00005628247740df in qmp_marshal_screendump (args=0x562828265e00, ret=0x7fff52a6fb68, errp=0x7fff52a6fb60) at qapi/qapi-commands-ui.c:110
> 
> Here, @ds from frame #0 (or @surface from frame #1) is
> dereferenced at the very beginning of ppm_save(). And because
> it's NULL crash happens.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> I'm not entirely sure if this is the right approach, but crasher is bad
> for sure.
> 
>  ui/console.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/ui/console.c b/ui/console.c
> index 945f05d728..ef1247f872 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -372,6 +372,11 @@ void qmp_screendump(const char *filename, bool has_device, const char *device,
>  
>      graphic_hw_update(con);
>      surface = qemu_console_surface(con);
> +    if (!surface) {
> +        error_setg(errp, "no surface");
> +        return;
> +    }
> +
>      ppm_save(filename, surface, errp);
>  }

Looks reasonable to me!

Reviewed-by: Thomas Huth <thuth@redhat.com>

(and added CC: to qemu-stable)