From 08d9864fa4e0c616e076ca8b225d39a7ecb189af Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Thu, 17 May 2018 17:00:11 +0200 Subject: [PATCH 1/3] console: Avoid segfault in screendump 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 Reviewed-by: Thomas Huth Message-id: cb05bb1909daa6ba62145c0194aafa05a14ed3d1.1526569138.git.mprivozn@redhat.com Signed-off-by: Gerd Hoffmann --- 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); } From 68898bc82bcb0e697ed03c2405321033ba7feaf7 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 17 May 2018 14:39:42 +0200 Subject: [PATCH 2/3] ui: add x_keymap.o to modules x_keymap.o is common to the SDL and GTK+ modules, and it causes the QEMU binary to link to the X11 libraries. Add it separately to the modules to keep the main QEMU binary smaller. Signed-off-by: Paolo Bonzini Message-id: 1526560782-18732-1-git-send-email-pbonzini@redhat.com [ kraxel: fix lm32 target build (milkymist-tmu2) ] Signed-off-by: Gerd Hoffmann --- hw/display/Makefile.objs | 2 ++ ui/Makefile.objs | 11 +++++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs index 3c7c75b94d..11321e466b 100644 --- a/hw/display/Makefile.objs +++ b/hw/display/Makefile.objs @@ -20,6 +20,8 @@ common-obj-$(CONFIG_MILKYMIST) += milkymist-vgafb.o common-obj-$(CONFIG_ZAURUS) += tc6393xb.o common-obj-$(CONFIG_MILKYMIST_TMU2) += milkymist-tmu2.o +milkymist-tmu2.o-cflags := $(X11_CFLAGS) +milkymist-tmu2.o-libs := $(X11_LIBS) obj-$(CONFIG_OMAP) += omap_dss.o obj-$(CONFIG_OMAP) += omap_lcdc.o diff --git a/ui/Makefile.objs b/ui/Makefile.objs index cc784346cb..00f6976c30 100644 --- a/ui/Makefile.objs +++ b/ui/Makefile.objs @@ -15,10 +15,6 @@ common-obj-$(CONFIG_COCOA) += cocoa.o common-obj-$(CONFIG_VNC) += $(vnc-obj-y) common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o -common-obj-$(CONFIG_X11) += x_keymap.o -x_keymap.o-cflags := $(X11_CFLAGS) -x_keymap.o-libs := $(X11_LIBS) - # ui-sdl module common-obj-$(CONFIG_SDL) += sdl.mo ifeq ($(CONFIG_SDLABI),1.2) @@ -46,6 +42,13 @@ gtk.mo-objs += gtk-gl-area.o endif endif +ifeq ($(CONFIG_X11),y) +sdl.mo-objs += x_keymap.o +gtk.mo-objs += x_keymap.o +x_keymap.o-cflags := $(X11_CFLAGS) +x_keymap.o-libs := $(X11_LIBS) +endif + common-obj-$(CONFIG_CURSES) += curses.mo curses.mo-objs := curses.o curses.mo-cflags := $(CURSES_CFLAGS) From e8dcb8ae5121965ac8c89e6b277ac127e9d08452 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 15 May 2018 19:58:14 +0100 Subject: [PATCH 3/3] sdl: Move use of surface pointer below check for whether it is NULL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In commit 2ab858c6c38ee1 we added a use of the 'surf' variable in sdl2_2d_update() that was unfortunately placed above the early-exit-if-NULL check. Move it to where it ought to be. Fixes: Coverity CID 1390598 Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Message-id: 20180515185814.1374-1-peter.maydell@linaro.org Signed-off-by: Gerd Hoffmann --- ui/sdl2-2d.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ui/sdl2-2d.c b/ui/sdl2-2d.c index 1f34817bae..85484407be 100644 --- a/ui/sdl2-2d.c +++ b/ui/sdl2-2d.c @@ -36,9 +36,7 @@ void sdl2_2d_update(DisplayChangeListener *dcl, struct sdl2_console *scon = container_of(dcl, struct sdl2_console, dcl); DisplaySurface *surf = qemu_console_surface(dcl->con); SDL_Rect rect; - size_t surface_data_offset = surface_bytes_per_pixel(surf) * x + - surface_stride(surf) * y; - + size_t surface_data_offset; assert(!scon->opengl); if (!surf) { @@ -48,6 +46,8 @@ void sdl2_2d_update(DisplayChangeListener *dcl, return; } + surface_data_offset = surface_bytes_per_pixel(surf) * x + + surface_stride(surf) * y; rect.x = x; rect.y = y; rect.w = w;