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 <mpriv...@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 <th...@redhat.com>
(and added CC: to qemu-stable)
