[Qemu-devel] [PATCH v3 00/18] Various build-sys and sanitizer related fixes

[Qemu-devel] [PATCH v3 00/18] Various build-sys and sanitizer related fixes

[Marc-André Lureau]

Hi,

This is a series that improves a bit the build system, and introduces
ASAN/UBSan by default when --enable-debug. Then it fixes a few leaks
that occur during make check: common and x86_64 target tests are leak
free after this series. The other targets will need some work to fix
the leaks & warnings.

v3:
- add ubsan too with --enable-debug, since it is recommended and has
  low runtime cost
- add a patch "compile with -Og or -O1 when --enable-debug", suggested
  by Paolo
- add 2 new fixes to pass full make check with asan/ubsan
- modify docker tests to run --enable-debug & asan with new test-debug
- add some r-b/a-b tags

v2:
- simplify "build-sys: silence make by default or V=0": make it a
  oneliner MAKEFLAGS, use --quiet.
- document print-VAR rule in docs/devel/build-system.txt
- fix ASAN coroutine instrumentation failure
- should fix builds on gcc 4.4.7 (centos 6)
- new coroutine test leak fix
- add some r-b tags

Marc-André Lureau (18):
  build-sys: fix qemu-ga -pthread linking
  build-sys: silence make by default or V=0
  build-sys: add a rule to print a variable
  build-sys: compile with -Og or -O1 when --enable-debug
  tests/docker: add some sanitizers to fedora dockerfile
  tests/docker: add test-debug
  build-sys: add some sanitizers when --enable-debug if possible
  tests: fix check-qobject leak
  vl: fix direct firmware directories leak
  readline: add a free function
  tests: fix migration-test leak
  crypto: fix stack-buffer-overflow error
  qemu-config: fix leak in query-command-line-options
  tests: fix qmp-test leak
  ucontext: annotate coroutine stack for ASAN
  tests: fix coroutine leak in /basic/entered
  mips: fix potential fopen(NULL,...)
  disas/s390: fix global-buffer-overflow

 include/qemu/compiler.h                |  4 +++
 include/qemu/readline.h                |  1 +
 crypto/ivgen-essiv.c                   |  2 +-
 disas/s390.c                           | 16 +++++-------
 hw/nvram/ds1225y.c                     |  4 +--
 monitor.c                              |  2 +-
 tests/check-qobject.c                  |  2 ++
 tests/migration-test.c                 |  3 ++-
 tests/qmp-test.c                       |  3 ++-
 tests/test-coroutine.c                 |  1 -
 util/coroutine-ucontext.c              | 46 ++++++++++++++++++++++++++++++++++
 util/qemu-config.c                     |  3 ++-
 util/readline.c                        | 18 ++++++++++++-
 vl.c                                   |  9 ++++---
 Makefile                               |  7 ++++--
 configure                              | 23 +++++++++++++++--
 docs/devel/build-system.txt            | 13 ++++++++++
 rules.mak                              |  2 ++
 tests/docker/dockerfiles/fedora.docker |  4 +--
 tests/docker/test-clang                |  2 +-
 tests/docker/test-debug                | 26 +++++++++++++++++++
 tests/docker/test-mingw                |  2 --
 22 files changed, 162 insertions(+), 31 deletions(-)
 create mode 100755 tests/docker/test-debug

-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 01/18] build-sys: fix qemu-ga -pthread linking

[Marc-André Lureau]

When linking qemu-ga under some configuration (when gthread-2.0.pc
doesn't have -pthread, as happening atm with meson build), you may
have this linking issue:

/usr/bin/ld: libqemuutil.a(qemu-thread-posix.o): undefined reference to symbol 'pthread_setname_np@@GLIBC_2.12'
/usr/lib64/libpthread.so.0: error adding symbols: DSO missing from command line

Make sure qemu-ga links with the pthread library, by adding correct
flags to libs_qga.

This is really a QEMU bug, because it's QEMU code that's using pthread
functions, and so we must explicitly link against pthreads. The bug
was just masked by the fact that often some pkg-config or another for
one of our dependencies will add -pthread to the link line anyway.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index 100309c33f..de1f939a28 100755
--- a/configure
+++ b/configure
@@ -3445,6 +3445,7 @@ else
       done
       if test "$found" = "no"; then
         LIBS="$pthread_lib $LIBS"
+        libs_qga="$pthread_lib $libs_qga"
       fi
       PTHREAD_LIB="$pthread_lib"
       break
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 02/18] build-sys: silence make by default or V=0

[Marc-André Lureau]

Move generic make flags in MAKEFLAGS (SUBDIR_MAKEFLAGS is more qemu specific).

Use --quiet to silence make 'is up to date' message.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Tested-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Makefile  | 2 +-
 rules.mak | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index d86ecd2dd4..1671db3bdd 100644
--- a/Makefile
+++ b/Makefile
@@ -277,7 +277,7 @@ else
 DOCS=
 endif
 
-SUBDIR_MAKEFLAGS=$(if $(V),,--no-print-directory) BUILD_DIR=$(BUILD_DIR)
+SUBDIR_MAKEFLAGS=BUILD_DIR=$(BUILD_DIR)
 SUBDIR_DEVICES_MAK=$(patsubst %, %/config-devices.mak, $(TARGET_DIRS))
 SUBDIR_DEVICES_MAK_DEP=$(patsubst %, %-config-devices.mak.d, $(TARGET_DIRS))
 
diff --git a/rules.mak b/rules.mak
index 6e943335f3..5fb4951561 100644
--- a/rules.mak
+++ b/rules.mak
@@ -131,6 +131,8 @@ modules:
 # If called with only a single argument, will print nothing in quiet mode.
 quiet-command = $(if $(V),$1,$(if $(2),@printf "  %-7s %s\n" $2 $3 && $1, @$1))
 
+MAKEFLAGS += $(if $(V),,--no-print-directory --quiet)
+
 # cc-option
 # Usage: CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0)
 
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 03/18] build-sys: add a rule to print a variable

[Marc-André Lureau]

$ make print-CFLAGS
CFLAGS=-fsanitize=address -Og -g

Trick from various sources:
https://stackoverflow.com/questions/16467718/how-to-print-out-a-variable-in-makefile
https://www.cmcrossroads.com/article/printing-value-makefile-variable

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 Makefile                    |  5 ++++-
 docs/devel/build-system.txt | 13 +++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 1671db3bdd..f26ef1b1df 100644
--- a/Makefile
+++ b/Makefile
@@ -8,9 +8,12 @@ SRC_PATH=.
 
 UNCHECKED_GOALS := %clean TAGS cscope ctags dist \
     html info pdf txt \
-    help check-help \
+    help check-help print-% \
     docker docker-% vm-test vm-build-%
 
+print-%:
+	@echo '$*=$($*)'
+
 # All following code might depend on configuration variables
 ifneq ($(wildcard config-host.mak),)
 # Put the all: rule here so that config-host.mak can contain dependencies.
diff --git a/docs/devel/build-system.txt b/docs/devel/build-system.txt
index 386ef36ee3..52501f2ad9 100644
--- a/docs/devel/build-system.txt
+++ b/docs/devel/build-system.txt
@@ -510,3 +510,16 @@ default-configs/$TARGET-NAME file as input.
 This is the entrypoint used when make recurses to build a single system
 or userspace emulator target. It is merely a symlink back to the
 Makefile.target in the top level.
+
+
+Useful make targets
+===================
+
+- help
+
+  Print a help message for the most common build targets.
+
+- print-VAR
+
+  Print the value of the variable VAR. Useful for debugging the build
+  system.
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 04/18] build-sys: compile with -Og or -O1 when --enable-debug

[Marc-André Lureau]

When --enable-debug is turned on, configure doesn't set -O level, and
uses default compiler -O0 level, which is slow.

Instead, use -Og if supported by the compiler (optimize debugging
experience), or -O1 (keeps code somewhat debuggable and works around
compiler bugs).

Unfortunately, gcc has many false-positive maybe-uninitialized
errors with Og and O1 (f27 gcc 7.2.1 20170915):

/home/elmarco/src/qemu/hw/ipmi/isa_ipmi_kcs.c: In function ‘ipmi_kcs_ioport_read’:
/home/elmarco/src/qemu/hw/ipmi/isa_ipmi_kcs.c:279:12: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     return ret;
            ^~~
cc1: all warnings being treated as errors
make: *** [/home/elmarco/src/qemu/rules.mak:66: hw/ipmi/isa_ipmi_kcs.o] Error 1
make: *** Waiting for unfinished jobs....
/home/elmarco/src/qemu/hw/ide/ahci.c: In function ‘ahci_populate_sglist’:
/home/elmarco/src/qemu/hw/ide/ahci.c:903:58: error: ‘tbl_entry_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
         if ((off_idx == -1) || (off_pos < 0) || (off_pos > tbl_entry_size)) {
                                                 ~~~~~~~~~^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [/home/elmarco/src/qemu/rules.mak:66: hw/ide/ahci.o] Error 1
/home/elmarco/src/qemu/hw/display/qxl.c: In function ‘qxl_add_memslot’:
/home/elmarco/src/qemu/hw/display/qxl.c:1397:52: error: ‘pci_start’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     memslot.virt_end   = virt_start + (guest_end   - pci_start);
                                       ~~~~~~~~~~~~~^~~~~~~~~~~~
/home/elmarco/src/qemu/hw/display/qxl.c:1389:9: error: ‘pci_region’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
         qxl_set_guest_bug(d, "%s: pci_region = %d", __func__, pci_region);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

There seems to be a long list of related bugs in upstream GCC, some of
them are being fixed very recently:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=24639

For now, let's workaround it by using Wno-maybe-uninitialized (gcc-only).

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 configure | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index de1f939a28..3953859314 100755
--- a/configure
+++ b/configure
@@ -5160,8 +5160,19 @@ if test "$gcov" = "yes" ; then
   LDFLAGS="-fprofile-arcs -ftest-coverage $LDFLAGS"
 elif test "$fortify_source" = "yes" ; then
   CFLAGS="-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 $CFLAGS"
-elif test "$debug" = "no"; then
-  CFLAGS="-O2 $CFLAGS"
+elif test "$debug" = "yes"; then
+  if compile_prog "-Og" ""; then
+      CFLAGS="-Og $CFLAGS"
+  elif compile_prog "-O1" ""; then
+      CFLAGS="-O1 $CFLAGS"
+  fi
+  # Workaround GCC false-positive Wuninitialized bugs with Og or O1:
+  # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=24639
+  if cc_has_warning_flag "-Wno-maybe-uninitialized"; then
+      CFLAGS="-Wno-maybe-uninitialized $CFLAGS"
+  fi
+else
+    CFLAGS="-O2 $CFLAGS"
 fi
 
 ##########################################
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 05/18] tests/docker: add some sanitizers to fedora dockerfile

[Marc-André Lureau]

Build fedora image with ASAN/UBSan support.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/docker/dockerfiles/fedora.docker | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/docker/dockerfiles/fedora.docker b/tests/docker/dockerfiles/fedora.docker
index 4b26c3aded..32de731675 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -3,7 +3,7 @@ ENV PACKAGES \
     ccache gettext git tar PyYAML sparse flex bison python2 bzip2 hostname \
     glib2-devel pixman-devel zlib-devel SDL-devel libfdt-devel \
     gcc gcc-c++ clang make perl which bc findutils libaio-devel \
-    nettle-devel \
+    nettle-devel libasan libubsan \
     mingw32-pixman mingw32-glib2 mingw32-gmp mingw32-SDL mingw32-pkg-config \
     mingw32-gtk2 mingw32-gtk3 mingw32-gnutls mingw32-nettle mingw32-libtasn1 \
     mingw32-libjpeg-turbo mingw32-libpng mingw32-curl mingw32-libssh2 \
@@ -15,4 +15,4 @@ ENV PACKAGES \
 
 RUN dnf install -y $PACKAGES
 RUN rpm -q $PACKAGES | sort > /packages.txt
-ENV FEATURES mingw clang pyyaml
+ENV FEATURES mingw clang pyyaml asan
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 06/18] tests/docker: add test-debug

[Marc-André Lureau]

Add a new test with --enable-debug using clang/asan/ubsan, remove
--enable-debug from test-clang & test-mingw.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/docker/test-clang |  2 +-
 tests/docker/test-debug | 26 ++++++++++++++++++++++++++
 tests/docker/test-mingw |  2 --
 3 files changed, 27 insertions(+), 3 deletions(-)
 create mode 100755 tests/docker/test-debug

diff --git a/tests/docker/test-clang b/tests/docker/test-clang
index 1eb61a3af7..e90a793178 100755
--- a/tests/docker/test-clang
+++ b/tests/docker/test-clang
@@ -17,7 +17,7 @@ requires clang
 
 cd "$BUILD_DIR"
 
-OPTS="--enable-debug --cxx=clang++ --cc=clang --host-cc=clang"
+OPTS="--cxx=clang++ --cc=clang --host-cc=clang"
 # -fsanitize=undefined is broken on Fedora 23, skip it for now
 # See also: https://bugzilla.redhat.com/show_bug.cgi?id=1263834
 #OPTS="$OPTS --extra-cflags=-fsanitize=undefined \
diff --git a/tests/docker/test-debug b/tests/docker/test-debug
new file mode 100755
index 0000000000..d020b06917
--- /dev/null
+++ b/tests/docker/test-debug
@@ -0,0 +1,26 @@
+#!/bin/bash -e
+#
+# Compile and check with clang & --enable-debug.
+#
+# Copyright (c) 2016-2018 Red Hat Inc.
+#
+# Authors:
+#  Fam Zheng <famz@redhat.com>
+#  Marc-André Lureau <marcandre.lureau@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2
+# or (at your option) any later version. See the COPYING file in
+# the top-level directory.
+
+. common.rc
+
+requires clang asan
+
+cd "$BUILD_DIR"
+
+OPTS="--cxx=clang++ --cc=clang --host-cc=clang"
+OPTS="--enable-debug $OPTS"
+
+build_qemu $OPTS
+make $MAKEFLAGS check
+install_qemu
diff --git a/tests/docker/test-mingw b/tests/docker/test-mingw
index 39a1da448e..503a6bc6f7 100755
--- a/tests/docker/test-mingw
+++ b/tests/docker/test-mingw
@@ -22,7 +22,6 @@ for prefix in x86_64-w64-mingw32- i686-w64-mingw32-; do
     TARGET_LIST=${TARGET_LIST:-$DEF_TARGET_LIST} \
         build_qemu --cross-prefix=$prefix \
         --enable-trace-backends=simple \
-        --enable-debug \
         --enable-gnutls \
         --enable-nettle \
         --enable-curl \
@@ -35,4 +34,3 @@ for prefix in x86_64-w64-mingw32- i686-w64-mingw32-; do
     make clean
 
 done
-
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 07/18] build-sys: add some sanitizers when --enable-debug if possible

[Marc-André Lureau]

Enable ASAN/UBSan by default if the compiler supports it.

Typical slowdown introduced by AddressSanitizer is 2x.
UBSan shouldn't have much impact on runtime cost.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 configure | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/configure b/configure
index 3953859314..de1477c93a 100755
--- a/configure
+++ b/configure
@@ -5161,6 +5161,13 @@ if test "$gcov" = "yes" ; then
 elif test "$fortify_source" = "yes" ; then
   CFLAGS="-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 $CFLAGS"
 elif test "$debug" = "yes"; then
+  write_c_skeleton;
+  if compile_prog "-fsanitize=address" ""; then
+      CFLAGS="-fsanitize=address $CFLAGS"
+  fi
+  if compile_prog "-fsanitize=undefined" ""; then
+      CFLAGS="-fsanitize=undefined $CFLAGS"
+  fi
   if compile_prog "-Og" ""; then
       CFLAGS="-Og $CFLAGS"
   elif compile_prog "-O1" ""; then
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 08/18] tests: fix check-qobject leak

[Marc-André Lureau]

/public/qobject_is_equal_conversion: OK

=================================================================
==14396==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 56 byte(s) in 1 object(s) allocated from:
    #0 0x7f07682c5850 in malloc (/lib64/libasan.so.4+0xde850)
    #1 0x7f0767d12f0c in g_malloc ../glib/gmem.c:94
    #2 0x7f0767d131cf in g_malloc_n ../glib/gmem.c:331
    #3 0x562bd767371f in do_test_equality /home/elmarco/src/qq/tests/check-qobject.c:49
    #4 0x562bd7674a35 in qobject_is_equal_dict_test /home/elmarco/src/qq/tests/check-qobject.c:267
    #5 0x7f0767d37b04 in test_case_run ../glib/gtestutils.c:2237
    #6 0x7f0767d37ec4 in g_test_run_suite_internal ../glib/gtestutils.c:2321
    #7 0x7f0767d37f6d in g_test_run_suite_internal ../glib/gtestutils.c:2333
    #8 0x7f0767d38184 in g_test_run_suite ../glib/gtestutils.c:2408
    #9 0x7f0767d36e0d in g_test_run ../glib/gtestutils.c:1674
    #10 0x562bd7674e75 in main /home/elmarco/src/qq/tests/check-qobject.c:327
    #11 0x7f0766009039 in __libc_start_main (/lib64/libc.so.6+0x21039)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/check-qobject.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/check-qobject.c b/tests/check-qobject.c
index 03e9175113..710f9e6b0a 100644
--- a/tests/check-qobject.c
+++ b/tests/check-qobject.c
@@ -59,6 +59,8 @@ static void do_test_equality(bool expected, int _, ...)
             g_assert(qobject_is_equal(args[i], args[j]) == expected);
         }
     }
+
+    g_free(args);
 }
 
 #define check_equal(...) \
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 09/18] vl: fix direct firmware directories leak

[Marc-André Lureau]

Note that data_dir[] will now point to allocated strings.

Fixes:
Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x7f1448181850 in malloc (/lib64/libasan.so.4+0xde850)
    #1 0x7f1446ed8f0c in g_malloc ../glib/gmem.c:94
    #2 0x7f1446ed91cf in g_malloc_n ../glib/gmem.c:331
    #3 0x7f1446ef739a in g_strsplit ../glib/gstrfuncs.c:2364
    #4 0x55cf276439d7 in main /home/elmarco/src/qq/vl.c:4311
    #5 0x7f143dfad039 in __libc_start_main (/lib64/libc.so.6+0x21039)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 vl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/vl.c b/vl.c
index d3a5c5d021..8430b0c731 100644
--- a/vl.c
+++ b/vl.c
@@ -2318,7 +2318,7 @@ static void qemu_add_data_dir(const char *path)
             return; /* duplicate */
         }
     }
-    data_dir[data_dir_idx++] = path;
+    data_dir[data_dir_idx++] = g_strdup(path);
 }
 
 static inline bool nonempty_str(const char *str)
@@ -3079,7 +3079,7 @@ int main(int argc, char **argv, char **envp)
     Error *main_loop_err = NULL;
     Error *err = NULL;
     bool list_data_dirs = false;
-    char **dirs;
+    char *dir, **dirs;
     typedef struct BlockdevOptions_queue {
         BlockdevOptions *bdo;
         Location loc;
@@ -4263,9 +4263,12 @@ int main(int argc, char **argv, char **envp)
     for (i = 0; dirs[i] != NULL; i++) {
         qemu_add_data_dir(dirs[i]);
     }
+    g_strfreev(dirs);
 
     /* try to find datadir relative to the executable path */
-    qemu_add_data_dir(os_find_datadir());
+    dir = os_find_datadir();
+    qemu_add_data_dir(dir);
+    g_free(dir);
 
     /* add the datadir specified when building */
     qemu_add_data_dir(CONFIG_QEMU_DATADIR);
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 10/18] readline: add a free function

[Marc-André Lureau]

Fixes leaks such as:

Direct leak of 2 byte(s) in 1 object(s) allocated from:
    #0 0x7eff58beb850 in malloc (/lib64/libasan.so.4+0xde850)
    #1 0x7eff57942f0c in g_malloc ../glib/gmem.c:94
    #2 0x7eff579431cf in g_malloc_n ../glib/gmem.c:331
    #3 0x7eff5795f6eb in g_strdup ../glib/gstrfuncs.c:363
    #4 0x55db720f1d46 in readline_hist_add /home/elmarco/src/qq/util/readline.c:258
    #5 0x55db720f2d34 in readline_handle_byte /home/elmarco/src/qq/util/readline.c:387
    #6 0x55db71539d00 in monitor_read /home/elmarco/src/qq/monitor.c:3896
    #7 0x55db71f9be35 in qemu_chr_be_write_impl /home/elmarco/src/qq/chardev/char.c:167
    #8 0x55db71f9bed3 in qemu_chr_be_write /home/elmarco/src/qq/chardev/char.c:179
    #9 0x55db71fa013c in fd_chr_read /home/elmarco/src/qq/chardev/char-fd.c:66
    #10 0x55db71fe18a8 in qio_channel_fd_source_dispatch /home/elmarco/src/qq/io/channel-watch.c:84
    #11 0x7eff5793a90b in g_main_dispatch ../glib/gmain.c:3182
    #12 0x7eff5793b7ac in g_main_context_dispatch ../glib/gmain.c:3847
    #13 0x55db720af3bd in glib_pollfds_poll /home/elmarco/src/qq/util/main-loop.c:214
    #14 0x55db720af505 in os_host_main_loop_wait /home/elmarco/src/qq/util/main-loop.c:261
    #15 0x55db720af6d6 in main_loop_wait /home/elmarco/src/qq/util/main-loop.c:515
    #16 0x55db7184e0de in main_loop /home/elmarco/src/qq/vl.c:1995
    #17 0x55db7185e956 in main /home/elmarco/src/qq/vl.c:4914
    #18 0x7eff4ea17039 in __libc_start_main (/lib64/libc.so.6+0x21039)

(while at it, use g_new0(ReadLineState), it's a bit easier to read)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/qemu/readline.h |  1 +
 monitor.c               |  2 +-
 util/readline.c         | 18 +++++++++++++++++-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/qemu/readline.h b/include/qemu/readline.h
index c08cf7400e..e81258322b 100644
--- a/include/qemu/readline.h
+++ b/include/qemu/readline.h
@@ -59,5 +59,6 @@ ReadLineState *readline_init(ReadLinePrintfFunc *printf_func,
                              ReadLineFlushFunc *flush_func,
                              void *opaque,
                              ReadLineCompletionFunc *completion_finder);
+void readline_free(ReadLineState *rs);
 
 #endif /* READLINE_H */
diff --git a/monitor.c b/monitor.c
index d682eee2d8..b9da5e20d1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -583,7 +583,7 @@ static void monitor_data_destroy(Monitor *mon)
     if (monitor_is_qmp(mon)) {
         json_message_parser_destroy(&mon->qmp.parser);
     }
-    g_free(mon->rs);
+    readline_free(mon->rs);
     QDECREF(mon->outbuf);
     qemu_mutex_destroy(&mon->out_lock);
 }
diff --git a/util/readline.c b/util/readline.c
index bbdee790b0..24ec839854 100644
--- a/util/readline.c
+++ b/util/readline.c
@@ -500,12 +500,28 @@ const char *readline_get_history(ReadLineState *rs, unsigned int index)
     return rs->history[index];
 }
 
+void readline_free(ReadLineState *rs)
+{
+    int i;
+
+    if (!rs) {
+        return;
+    }
+    for (i = 0; i < READLINE_MAX_CMDS; i++) {
+        g_free(rs->history[i]);
+    }
+    for (i = 0; i < READLINE_MAX_COMPLETIONS; i++) {
+        g_free(rs->completions[i]);
+    }
+    g_free(rs);
+}
+
 ReadLineState *readline_init(ReadLinePrintfFunc *printf_func,
                              ReadLineFlushFunc *flush_func,
                              void *opaque,
                              ReadLineCompletionFunc *completion_finder)
 {
-    ReadLineState *rs = g_malloc0(sizeof(*rs));
+    ReadLineState *rs = g_new0(ReadLineState, 1);
 
     rs->hist_entry = -1;
     rs->opaque = opaque;
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 11/18] tests: fix migration-test leak

[Marc-André Lureau]

Direct leak of 12 byte(s) in 2 object(s) allocated from:
    #0 0x7f50d403c850 in malloc (/lib64/libasan.so.4+0xde850)
    #1 0x7f50d1ddf98f in vasprintf (/lib64/libc.so.6+0x8098f)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/migration-test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index be598d3257..799e24ebc6 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -358,13 +358,14 @@ static void migrate_check_parameter(QTestState *who, const char *parameter,
                                     const char *value)
 {
     QDict *rsp, *rsp_return;
-    const char *result;
+    char *result;
 
     rsp = wait_command(who, "{ 'execute': 'query-migrate-parameters' }");
     rsp_return = qdict_get_qdict(rsp, "return");
     result = g_strdup_printf("%" PRId64,
                              qdict_get_try_int(rsp_return,  parameter, -1));
     g_assert_cmpstr(result, ==, value);
+    g_free(result);
     QDECREF(rsp);
 }
 
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 12/18] crypto: fix stack-buffer-overflow error

[Marc-André Lureau]

ASAN complains about:

==8856==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd8a1fe168 at pc 0x561136cb4451 bp 0x7ffd8a1fe130 sp 0x7ffd8a1fd8e0
READ of size 16 at 0x7ffd8a1fe168 thread T0
    #0 0x561136cb4450 in __asan_memcpy (/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x110450)
    #1 0x561136d2a6a7 in qcrypto_ivgen_essiv_calculate /home/elmarco/src/qq/crypto/ivgen-essiv.c:83:5
    #2 0x561136d29af8 in qcrypto_ivgen_calculate /home/elmarco/src/qq/crypto/ivgen.c:72:12
    #3 0x561136d07c8e in test_ivgen /home/elmarco/src/qq/tests/test-crypto-ivgen.c:148:5
    #4 0x7f77772c3b04 in test_case_run /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2237
    #5 0x7f77772c3ec4 in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2321
    #6 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
    #7 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
    #8 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
    #9 0x7f77772c4184 in g_test_run_suite /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2408
    #10 0x7f77772c2e0d in g_test_run /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:1674
    #11 0x561136d0799b in main /home/elmarco/src/qq/tests/test-crypto-ivgen.c:173:12
    #12 0x7f77756e6039 in __libc_start_main (/lib64/libc.so.6+0x21039)
    #13 0x561136c13d89 in _start (/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x6fd89)

Address 0x7ffd8a1fe168 is located in stack of thread T0 at offset 40 in frame
    #0 0x561136d2a40f in qcrypto_ivgen_essiv_calculate /home/elmarco/src/qq/crypto/ivgen-essiv.c:76

  This frame has 1 object(s):
    [32, 40) 'sector.addr' <== Memory access at offset 40 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow (/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x110450) in __asan_memcpy
Shadow bytes around the buggy address:
  0x100031437bd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437be0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437bf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x100031437c20: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00[f3]f3 f3
  0x100031437c30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb

It looks like the rest of the code copes with ndata being larger than
sizeof(sector), so limit the memcpy() range.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
---
 crypto/ivgen-essiv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/ivgen-essiv.c b/crypto/ivgen-essiv.c
index cba20bde6c..ad4d926c19 100644
--- a/crypto/ivgen-essiv.c
+++ b/crypto/ivgen-essiv.c
@@ -79,7 +79,7 @@ static int qcrypto_ivgen_essiv_calculate(QCryptoIVGen *ivgen,
     uint8_t *data = g_new(uint8_t, ndata);
 
     sector = cpu_to_le64(sector);
-    memcpy(data, (uint8_t *)&sector, ndata);
+    memcpy(data, (uint8_t *)&sector, MIN(sizeof(sector), ndata));
     if (sizeof(sector) < ndata) {
         memset(data + sizeof(sector), 0, ndata - sizeof(sector));
     }
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 13/18] qemu-config: fix leak in query-command-line-options

[Marc-André Lureau]

Direct leak of 160 byte(s) in 4 object(s) allocated from:
    #0 0x55ed7678cda8 in calloc (/home/elmarco/src/qq/build/x86_64-softmmu/qemu-system-x86_64+0x797da8)
    #1 0x7f3f5e725f75 in g_malloc0 /home/elmarco/src/gnome/glib/builddir/../glib/gmem.c:124
    #2 0x55ed778aa3a7 in query_option_descs /home/elmarco/src/qq/util/qemu-config.c:60:16
    #3 0x55ed778aa307 in get_drive_infolist /home/elmarco/src/qq/util/qemu-config.c:140:19
    #4 0x55ed778a9f40 in qmp_query_command_line_options /home/elmarco/src/qq/util/qemu-config.c:254:36
    #5 0x55ed76d4868c in qmp_marshal_query_command_line_options /home/elmarco/src/qq/build/qmp-marshal.c:3078:14
    #6 0x55ed77855dd5 in do_qmp_dispatch /home/elmarco/src/qq/qapi/qmp-dispatch.c:104:5
    #7 0x55ed778558cc in qmp_dispatch /home/elmarco/src/qq/qapi/qmp-dispatch.c:131:11
    #8 0x55ed768b592f in handle_qmp_command /home/elmarco/src/qq/monitor.c:3840:11
    #9 0x55ed7786ccfe in json_message_process_token /home/elmarco/src/qq/qobject/json-streamer.c:105:5
    #10 0x55ed778fe37c in json_lexer_feed_char /home/elmarco/src/qq/qobject/json-lexer.c:323:13
    #11 0x55ed778fdde6 in json_lexer_feed /home/elmarco/src/qq/qobject/json-lexer.c:373:15
    #12 0x55ed7786cd83 in json_message_parser_feed /home/elmarco/src/qq/qobject/json-streamer.c:124:12
    #13 0x55ed768b559e in monitor_qmp_read /home/elmarco/src/qq/monitor.c:3882:5
    #14 0x55ed77714f29 in qemu_chr_be_write_impl /home/elmarco/src/qq/chardev/char.c:167:9
    #15 0x55ed77714fde in qemu_chr_be_write /home/elmarco/src/qq/chardev/char.c:179:9
    #16 0x55ed7772ffad in tcp_chr_read /home/elmarco/src/qq/chardev/char-socket.c:440:13
    #17 0x55ed7777113b in qio_channel_fd_source_dispatch /home/elmarco/src/qq/io/channel-watch.c:84:12
    #18 0x7f3f5e71d90b in g_main_dispatch /home/elmarco/src/gnome/glib/builddir/../glib/gmain.c:3182
    #19 0x7f3f5e71e7ac in g_main_context_dispatch /home/elmarco/src/gnome/glib/builddir/../glib/gmain.c:3847
    #20 0x55ed77886ffc in glib_pollfds_poll /home/elmarco/src/qq/util/main-loop.c:214:9
    #21 0x55ed778865fd in os_host_main_loop_wait /home/elmarco/src/qq/util/main-loop.c:261:5
    #22 0x55ed77886222 in main_loop_wait /home/elmarco/src/qq/util/main-loop.c:515:11
    #23 0x55ed76d2a4df in main_loop /home/elmarco/src/qq/vl.c:1995:9
    #24 0x55ed76d1cb4a in main /home/elmarco/src/qq/vl.c:4914:5
    #25 0x7f3f555f6039 in __libc_start_main (/lib64/libc.so.6+0x21039)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 util/qemu-config.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/qemu-config.c b/util/qemu-config.c
index 99b0e46fa3..029fec53a9 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -105,7 +105,8 @@ static void cleanup_infolist(CommandLineParameterInfoList *head)
             if (!strcmp(pre_entry->value->name, cur->next->value->name)) {
                 del_entry = cur->next;
                 cur->next = cur->next->next;
-                g_free(del_entry);
+                del_entry->next = NULL;
+                qapi_free_CommandLineParameterInfoList(del_entry);
                 break;
             }
             pre_entry = pre_entry->next;
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 14/18] tests: fix qmp-test leak

[Marc-André Lureau]

Direct leak of 913 byte(s) in 43 object(s) allocated from:
    #0 0x55880a15df60 in __interceptor_malloc (/home/elmarco/src/qq/build/tests/qmp-test+0x110f60)
    #1 0x7f3f20fd098f in _IO_vasprintf (/lib64/libc.so.6+0x8098f)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/qmp-test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index c5a5c10b41..36feb2204b 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -271,7 +271,7 @@ static void add_query_tests(QmpSchema *schema)
 {
     SchemaInfoList *tail;
     SchemaInfo *si, *arg_type, *ret_type;
-    const char *test_name;
+    char *test_name;
 
     /* Test the query-like commands */
     for (tail = schema->list; tail; tail = tail->next) {
@@ -297,6 +297,7 @@ static void add_query_tests(QmpSchema *schema)
 
         test_name = g_strdup_printf("qmp/%s", si->name);
         qtest_add_data_func(test_name, si->name, test_query);
+        g_free(test_name);
     }
 }
 
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 15/18] ucontext: annotate coroutine stack for ASAN

[Marc-André Lureau]

It helps ASAN to detect more leaks on coroutine stacks, as found in
the following patch.

A similar work would need to be done for sigaltstack & windows fibers
to have similar coverage. Since ucontext is preferred, I didn't bother
checking the other coroutine implementations for now.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/compiler.h   |  4 ++++
 util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 340e5fdc09..5fcc4f7ec7 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -111,4 +111,8 @@
 #define GCC_FMT_ATTR(n, m)
 #endif
 
+#ifndef __has_feature
+#define __has_feature(x) 0 /* compatibility with non-clang compilers */
+#endif
+
 #endif /* COMPILER_H */
diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index 6621f3f692..e78eae8766 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -31,6 +31,11 @@
 #include <valgrind/valgrind.h>
 #endif
 
+#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
+#define CONFIG_ASAN 1
+#include <sanitizer/asan_interface.h>
+#endif
+
 typedef struct {
     Coroutine base;
     void *stack;
@@ -59,11 +64,37 @@ union cc_arg {
     int i[2];
 };
 
+static void finish_switch_fiber(void *fake_stack_save)
+{
+#ifdef CONFIG_ASAN
+    const void *bottom_old;
+    size_t size_old;
+
+    __sanitizer_finish_switch_fiber(fake_stack_save, &bottom_old, &size_old);
+
+    if (!leader.stack) {
+        leader.stack = (void *)bottom_old;
+        leader.stack_size = size_old;
+    }
+#endif
+}
+
+static void start_switch_fiber(void **fake_stack_save,
+                               const void *bottom, size_t size)
+{
+#ifdef CONFIG_ASAN
+    __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
+#endif
+}
+
 static void coroutine_trampoline(int i0, int i1)
 {
     union cc_arg arg;
     CoroutineUContext *self;
     Coroutine *co;
+    void *fake_stack_save = NULL;
+
+    finish_switch_fiber(NULL);
 
     arg.i[0] = i0;
     arg.i[1] = i1;
@@ -72,9 +103,13 @@ static void coroutine_trampoline(int i0, int i1)
 
     /* Initialize longjmp environment and switch back the caller */
     if (!sigsetjmp(self->env, 0)) {
+        start_switch_fiber(&fake_stack_save,
+                           leader.stack, leader.stack_size);
         siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
     }
 
+    finish_switch_fiber(fake_stack_save);
+
     while (true) {
         co->entry(co->entry_arg);
         qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
@@ -87,6 +122,7 @@ Coroutine *qemu_coroutine_new(void)
     ucontext_t old_uc, uc;
     sigjmp_buf old_env;
     union cc_arg arg = {0};
+    void *fake_stack_save = NULL;
 
     /* The ucontext functions preserve signal masks which incurs a
      * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
@@ -122,8 +158,12 @@ Coroutine *qemu_coroutine_new(void)
 
     /* swapcontext() in, siglongjmp() back out */
     if (!sigsetjmp(old_env, 0)) {
+        start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
         swapcontext(&old_uc, &uc);
     }
+
+    finish_switch_fiber(fake_stack_save);
+
     return &co->base;
 }
 
@@ -169,13 +209,19 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
     CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
     CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
     int ret;
+    void *fake_stack_save = NULL;
 
     current = to_;
 
     ret = sigsetjmp(from->env, 0);
     if (ret == 0) {
+        start_switch_fiber(action == COROUTINE_TERMINATE ?
+                           NULL : &fake_stack_save, to->stack, to->stack_size);
         siglongjmp(to->env, action);
     }
+
+    finish_switch_fiber(fake_stack_save);
+
     return ret;
 }
 
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 16/18] tests: fix coroutine leak in /basic/entered

[Marc-André Lureau]

The coroutine is not finished by the time the test ends, resulting in
ASAN warning:

==7005==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 312 byte(s) in 1 object(s) allocated from:
    #0 0x7fd35290fa38 in __interceptor_calloc (/lib64/libasan.so.4+0xdea38)
    #1 0x7fd3506c5f75 in g_malloc0 ../glib/gmem.c:124
    #2 0x55994af03e47 in qemu_coroutine_new /home/elmarco/src/qemu/util/coroutine-ucontext.c:144
    #3 0x55994aefed99 in qemu_coroutine_create /home/elmarco/src/qemu/util/qemu-coroutine.c:76
    #4 0x55994ac1eb50 in verify_entered_step_1 /home/elmarco/src/qemu/tests/test-coroutine.c:80
    #5 0x55994af03c75 in coroutine_trampoline /home/elmarco/src/qemu/util/coroutine-ucontext.c:119
    #6 0x7fd34ec02bef  (/lib64/libc.so.6+0x50bef)

Do not yield() to let the coroutine terminate.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/test-coroutine.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/test-coroutine.c b/tests/test-coroutine.c
index abd97c23c1..76c646107e 100644
--- a/tests/test-coroutine.c
+++ b/tests/test-coroutine.c
@@ -67,7 +67,6 @@ static void coroutine_fn verify_entered_step_2(void *opaque)
     /* Once more to check it still works after yielding */
     g_assert(qemu_coroutine_entered(caller));
     g_assert(qemu_coroutine_entered(qemu_coroutine_self()));
-    qemu_coroutine_yield();
 }
 
 static void coroutine_fn verify_entered_step_1(void *opaque)
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 17/18] mips: fix potential fopen(NULL,...)

[Marc-André Lureau]

Spotted thanks to ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/nvram/ds1225y.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/nvram/ds1225y.c b/hw/nvram/ds1225y.c
index 57d5ab2154..ad7345f288 100644
--- a/hw/nvram/ds1225y.c
+++ b/hw/nvram/ds1225y.c
@@ -80,7 +80,7 @@ static int nvram_post_load(void *opaque, int version_id)
     }
 
     /* Write back nvram contents */
-    s->file = fopen(s->filename, "wb");
+    s->file = s->filename ? fopen(s->filename, "wb") : NULL;
     if (s->file) {
         /* Write back contents, as 'wb' mode cleaned the file */
         if (fwrite(s->contents, s->chip_size, 1, s->file) != 1) {
@@ -126,7 +126,7 @@ static int nvram_sysbus_initfn(SysBusDevice *dev)
     sysbus_init_mmio(dev, &s->iomem);
 
     /* Read current file */
-    file = fopen(s->filename, "rb");
+    file = s->filename ? fopen(s->filename, "rb") : NULL;
     if (file) {
         /* Read nvram contents */
         if (fread(s->contents, s->chip_size, 1, file) != 1) {
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v3 18/18] disas/s390: fix global-buffer-overflow

[Marc-André Lureau]

Spotted thanks to ASAN:

==25226==ERROR: AddressSanitizer: global-buffer-overflow on address 0x556715a1f120 at pc 0x556714b6f6b1 bp 0x7ffcdfac1360 sp 0x7ffcdfac1350
READ of size 1 at 0x556715a1f120 thread T0
    #0 0x556714b6f6b0 in init_disasm /home/elmarco/src/qemu/disas/s390.c:219
    #1 0x556714b6fa6a in print_insn_s390 /home/elmarco/src/qemu/disas/s390.c:294
    #2 0x55671484d031 in monitor_disas /home/elmarco/src/qemu/disas.c:635
    #3 0x556714862ec0 in memory_dump /home/elmarco/src/qemu/monitor.c:1324
    #4 0x55671486342a in hmp_memory_dump /home/elmarco/src/qemu/monitor.c:1418
    #5 0x5567148670be in handle_hmp_command /home/elmarco/src/qemu/monitor.c:3109
    #6 0x5567148674ed in qmp_human_monitor_command /home/elmarco/src/qemu/monitor.c:613
    #7 0x556714b00918 in qmp_marshal_human_monitor_command /home/elmarco/src/qemu/build/qmp-marshal.c:1704
    #8 0x556715138a3e in do_qmp_dispatch /home/elmarco/src/qemu/qapi/qmp-dispatch.c:104
    #9 0x556715138f83 in qmp_dispatch /home/elmarco/src/qemu/qapi/qmp-dispatch.c:131
    #10 0x55671485cf88 in handle_qmp_command /home/elmarco/src/qemu/monitor.c:3839
    #11 0x55671514e80b in json_message_process_token /home/elmarco/src/qemu/qobject/json-streamer.c:105
    #12 0x5567151bf2dc in json_lexer_feed_char /home/elmarco/src/qemu/qobject/json-lexer.c:323
    #13 0x5567151bf827 in json_lexer_feed /home/elmarco/src/qemu/qobject/json-lexer.c:373
    #14 0x55671514ee62 in json_message_parser_feed /home/elmarco/src/qemu/qobject/json-streamer.c:124
    #15 0x556714854b1f in monitor_qmp_read /home/elmarco/src/qemu/monitor.c:3881
    #16 0x556715045440 in qemu_chr_be_write_impl /home/elmarco/src/qemu/chardev/char.c:172
    #17 0x556715047184 in qemu_chr_be_write /home/elmarco/src/qemu/chardev/char.c:184
    #18 0x55671505a8e6 in tcp_chr_read /home/elmarco/src/qemu/chardev/char-socket.c:440
    #19 0x5567150943c3 in qio_channel_fd_source_dispatch /home/elmarco/src/qemu/io/channel-watch.c:84
    #20 0x7fb90292b90b in g_main_dispatch ../glib/gmain.c:3182
    #21 0x7fb90292c7ac in g_main_context_dispatch ../glib/gmain.c:3847
    #22 0x556715162eca in glib_pollfds_poll /home/elmarco/src/qemu/util/main-loop.c:214
    #23 0x556715163001 in os_host_main_loop_wait /home/elmarco/src/qemu/util/main-loop.c:261
    #24 0x5567151631fa in main_loop_wait /home/elmarco/src/qemu/util/main-loop.c:515
    #25 0x556714ad6d3b in main_loop /home/elmarco/src/qemu/vl.c:1950
    #26 0x556714ade329 in main /home/elmarco/src/qemu/vl.c:4865
    #27 0x7fb8fe5c9009 in __libc_start_main (/lib64/libc.so.6+0x21009)
    #28 0x5567147af4d9 in _start (/home/elmarco/src/qemu/build/s390x-softmmu/qemu-system-s390x+0xf674d9)

0x556715a1f120 is located 32 bytes to the left of global variable 'char_hci_type_info' defined in '/home/elmarco/src/qemu/hw/bt/hci-csr.c:493:23' (0x556715a1f140) of size 104
0x556715a1f120 is located 8 bytes to the right of global variable 's390_opcodes' defined in '/home/elmarco/src/qemu/disas/s390.c:860:33' (0x556715a15280) of size 40600

This fix is based on Andreas Arnez <arnez@linux.vnet.ibm.com> upstream
commit:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=9ace48f3d7d80ce09c5df60cccb433470410b11b

2014-08-19  Andreas Arnez  <arnez@linux.vnet.ibm.com>

       * s390-dis.c (init_disasm): Simplify initialization of
       opc_index[].  This also fixes an access after the last element
       of s390_opcodes[].

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 disas/s390.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/disas/s390.c b/disas/s390.c
index 1f167d2eaa..6393860239 100644
--- a/disas/s390.c
+++ b/disas/s390.c
@@ -207,18 +207,14 @@ static int opc_index[256];
 static void
 init_disasm (struct disassemble_info *info)
 {
-  const struct s390_opcode *opcode;
-  const struct s390_opcode *opcode_end;
+  int i;
 
   memset (opc_index, 0, sizeof (opc_index));
-  opcode_end = s390_opcodes + s390_num_opcodes;
-  for (opcode = s390_opcodes; opcode < opcode_end; opcode++)
-    {
-      opc_index[(int) opcode->opcode[0]] = opcode - s390_opcodes;
-      while ((opcode < opcode_end) &&
-	     (opcode[1].opcode[0] == opcode->opcode[0]))
-	opcode++;
-    }
+
+  /* Reverse order, such that each opc_index ends up pointing to the
+     first matching entry instead of the last.  */
+  for (i = s390_num_opcodes; i--; )
+    opc_index[s390_opcodes[i].opcode[0]] = i;
 
 #ifdef QEMU_DISABLE
   switch (info->mach)
-- 
2.15.1.355.g36791d7216

Re: [Qemu-devel] [PATCH v3 03/18] build-sys: add a rule to print a variable

[Philippe Mathieu-Daudé]

Hi Marc-André,

On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
> $ make print-CFLAGS
> CFLAGS=-fsanitize=address -Og -g

smiley mode:

$ make print-*
*=*

I didn't understood this one:

$ make print-\$
  CC      tests/qemu-iotests/socket_scm_helper.o
  LINK    tests/qemu-iotests/socket_scm_helper
  GEN     qga/qapi-generated/qga-qapi-types.h
  GEN     qga/qapi-generated/qga-qapi-visit.h
  GEN     qga/qapi-generated/qga-qmp-commands.h
  CC      qga/commands.o
  ...


> Trick from various sources:
> https://stackoverflow.com/questions/16467718/how-to-print-out-a-variable-in-makefile
> https://www.cmcrossroads.com/article/printing-value-makefile-variable
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  Makefile                    |  5 ++++-
>  docs/devel/build-system.txt | 13 +++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 1671db3bdd..f26ef1b1df 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -8,9 +8,12 @@ SRC_PATH=.
>  
>  UNCHECKED_GOALS := %clean TAGS cscope ctags dist \
>      html info pdf txt \
> -    help check-help \
> +    help check-help print-% \
>      docker docker-% vm-test vm-build-%
>  
> +print-%:
> +	@echo '$*=$($*)'
> +
>  # All following code might depend on configuration variables
>  ifneq ($(wildcard config-host.mak),)
>  # Put the all: rule here so that config-host.mak can contain dependencies.
> diff --git a/docs/devel/build-system.txt b/docs/devel/build-system.txt
> index 386ef36ee3..52501f2ad9 100644
> --- a/docs/devel/build-system.txt
> +++ b/docs/devel/build-system.txt
> @@ -510,3 +510,16 @@ default-configs/$TARGET-NAME file as input.
>  This is the entrypoint used when make recurses to build a single system
>  or userspace emulator target. It is merely a symlink back to the
>  Makefile.target in the top level.
> +
> +
> +Useful make targets
> +===================
> +
> +- help
> +
> +  Print a help message for the most common build targets.
> +
> +- print-VAR
> +
> +  Print the value of the variable VAR. Useful for debugging the build
> +  system.
> 

Re: [Qemu-devel] [PATCH v3 17/18] mips: fix potential fopen(NULL, ...)

[Philippe Mathieu-Daudé]

Hi Marc-André,

On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
> Spotted thanks to ASAN.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/nvram/ds1225y.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvram/ds1225y.c b/hw/nvram/ds1225y.c
> index 57d5ab2154..ad7345f288 100644
> --- a/hw/nvram/ds1225y.c
> +++ b/hw/nvram/ds1225y.c
> @@ -80,7 +80,7 @@ static int nvram_post_load(void *opaque, int version_id)
>      }
>  

More diffstats, but this let the code simpler imho:

   if (s->filename) {

>      /* Write back nvram contents */
> -    s->file = fopen(s->filename, "wb");
> +    s->file = s->filename ? fopen(s->filename, "wb") : NULL;
>      if (s->file) {
>          /* Write back contents, as 'wb' mode cleaned the file */
>          if (fwrite(s->contents, s->chip_size, 1, s->file) != 1) {

       ...

   }

> @@ -126,7 +126,7 @@ static int nvram_sysbus_initfn(SysBusDevice *dev)
>      sysbus_init_mmio(dev, &s->iomem);
>  
>      /* Read current file */
> -    file = fopen(s->filename, "rb");
> +    file = s->filename ? fopen(s->filename, "rb") : NULL;
>      if (file) {
>          /* Read nvram contents */
>          if (fread(s->contents, s->chip_size, 1, file) != 1) {
> 

ditto.

Re: [Qemu-devel] [PATCH v3 12/18] crypto: fix stack-buffer-overflow error

[Thomas Huth]

On 04.01.2018 17:05, Marc-André Lureau wrote:
> ASAN complains about:
> 
> ==8856==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd8a1fe168 at pc 0x561136cb4451 bp 0x7ffd8a1fe130 sp 0x7ffd8a1fd8e0
> READ of size 16 at 0x7ffd8a1fe168 thread T0
>     #0 0x561136cb4450 in __asan_memcpy (/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x110450)
>     #1 0x561136d2a6a7 in qcrypto_ivgen_essiv_calculate /home/elmarco/src/qq/crypto/ivgen-essiv.c:83:5
>     #2 0x561136d29af8 in qcrypto_ivgen_calculate /home/elmarco/src/qq/crypto/ivgen.c:72:12
>     #3 0x561136d07c8e in test_ivgen /home/elmarco/src/qq/tests/test-crypto-ivgen.c:148:5
>     #4 0x7f77772c3b04 in test_case_run /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2237
>     #5 0x7f77772c3ec4 in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2321
>     #6 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
>     #7 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
>     #8 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
>     #9 0x7f77772c4184 in g_test_run_suite /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2408
>     #10 0x7f77772c2e0d in g_test_run /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:1674
>     #11 0x561136d0799b in main /home/elmarco/src/qq/tests/test-crypto-ivgen.c:173:12
>     #12 0x7f77756e6039 in __libc_start_main (/lib64/libc.so.6+0x21039)
>     #13 0x561136c13d89 in _start (/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x6fd89)
> 
> Address 0x7ffd8a1fe168 is located in stack of thread T0 at offset 40 in frame
>     #0 0x561136d2a40f in qcrypto_ivgen_essiv_calculate /home/elmarco/src/qq/crypto/ivgen-essiv.c:76
> 
>   This frame has 1 object(s):
>     [32, 40) 'sector.addr' <== Memory access at offset 40 overflows this variable
> HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
>       (longjmp and C++ exceptions *are* supported)
> SUMMARY: AddressSanitizer: stack-buffer-overflow (/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x110450) in __asan_memcpy
> Shadow bytes around the buggy address:
>   0x100031437bd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x100031437be0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x100031437bf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x100031437c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x100031437c10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> =>0x100031437c20: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00[f3]f3 f3
>   0x100031437c30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x100031437c40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x100031437c50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x100031437c60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x100031437c70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Shadow byte legend (one shadow byte represents 8 application bytes):
>   Addressable:           00
>   Partially addressable: 01 02 03 04 05 06 07
>   Heap left redzone:       fa
>   Freed heap region:       fd
>   Stack left redzone:      f1
>   Stack mid redzone:       f2
>   Stack right redzone:     f3
>   Stack after return:      f5
>   Stack use after scope:   f8
>   Global redzone:          f9
>   Global init order:       f6
>   Poisoned by user:        f7
>   Container overflow:      fc
>   Array cookie:            ac
>   Intra object redzone:    bb
>   ASan internal:           fe
>   Left alloca redzone:     ca
>   Right alloca redzone:    cb
> 
> It looks like the rest of the code copes with ndata being larger than
> sizeof(sector), so limit the memcpy() range.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  crypto/ivgen-essiv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/crypto/ivgen-essiv.c b/crypto/ivgen-essiv.c
> index cba20bde6c..ad4d926c19 100644
> --- a/crypto/ivgen-essiv.c
> +++ b/crypto/ivgen-essiv.c
> @@ -79,7 +79,7 @@ static int qcrypto_ivgen_essiv_calculate(QCryptoIVGen *ivgen,
>      uint8_t *data = g_new(uint8_t, ndata);
>  
>      sector = cpu_to_le64(sector);
> -    memcpy(data, (uint8_t *)&sector, ndata);
> +    memcpy(data, (uint8_t *)&sector, MIN(sizeof(sector), ndata));
>      if (sizeof(sector) < ndata) {
>          memset(data + sizeof(sector), 0, ndata - sizeof(sector));
>      }
> 

Ah, funny, completely unaware of your patch series, I accidentally came
to the same conclusion two days ago:

 https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg00062.html

So if you like, feel free to add:

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

Re: [Qemu-devel] [PATCH v3 00/18] Various build-sys and sanitizer related fixes

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180104160523.22995-1-marcandre.lureau@redhat.com
Subject: [Qemu-devel] [PATCH v3 00/18] Various build-sys and sanitizer related fixes

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20180104160523.22995-1-marcandre.lureau@redhat.com -> patchew/20180104160523.22995-1-marcandre.lureau@redhat.com
 * [new tag]               patchew/20180104163652.65450-1-marcel@redhat.com -> patchew/20180104163652.65450-1-marcel@redhat.com
Switched to a new branch 'test'
96cafa2be4 disas/s390: fix global-buffer-overflow
4245ebd52a mips: fix potential fopen(NULL,...)
3bacf5b2ac tests: fix coroutine leak in /basic/entered
d40798477c ucontext: annotate coroutine stack for ASAN
2bd3966455 tests: fix qmp-test leak
919691db5a qemu-config: fix leak in query-command-line-options
736e215840 crypto: fix stack-buffer-overflow error
55ff41af25 tests: fix migration-test leak
e80f140372 readline: add a free function
645e97ba93 vl: fix direct firmware directories leak
6c79d4befa tests: fix check-qobject leak
ff1f9a1136 build-sys: add some sanitizers when --enable-debug if possible
2c8797bcad tests/docker: add test-debug
448f1c05e1 tests/docker: add some sanitizers to fedora dockerfile
7a1cf8e3a8 build-sys: compile with -Og or -O1 when --enable-debug
5085640b16 build-sys: add a rule to print a variable
2436eb64e1 build-sys: silence make by default or V=0
34bc0f9b4b build-sys: fix qemu-ga -pthread linking

=== OUTPUT BEGIN ===
Checking PATCH 1/18: build-sys: fix qemu-ga -pthread linking...
Checking PATCH 2/18: build-sys: silence make by default or V=0...
Checking PATCH 3/18: build-sys: add a rule to print a variable...
Checking PATCH 4/18: build-sys: compile with -Og or -O1 when --enable-debug...
Checking PATCH 5/18: tests/docker: add some sanitizers to fedora dockerfile...
Checking PATCH 6/18: tests/docker: add test-debug...
Checking PATCH 7/18: build-sys: add some sanitizers when --enable-debug if possible...
Checking PATCH 8/18: tests: fix check-qobject leak...
Checking PATCH 9/18: vl: fix direct firmware directories leak...
Checking PATCH 10/18: readline: add a free function...
Checking PATCH 11/18: tests: fix migration-test leak...
Checking PATCH 12/18: crypto: fix stack-buffer-overflow error...
Checking PATCH 13/18: qemu-config: fix leak in query-command-line-options...
Checking PATCH 14/18: tests: fix qmp-test leak...
Checking PATCH 15/18: ucontext: annotate coroutine stack for ASAN...
WARNING: architecture specific defines should be avoided
#29: FILE: include/qemu/compiler.h:114:
+#ifndef __has_feature

WARNING: architecture specific defines should be avoided
#42: FILE: util/coroutine-ucontext.c:34:
+#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)

total: 0 errors, 2 warnings, 107 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 16/18: tests: fix coroutine leak in /basic/entered...
Checking PATCH 17/18: mips: fix potential fopen(NULL,...)...
Checking PATCH 18/18: disas/s390: fix global-buffer-overflow...
ERROR: braces {} are necessary even for single statement blocks
#83: FILE: disas/s390.c:216:
+  for (i = s390_num_opcodes; i--; )
+    opc_index[s390_opcodes[i].opcode[0]] = i;

total: 1 errors, 0 warnings, 24 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v3 07/18] build-sys: add some sanitizers when --enable-debug if possible

[Philippe Mathieu-Daudé]

On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
> Enable ASAN/UBSan by default if the compiler supports it.
> 
> Typical slowdown introduced by AddressSanitizer is 2x.
> UBSan shouldn't have much impact on runtime cost.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

docker-debian-amd64 with QEMU_CONFIGURE_OPTS=--enable-debug:
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  configure | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/configure b/configure
> index 3953859314..de1477c93a 100755
> --- a/configure
> +++ b/configure
> @@ -5161,6 +5161,13 @@ if test "$gcov" = "yes" ; then
>  elif test "$fortify_source" = "yes" ; then
>    CFLAGS="-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 $CFLAGS"
>  elif test "$debug" = "yes"; then
> +  write_c_skeleton;
> +  if compile_prog "-fsanitize=address" ""; then
> +      CFLAGS="-fsanitize=address $CFLAGS"
> +  fi
> +  if compile_prog "-fsanitize=undefined" ""; then
> +      CFLAGS="-fsanitize=undefined $CFLAGS"
> +  fi
>    if compile_prog "-Og" ""; then
>        CFLAGS="-Og $CFLAGS"
>    elif compile_prog "-O1" ""; then
> 

Re: [Qemu-devel] [PATCH v3 12/18] crypto: fix stack-buffer-overflow error

[Philippe Mathieu-Daudé]

On 01/04/2018 01:40 PM, Thomas Huth wrote:
> On 04.01.2018 17:05, Marc-André Lureau wrote:
>> ASAN complains about:
>>
>> ==8856==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd8a1fe168 at pc 0x561136cb4451 bp 0x7ffd8a1fe130 sp 0x7ffd8a1fd8e0
>> READ of size 16 at 0x7ffd8a1fe168 thread T0
>>     #0 0x561136cb4450 in __asan_memcpy (/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x110450)
>>     #1 0x561136d2a6a7 in qcrypto_ivgen_essiv_calculate /home/elmarco/src/qq/crypto/ivgen-essiv.c:83:5
>>     #2 0x561136d29af8 in qcrypto_ivgen_calculate /home/elmarco/src/qq/crypto/ivgen.c:72:12
>>     #3 0x561136d07c8e in test_ivgen /home/elmarco/src/qq/tests/test-crypto-ivgen.c:148:5
>>     #4 0x7f77772c3b04 in test_case_run /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2237
>>     #5 0x7f77772c3ec4 in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2321
>>     #6 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
>>     #7 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
>>     #8 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
>>     #9 0x7f77772c4184 in g_test_run_suite /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2408
>>     #10 0x7f77772c2e0d in g_test_run /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:1674
>>     #11 0x561136d0799b in main /home/elmarco/src/qq/tests/test-crypto-ivgen.c:173:12
>>     #12 0x7f77756e6039 in __libc_start_main (/lib64/libc.so.6+0x21039)
>>     #13 0x561136c13d89 in _start (/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x6fd89)
>>
>> Address 0x7ffd8a1fe168 is located in stack of thread T0 at offset 40 in frame
>>     #0 0x561136d2a40f in qcrypto_ivgen_essiv_calculate /home/elmarco/src/qq/crypto/ivgen-essiv.c:76
>>
>>   This frame has 1 object(s):
>>     [32, 40) 'sector.addr' <== Memory access at offset 40 overflows this variable
>> HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
>>       (longjmp and C++ exceptions *are* supported)
>> SUMMARY: AddressSanitizer: stack-buffer-overflow (/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x110450) in __asan_memcpy
>> Shadow bytes around the buggy address:
>>   0x100031437bd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>   0x100031437be0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>   0x100031437bf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>   0x100031437c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>   0x100031437c10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> =>0x100031437c20: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00[f3]f3 f3
>>   0x100031437c30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>   0x100031437c40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>   0x100031437c50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>   0x100031437c60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>   0x100031437c70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> Shadow byte legend (one shadow byte represents 8 application bytes):
>>   Addressable:           00
>>   Partially addressable: 01 02 03 04 05 06 07
>>   Heap left redzone:       fa
>>   Freed heap region:       fd
>>   Stack left redzone:      f1
>>   Stack mid redzone:       f2
>>   Stack right redzone:     f3
>>   Stack after return:      f5
>>   Stack use after scope:   f8
>>   Global redzone:          f9
>>   Global init order:       f6
>>   Poisoned by user:        f7
>>   Container overflow:      fc
>>   Array cookie:            ac
>>   Intra object redzone:    bb
>>   ASan internal:           fe
>>   Left alloca redzone:     ca
>>   Right alloca redzone:    cb
>>
>> It looks like the rest of the code copes with ndata being larger than
>> sizeof(sector), so limit the memcpy() range.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>>  crypto/ivgen-essiv.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/crypto/ivgen-essiv.c b/crypto/ivgen-essiv.c
>> index cba20bde6c..ad4d926c19 100644
>> --- a/crypto/ivgen-essiv.c
>> +++ b/crypto/ivgen-essiv.c
>> @@ -79,7 +79,7 @@ static int qcrypto_ivgen_essiv_calculate(QCryptoIVGen *ivgen,
>>      uint8_t *data = g_new(uint8_t, ndata);
>>  
>>      sector = cpu_to_le64(sector);
>> -    memcpy(data, (uint8_t *)&sector, ndata);
>> +    memcpy(data, (uint8_t *)&sector, MIN(sizeof(sector), ndata));
>>      if (sizeof(sector) < ndata) {
>>          memset(data + sizeof(sector), 0, ndata - sizeof(sector));
>>      }
>>
> 
> Ah, funny, completely unaware of your patch series, I accidentally came
> to the same conclusion two days ago:
> 
>  https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg00062.html
> 
> So if you like, feel free to add:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Tested-by: Thomas Huth <thuth@redhat.com>

Or share a Signed-off-by? :)

Re: [Qemu-devel] [PATCH v3 05/18] tests/docker: add some sanitizers to fedora dockerfile

[Philippe Mathieu-Daudé]

On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
> Build fedora image with ASAN/UBSan support.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/docker/dockerfiles/fedora.docker | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/docker/dockerfiles/fedora.docker b/tests/docker/dockerfiles/fedora.docker
> index 4b26c3aded..32de731675 100644
> --- a/tests/docker/dockerfiles/fedora.docker
> +++ b/tests/docker/dockerfiles/fedora.docker
> @@ -3,7 +3,7 @@ ENV PACKAGES \
>      ccache gettext git tar PyYAML sparse flex bison python2 bzip2 hostname \
>      glib2-devel pixman-devel zlib-devel SDL-devel libfdt-devel \
>      gcc gcc-c++ clang make perl which bc findutils libaio-devel \
> -    nettle-devel \
> +    nettle-devel libasan libubsan \
>      mingw32-pixman mingw32-glib2 mingw32-gmp mingw32-SDL mingw32-pkg-config \
>      mingw32-gtk2 mingw32-gtk3 mingw32-gnutls mingw32-nettle mingw32-libtasn1 \
>      mingw32-libjpeg-turbo mingw32-libpng mingw32-curl mingw32-libssh2 \
> @@ -15,4 +15,4 @@ ENV PACKAGES \
>  
>  RUN dnf install -y $PACKAGES
>  RUN rpm -q $PACKAGES | sort > /packages.txt
> -ENV FEATURES mingw clang pyyaml
> +ENV FEATURES mingw clang pyyaml asan
> 

Re: [Qemu-devel] [PATCH v3 06/18] tests/docker: add test-debug

[Philippe Mathieu-Daudé]

On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
> Add a new test with --enable-debug using clang/asan/ubsan, remove
> --enable-debug from test-clang & test-mingw.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/docker/test-clang |  2 +-
>  tests/docker/test-debug | 26 ++++++++++++++++++++++++++
>  tests/docker/test-mingw |  2 --
>  3 files changed, 27 insertions(+), 3 deletions(-)
>  create mode 100755 tests/docker/test-debug
> 
> diff --git a/tests/docker/test-clang b/tests/docker/test-clang
> index 1eb61a3af7..e90a793178 100755
> --- a/tests/docker/test-clang
> +++ b/tests/docker/test-clang
> @@ -17,7 +17,7 @@ requires clang
>  
>  cd "$BUILD_DIR"
>  
> -OPTS="--enable-debug --cxx=clang++ --cc=clang --host-cc=clang"
> +OPTS="--cxx=clang++ --cc=clang --host-cc=clang"
>  # -fsanitize=undefined is broken on Fedora 23, skip it for now
>  # See also: https://bugzilla.redhat.com/show_bug.cgi?id=1263834
>  #OPTS="$OPTS --extra-cflags=-fsanitize=undefined \


> diff --git a/tests/docker/test-debug b/tests/docker/test-debug
> new file mode 100755
> index 0000000000..d020b06917
> --- /dev/null
> +++ b/tests/docker/test-debug
> @@ -0,0 +1,26 @@
> +#!/bin/bash -e
> +#
> +# Compile and check with clang & --enable-debug.
> +#
> +# Copyright (c) 2016-2018 Red Hat Inc.
> +#
> +# Authors:
> +#  Fam Zheng <famz@redhat.com>
> +#  Marc-André Lureau <marcandre.lureau@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2
> +# or (at your option) any later version. See the COPYING file in
> +# the top-level directory.
> +
> +. common.rc
> +
> +requires clang asan
> +
> +cd "$BUILD_DIR"
> +
> +OPTS="--cxx=clang++ --cc=clang --host-cc=clang"
> +OPTS="--enable-debug $OPTS"
> +
> +build_qemu $OPTS
> +make $MAKEFLAGS check
> +install_qemu


> diff --git a/tests/docker/test-mingw b/tests/docker/test-mingw
> index 39a1da448e..503a6bc6f7 100755
> --- a/tests/docker/test-mingw
> +++ b/tests/docker/test-mingw
> @@ -22,7 +22,6 @@ for prefix in x86_64-w64-mingw32- i686-w64-mingw32-; do
>      TARGET_LIST=${TARGET_LIST:-$DEF_TARGET_LIST} \
>          build_qemu --cross-prefix=$prefix \
>          --enable-trace-backends=simple \
> -        --enable-debug \
>          --enable-gnutls \
>          --enable-nettle \
>          --enable-curl \
> @@ -35,4 +34,3 @@ for prefix in x86_64-w64-mingw32- i686-w64-mingw32-; do
>      make clean
>  
>  done
> -
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [PATCH v3 04/18] build-sys: compile with -Og or -O1 when --enable-debug

[Philippe Mathieu-Daudé]

On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
> When --enable-debug is turned on, configure doesn't set -O level, and
> uses default compiler -O0 level, which is slow.
> 
> Instead, use -Og if supported by the compiler (optimize debugging
> experience), or -O1 (keeps code somewhat debuggable and works around
> compiler bugs).
> 
> Unfortunately, gcc has many false-positive maybe-uninitialized
> errors with Og and O1 (f27 gcc 7.2.1 20170915):
> 
> /home/elmarco/src/qemu/hw/ipmi/isa_ipmi_kcs.c: In function ‘ipmi_kcs_ioport_read’:
> /home/elmarco/src/qemu/hw/ipmi/isa_ipmi_kcs.c:279:12: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      return ret;
>             ^~~
> cc1: all warnings being treated as errors
> make: *** [/home/elmarco/src/qemu/rules.mak:66: hw/ipmi/isa_ipmi_kcs.o] Error 1
> make: *** Waiting for unfinished jobs....
> /home/elmarco/src/qemu/hw/ide/ahci.c: In function ‘ahci_populate_sglist’:
> /home/elmarco/src/qemu/hw/ide/ahci.c:903:58: error: ‘tbl_entry_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>          if ((off_idx == -1) || (off_pos < 0) || (off_pos > tbl_entry_size)) {
>                                                  ~~~~~~~~~^~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> make: *** [/home/elmarco/src/qemu/rules.mak:66: hw/ide/ahci.o] Error 1
> /home/elmarco/src/qemu/hw/display/qxl.c: In function ‘qxl_add_memslot’:
> /home/elmarco/src/qemu/hw/display/qxl.c:1397:52: error: ‘pci_start’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      memslot.virt_end   = virt_start + (guest_end   - pci_start);
>                                        ~~~~~~~~~~~~~^~~~~~~~~~~~
> /home/elmarco/src/qemu/hw/display/qxl.c:1389:9: error: ‘pci_region’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>          qxl_set_guest_bug(d, "%s: pci_region = %d", __func__, pci_region);
>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> There seems to be a long list of related bugs in upstream GCC, some of
> them are being fixed very recently:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=24639
> 
> For now, let's workaround it by using Wno-maybe-uninitialized (gcc-only).
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  configure | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/configure b/configure
> index de1f939a28..3953859314 100755
> --- a/configure
> +++ b/configure
> @@ -5160,8 +5160,19 @@ if test "$gcov" = "yes" ; then
>    LDFLAGS="-fprofile-arcs -ftest-coverage $LDFLAGS"
>  elif test "$fortify_source" = "yes" ; then
>    CFLAGS="-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 $CFLAGS"
> -elif test "$debug" = "no"; then
> -  CFLAGS="-O2 $CFLAGS"
> +elif test "$debug" = "yes"; then
> +  if compile_prog "-Og" ""; then
> +      CFLAGS="-Og $CFLAGS"
> +  elif compile_prog "-O1" ""; then
> +      CFLAGS="-O1 $CFLAGS"
> +  fi
> +  # Workaround GCC false-positive Wuninitialized bugs with Og or O1:
> +  # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=24639
> +  if cc_has_warning_flag "-Wno-maybe-uninitialized"; then
> +      CFLAGS="-Wno-maybe-uninitialized $CFLAGS"
> +  fi
> +else
> +    CFLAGS="-O2 $CFLAGS"
>  fi
>  
>  ##########################################
> 

Re: [Qemu-devel] [PATCH v3 11/18] tests: fix migration-test leak

[Juan Quintela]

Marc-Andre Lureau <marcandre.lureau@redhat.com> wrote:
> Direct leak of 12 byte(s) in 2 object(s) allocated from:
>     #0 0x7f50d403c850 in malloc (/lib64/libasan.so.4+0xde850)
>     #1 0x7f50d1ddf98f in vasprintf (/lib64/libc.so.6+0x8098f)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/migration-test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index be598d3257..799e24ebc6 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -358,13 +358,14 @@ static void migrate_check_parameter(QTestState *who, const char *parameter,
>                                      const char *value)
>  {
>      QDict *rsp, *rsp_return;
> -    const char *result;
> +    char *result;
>  
>      rsp = wait_command(who, "{ 'execute': 'query-migrate-parameters' }");
>      rsp_return = qdict_get_qdict(rsp, "return");
>      result = g_strdup_printf("%" PRId64,
>                               qdict_get_try_int(rsp_return,  parameter, -1));
>      g_assert_cmpstr(result, ==, value);
> +    g_free(result);
>      QDECREF(rsp);
>  }

I got the same patch, it is already on the migration pull requset.

Thanks.

Re: [Qemu-devel] [PATCH v3 00/18] Various build-sys and sanitizer related fixes

[Paolo Bonzini]

On 04/01/2018 17:05, Marc-André Lureau wrote:
> Hi,
> 
> This is a series that improves a bit the build system, and introduces
> ASAN/UBSan by default when --enable-debug. Then it fixes a few leaks
> that occur during make check: common and x86_64 target tests are leak
> free after this series. The other targets will need some work to fix
> the leaks & warnings.
> 
> v3:
> - add ubsan too with --enable-debug, since it is recommended and has
>   low runtime cost
> - add a patch "compile with -Og or -O1 when --enable-debug", suggested
>   by Paolo
> - add 2 new fixes to pass full make check with asan/ubsan
> - modify docker tests to run --enable-debug & asan with new test-debug
> - add some r-b/a-b tags
> 
> v2:
> - simplify "build-sys: silence make by default or V=0": make it a
>   oneliner MAKEFLAGS, use --quiet.
> - document print-VAR rule in docs/devel/build-system.txt
> - fix ASAN coroutine instrumentation failure
> - should fix builds on gcc 4.4.7 (centos 6)
> - new coroutine test leak fix
> - add some r-b tags
> 
> Marc-André Lureau (18):
>   build-sys: fix qemu-ga -pthread linking
>   build-sys: silence make by default or V=0
>   build-sys: add a rule to print a variable
>   build-sys: compile with -Og or -O1 when --enable-debug
>   tests/docker: add some sanitizers to fedora dockerfile
>   tests/docker: add test-debug
>   build-sys: add some sanitizers when --enable-debug if possible
>   tests: fix check-qobject leak
>   vl: fix direct firmware directories leak
>   readline: add a free function
>   tests: fix migration-test leak
>   crypto: fix stack-buffer-overflow error
>   qemu-config: fix leak in query-command-line-options
>   tests: fix qmp-test leak
>   ucontext: annotate coroutine stack for ASAN
>   tests: fix coroutine leak in /basic/entered
>   mips: fix potential fopen(NULL,...)
>   disas/s390: fix global-buffer-overflow
> 
>  include/qemu/compiler.h                |  4 +++
>  include/qemu/readline.h                |  1 +
>  crypto/ivgen-essiv.c                   |  2 +-
>  disas/s390.c                           | 16 +++++-------
>  hw/nvram/ds1225y.c                     |  4 +--
>  monitor.c                              |  2 +-
>  tests/check-qobject.c                  |  2 ++
>  tests/migration-test.c                 |  3 ++-
>  tests/qmp-test.c                       |  3 ++-
>  tests/test-coroutine.c                 |  1 -
>  util/coroutine-ucontext.c              | 46 ++++++++++++++++++++++++++++++++++
>  util/qemu-config.c                     |  3 ++-
>  util/readline.c                        | 18 ++++++++++++-
>  vl.c                                   |  9 ++++---
>  Makefile                               |  7 ++++--
>  configure                              | 23 +++++++++++++++--
>  docs/devel/build-system.txt            | 13 ++++++++++
>  rules.mak                              |  2 ++
>  tests/docker/dockerfiles/fedora.docker |  4 +--
>  tests/docker/test-clang                |  2 +-
>  tests/docker/test-debug                | 26 +++++++++++++++++++
>  tests/docker/test-mingw                |  2 --
>  22 files changed, 162 insertions(+), 31 deletions(-)
>  create mode 100755 tests/docker/test-debug
> 

Queued, thanks.

Paolo

Re: [Qemu-devel] [PATCH v3 15/18] ucontext: annotate coroutine stack for ASAN

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 5441 bytes --]

Hi Marc-André,

On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
> It helps ASAN to detect more leaks on coroutine stacks, as found in
> the following patch.
> 
> A similar work would need to be done for sigaltstack & windows fibers
> to have similar coverage. Since ucontext is preferred, I didn't bother
> checking the other coroutine implementations for now.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/qemu/compiler.h   |  4 ++++
>  util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index 340e5fdc09..5fcc4f7ec7 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -111,4 +111,8 @@
>  #define GCC_FMT_ATTR(n, m)
>  #endif
>  
> +#ifndef __has_feature
> +#define __has_feature(x) 0 /* compatibility with non-clang compilers */
> +#endif
> +
>  #endif /* COMPILER_H */
> diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
> index 6621f3f692..e78eae8766 100644
> --- a/util/coroutine-ucontext.c
> +++ b/util/coroutine-ucontext.c
> @@ -31,6 +31,11 @@
>  #include <valgrind/valgrind.h>
>  #endif
>  
> +#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
> +#define CONFIG_ASAN 1
> +#include <sanitizer/asan_interface.h>
> +#endif

Sadly this fails on Travis:

$ export CONFIG="--enable-debug --enable-debug-tcg
--enable-trace-backends=log"
$ export CC=gcc
$ ./configure ${CONFIG}
C compiler        gcc
Host C compiler   cc
C++ compiler      c++
Objective-C compiler clang
CFLAGS            -Wno-maybe-uninitialized -Og -fsanitize=address -g
[...]
  CC      util/qemu-coroutine.o
  CC      util/qemu-coroutine-lock.o
  CC      util/qemu-coroutine-io.o
  CC      util/qemu-coroutine-sleep.o
  CC      util/coroutine-ucontext.o
util/coroutine-ucontext.c:36:38: fatal error:
sanitizer/asan_interface.h: No such file or directory
 #include <sanitizer/asan_interface.h>
                                      ^
compilation terminated.
make: *** [util/coroutine-ucontext.o] Error 1

You simply need to update the .travis.yml, but does that mean your
previous patch "Enable ASAN/UBSan by default if the compiler supports
it." isn't correct?

> +
>  typedef struct {
>      Coroutine base;
>      void *stack;
> @@ -59,11 +64,37 @@ union cc_arg {
>      int i[2];
>  };
>  
> +static void finish_switch_fiber(void *fake_stack_save)
> +{
> +#ifdef CONFIG_ASAN
> +    const void *bottom_old;
> +    size_t size_old;
> +
> +    __sanitizer_finish_switch_fiber(fake_stack_save, &bottom_old, &size_old);
> +
> +    if (!leader.stack) {
> +        leader.stack = (void *)bottom_old;
> +        leader.stack_size = size_old;
> +    }
> +#endif
> +}
> +
> +static void start_switch_fiber(void **fake_stack_save,
> +                               const void *bottom, size_t size)
> +{
> +#ifdef CONFIG_ASAN
> +    __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
> +#endif
> +}
> +
>  static void coroutine_trampoline(int i0, int i1)
>  {
>      union cc_arg arg;
>      CoroutineUContext *self;
>      Coroutine *co;
> +    void *fake_stack_save = NULL;
> +
> +    finish_switch_fiber(NULL);
>  
>      arg.i[0] = i0;
>      arg.i[1] = i1;
> @@ -72,9 +103,13 @@ static void coroutine_trampoline(int i0, int i1)
>  
>      /* Initialize longjmp environment and switch back the caller */
>      if (!sigsetjmp(self->env, 0)) {
> +        start_switch_fiber(&fake_stack_save,
> +                           leader.stack, leader.stack_size);
>          siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
>      }
>  
> +    finish_switch_fiber(fake_stack_save);
> +
>      while (true) {
>          co->entry(co->entry_arg);
>          qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
> @@ -87,6 +122,7 @@ Coroutine *qemu_coroutine_new(void)
>      ucontext_t old_uc, uc;
>      sigjmp_buf old_env;
>      union cc_arg arg = {0};
> +    void *fake_stack_save = NULL;
>  
>      /* The ucontext functions preserve signal masks which incurs a
>       * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
> @@ -122,8 +158,12 @@ Coroutine *qemu_coroutine_new(void)
>  
>      /* swapcontext() in, siglongjmp() back out */
>      if (!sigsetjmp(old_env, 0)) {
> +        start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
>          swapcontext(&old_uc, &uc);
>      }
> +
> +    finish_switch_fiber(fake_stack_save);
> +
>      return &co->base;
>  }
>  
> @@ -169,13 +209,19 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
>      CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
>      CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
>      int ret;
> +    void *fake_stack_save = NULL;
>  
>      current = to_;
>  
>      ret = sigsetjmp(from->env, 0);
>      if (ret == 0) {
> +        start_switch_fiber(action == COROUTINE_TERMINATE ?
> +                           NULL : &fake_stack_save, to->stack, to->stack_size);
>          siglongjmp(to->env, action);
>      }
> +
> +    finish_switch_fiber(fake_stack_save);
> +
>      return ret;
>  }
>  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 15/18] ucontext: annotate coroutine stack for ASAN

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 5722 bytes --]

> On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
>> It helps ASAN to detect more leaks on coroutine stacks, as found in
>> the following patch.
>>
>> A similar work would need to be done for sigaltstack & windows fibers
>> to have similar coverage. Since ucontext is preferred, I didn't bother
>> checking the other coroutine implementations for now.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  include/qemu/compiler.h   |  4 ++++
>>  util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 50 insertions(+)
>>
>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>> index 340e5fdc09..5fcc4f7ec7 100644
>> --- a/include/qemu/compiler.h
>> +++ b/include/qemu/compiler.h
>> @@ -111,4 +111,8 @@
>>  #define GCC_FMT_ATTR(n, m)
>>  #endif
>>  
>> +#ifndef __has_feature
>> +#define __has_feature(x) 0 /* compatibility with non-clang compilers */
>> +#endif
>> +
>>  #endif /* COMPILER_H */
>> diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
>> index 6621f3f692..e78eae8766 100644
>> --- a/util/coroutine-ucontext.c
>> +++ b/util/coroutine-ucontext.c
>> @@ -31,6 +31,11 @@
>>  #include <valgrind/valgrind.h>
>>  #endif
>>  
>> +#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
>> +#define CONFIG_ASAN 1
>> +#include <sanitizer/asan_interface.h>
>> +#endif
> 
> Sadly this fails on Travis:
> 
> $ export CONFIG="--enable-debug --enable-debug-tcg
> --enable-trace-backends=log"
> $ export CC=gcc
> $ ./configure ${CONFIG}
> C compiler        gcc
> Host C compiler   cc
> C++ compiler      c++
> Objective-C compiler clang
> CFLAGS            -Wno-maybe-uninitialized -Og -fsanitize=address -g
> [...]
>   CC      util/qemu-coroutine.o
>   CC      util/qemu-coroutine-lock.o
>   CC      util/qemu-coroutine-io.o
>   CC      util/qemu-coroutine-sleep.o
>   CC      util/coroutine-ucontext.o
> util/coroutine-ucontext.c:36:38: fatal error:
> sanitizer/asan_interface.h: No such file or directory
>  #include <sanitizer/asan_interface.h>
>                                       ^
> compilation terminated.
> make: *** [util/coroutine-ucontext.o] Error 1
> 
> You simply need to update the .travis.yml, but does that mean your
> previous patch "Enable ASAN/UBSan by default if the compiler supports
> it." isn't correct?

Err, the previous patch subject is "build-sys: add some sanitizers when
--enable-debug if possible"

>> +
>>  typedef struct {
>>      Coroutine base;
>>      void *stack;
>> @@ -59,11 +64,37 @@ union cc_arg {
>>      int i[2];
>>  };
>>  
>> +static void finish_switch_fiber(void *fake_stack_save)
>> +{
>> +#ifdef CONFIG_ASAN
>> +    const void *bottom_old;
>> +    size_t size_old;
>> +
>> +    __sanitizer_finish_switch_fiber(fake_stack_save, &bottom_old, &size_old);
>> +
>> +    if (!leader.stack) {
>> +        leader.stack = (void *)bottom_old;
>> +        leader.stack_size = size_old;
>> +    }
>> +#endif
>> +}
>> +
>> +static void start_switch_fiber(void **fake_stack_save,
>> +                               const void *bottom, size_t size)
>> +{
>> +#ifdef CONFIG_ASAN
>> +    __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
>> +#endif
>> +}
>> +
>>  static void coroutine_trampoline(int i0, int i1)
>>  {
>>      union cc_arg arg;
>>      CoroutineUContext *self;
>>      Coroutine *co;
>> +    void *fake_stack_save = NULL;
>> +
>> +    finish_switch_fiber(NULL);
>>  
>>      arg.i[0] = i0;
>>      arg.i[1] = i1;
>> @@ -72,9 +103,13 @@ static void coroutine_trampoline(int i0, int i1)
>>  
>>      /* Initialize longjmp environment and switch back the caller */
>>      if (!sigsetjmp(self->env, 0)) {
>> +        start_switch_fiber(&fake_stack_save,
>> +                           leader.stack, leader.stack_size);
>>          siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
>>      }
>>  
>> +    finish_switch_fiber(fake_stack_save);
>> +
>>      while (true) {
>>          co->entry(co->entry_arg);
>>          qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
>> @@ -87,6 +122,7 @@ Coroutine *qemu_coroutine_new(void)
>>      ucontext_t old_uc, uc;
>>      sigjmp_buf old_env;
>>      union cc_arg arg = {0};
>> +    void *fake_stack_save = NULL;
>>  
>>      /* The ucontext functions preserve signal masks which incurs a
>>       * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
>> @@ -122,8 +158,12 @@ Coroutine *qemu_coroutine_new(void)
>>  
>>      /* swapcontext() in, siglongjmp() back out */
>>      if (!sigsetjmp(old_env, 0)) {
>> +        start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
>>          swapcontext(&old_uc, &uc);
>>      }
>> +
>> +    finish_switch_fiber(fake_stack_save);
>> +
>>      return &co->base;
>>  }
>>  
>> @@ -169,13 +209,19 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
>>      CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
>>      CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
>>      int ret;
>> +    void *fake_stack_save = NULL;
>>  
>>      current = to_;
>>  
>>      ret = sigsetjmp(from->env, 0);
>>      if (ret == 0) {
>> +        start_switch_fiber(action == COROUTINE_TERMINATE ?
>> +                           NULL : &fake_stack_save, to->stack, to->stack_size);
>>          siglongjmp(to->env, action);
>>      }
>> +
>> +    finish_switch_fiber(fake_stack_save);
>> +
>>      return ret;
>>  }
>>  
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 15/18] ucontext: annotate coroutine stack for ASAN

[Marc-André Lureau]

Hi

On Tue, Jan 9, 2018 at 12:09 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Marc-André,
>
> On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
>> It helps ASAN to detect more leaks on coroutine stacks, as found in
>> the following patch.
>>
>> A similar work would need to be done for sigaltstack & windows fibers
>> to have similar coverage. Since ucontext is preferred, I didn't bother
>> checking the other coroutine implementations for now.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  include/qemu/compiler.h   |  4 ++++
>>  util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 50 insertions(+)
>>
>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>> index 340e5fdc09..5fcc4f7ec7 100644
>> --- a/include/qemu/compiler.h
>> +++ b/include/qemu/compiler.h
>> @@ -111,4 +111,8 @@
>>  #define GCC_FMT_ATTR(n, m)
>>  #endif
>>
>> +#ifndef __has_feature
>> +#define __has_feature(x) 0 /* compatibility with non-clang compilers */
>> +#endif
>> +
>>  #endif /* COMPILER_H */
>> diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
>> index 6621f3f692..e78eae8766 100644
>> --- a/util/coroutine-ucontext.c
>> +++ b/util/coroutine-ucontext.c
>> @@ -31,6 +31,11 @@
>>  #include <valgrind/valgrind.h>
>>  #endif
>>
>> +#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
>> +#define CONFIG_ASAN 1
>> +#include <sanitizer/asan_interface.h>
>> +#endif
>
> Sadly this fails on Travis:
>
> $ export CONFIG="--enable-debug --enable-debug-tcg
> --enable-trace-backends=log"
> $ export CC=gcc
> $ ./configure ${CONFIG}
> C compiler        gcc
> Host C compiler   cc
> C++ compiler      c++
> Objective-C compiler clang
> CFLAGS            -Wno-maybe-uninitialized -Og -fsanitize=address -g
> [...]
>   CC      util/qemu-coroutine.o
>   CC      util/qemu-coroutine-lock.o
>   CC      util/qemu-coroutine-io.o
>   CC      util/qemu-coroutine-sleep.o
>   CC      util/coroutine-ucontext.o
> util/coroutine-ucontext.c:36:38: fatal error:
> sanitizer/asan_interface.h: No such file or directory
>  #include <sanitizer/asan_interface.h>
>                                       ^
> compilation terminated.
> make: *** [util/coroutine-ucontext.o] Error 1
>
> You simply need to update the .travis.yml, but does that mean your

I think we need libgcc-6-dev.

> previous patch "Enable ASAN/UBSan by default if the compiler supports
> it." isn't correct?

No, the problematic patch is "ucontext: annotate coroutine stack for
ASAN", it should probably check for asan_interface.h before using it.

Print a warning if it's not there during configure, as ASAN
experience/reports will be inferior.



>
>> +
>>  typedef struct {
>>      Coroutine base;
>>      void *stack;
>> @@ -59,11 +64,37 @@ union cc_arg {
>>      int i[2];
>>  };
>>
>> +static void finish_switch_fiber(void *fake_stack_save)
>> +{
>> +#ifdef CONFIG_ASAN
>> +    const void *bottom_old;
>> +    size_t size_old;
>> +
>> +    __sanitizer_finish_switch_fiber(fake_stack_save, &bottom_old, &size_old);
>> +
>> +    if (!leader.stack) {
>> +        leader.stack = (void *)bottom_old;
>> +        leader.stack_size = size_old;
>> +    }
>> +#endif
>> +}
>> +
>> +static void start_switch_fiber(void **fake_stack_save,
>> +                               const void *bottom, size_t size)
>> +{
>> +#ifdef CONFIG_ASAN
>> +    __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
>> +#endif
>> +}
>> +
>>  static void coroutine_trampoline(int i0, int i1)
>>  {
>>      union cc_arg arg;
>>      CoroutineUContext *self;
>>      Coroutine *co;
>> +    void *fake_stack_save = NULL;
>> +
>> +    finish_switch_fiber(NULL);
>>
>>      arg.i[0] = i0;
>>      arg.i[1] = i1;
>> @@ -72,9 +103,13 @@ static void coroutine_trampoline(int i0, int i1)
>>
>>      /* Initialize longjmp environment and switch back the caller */
>>      if (!sigsetjmp(self->env, 0)) {
>> +        start_switch_fiber(&fake_stack_save,
>> +                           leader.stack, leader.stack_size);
>>          siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
>>      }
>>
>> +    finish_switch_fiber(fake_stack_save);
>> +
>>      while (true) {
>>          co->entry(co->entry_arg);
>>          qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
>> @@ -87,6 +122,7 @@ Coroutine *qemu_coroutine_new(void)
>>      ucontext_t old_uc, uc;
>>      sigjmp_buf old_env;
>>      union cc_arg arg = {0};
>> +    void *fake_stack_save = NULL;
>>
>>      /* The ucontext functions preserve signal masks which incurs a
>>       * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
>> @@ -122,8 +158,12 @@ Coroutine *qemu_coroutine_new(void)
>>
>>      /* swapcontext() in, siglongjmp() back out */
>>      if (!sigsetjmp(old_env, 0)) {
>> +        start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
>>          swapcontext(&old_uc, &uc);
>>      }
>> +
>> +    finish_switch_fiber(fake_stack_save);
>> +
>>      return &co->base;
>>  }
>>
>> @@ -169,13 +209,19 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
>>      CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
>>      CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
>>      int ret;
>> +    void *fake_stack_save = NULL;
>>
>>      current = to_;
>>
>>      ret = sigsetjmp(from->env, 0);
>>      if (ret == 0) {
>> +        start_switch_fiber(action == COROUTINE_TERMINATE ?
>> +                           NULL : &fake_stack_save, to->stack, to->stack_size);
>>          siglongjmp(to->env, action);
>>      }
>> +
>> +    finish_switch_fiber(fake_stack_save);
>> +
>>      return ret;
>>  }
>>
>>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v3 15/18] ucontext: annotate coroutine stack for ASAN

[Marc-André Lureau]

Hi,

On Tue, Jan 9, 2018 at 12:23 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Tue, Jan 9, 2018 at 12:09 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Marc-André,
>>
>> On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
>>> It helps ASAN to detect more leaks on coroutine stacks, as found in
>>> the following patch.
>>>
>>> A similar work would need to be done for sigaltstack & windows fibers
>>> to have similar coverage. Since ucontext is preferred, I didn't bother
>>> checking the other coroutine implementations for now.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  include/qemu/compiler.h   |  4 ++++
>>>  util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 50 insertions(+)
>>>
>>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>>> index 340e5fdc09..5fcc4f7ec7 100644
>>> --- a/include/qemu/compiler.h
>>> +++ b/include/qemu/compiler.h
>>> @@ -111,4 +111,8 @@
>>>  #define GCC_FMT_ATTR(n, m)
>>>  #endif
>>>
>>> +#ifndef __has_feature
>>> +#define __has_feature(x) 0 /* compatibility with non-clang compilers */
>>> +#endif
>>> +
>>>  #endif /* COMPILER_H */
>>> diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
>>> index 6621f3f692..e78eae8766 100644
>>> --- a/util/coroutine-ucontext.c
>>> +++ b/util/coroutine-ucontext.c
>>> @@ -31,6 +31,11 @@
>>>  #include <valgrind/valgrind.h>
>>>  #endif
>>>
>>> +#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
>>> +#define CONFIG_ASAN 1
>>> +#include <sanitizer/asan_interface.h>
>>> +#endif
>>
>> Sadly this fails on Travis:
>>
>> $ export CONFIG="--enable-debug --enable-debug-tcg
>> --enable-trace-backends=log"
>> $ export CC=gcc
>> $ ./configure ${CONFIG}
>> C compiler        gcc
>> Host C compiler   cc
>> C++ compiler      c++
>> Objective-C compiler clang
>> CFLAGS            -Wno-maybe-uninitialized -Og -fsanitize=address -g
>> [...]
>>   CC      util/qemu-coroutine.o
>>   CC      util/qemu-coroutine-lock.o
>>   CC      util/qemu-coroutine-io.o
>>   CC      util/qemu-coroutine-sleep.o
>>   CC      util/coroutine-ucontext.o
>> util/coroutine-ucontext.c:36:38: fatal error:
>> sanitizer/asan_interface.h: No such file or directory
>>  #include <sanitizer/asan_interface.h>
>>                                       ^
>> compilation terminated.
>> make: *** [util/coroutine-ucontext.o] Error 1
>>
>> You simply need to update the .travis.yml, but does that mean your
>
> I think we need libgcc-6-dev.
>
>> previous patch "Enable ASAN/UBSan by default if the compiler supports
>> it." isn't correct?
>
> No, the problematic patch is "ucontext: annotate coroutine stack for
> ASAN", it should probably check for asan_interface.h before using it.
>
> Print a warning if it's not there during configure, as ASAN
> experience/reports will be inferior.
>

This should fix it:

(Paolo, would you like me to resend or will you review the diff &
squash? thanks)

diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index e78eae8766..62fce888fd 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -32,9 +32,11 @@
 #endif

 #if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
+#if HAVE_ASAN_IFACE_H
 #define CONFIG_ASAN 1
 #include <sanitizer/asan_interface.h>
 #endif
+#endif

 typedef struct {
     Coroutine base;
diff --git a/configure b/configure
index d033286b48..406ccd7cb4 100755
--- a/configure
+++ b/configure
@@ -5185,6 +5185,13 @@ if compile_prog "" "" ; then
     have_utmpx=yes
 fi

+##########################################
+# check for asan header
+have_asan_iface_h=yes
+if ! check_include "sanitizer/asan_interface.h" ; then
+    have_asan_iface_h=no
+fi
+
 ##########################################
 # End of CC checks
 # After here, no more $cc or $ld runs
@@ -5198,6 +5205,10 @@ elif test "$debug" = "yes"; then
   write_c_skeleton;
   if compile_prog "-fsanitize=address" ""; then
       CFLAGS="-fsanitize=address $CFLAGS"
+      if test "$have_asan_iface_h" = "no" ; then
+          print_error "ASAN build enabled, but ASAN header missing." \
+                      "Without code annotation, the report may be inferior."
+      fi
   fi
   if compile_prog "-fsanitize=undefined" ""; then
       CFLAGS="-fsanitize=undefined $CFLAGS"
@@ -6320,6 +6331,10 @@ if test "$have_utmpx" = "yes" ; then
   echo "HAVE_UTMPX=y" >> $config_host_mak
 fi

+if test "$have_asan_iface_h" = "yes" ; then
+    echo "HAVE_ASAN_IFACE_H=y" >> $config_host_mak
+fi
+
 if test "$ivshmem" = "yes" ; then
   echo "CONFIG_IVSHMEM=y" >> $config_host_mak
 fi


diff --git a/.travis.yml b/.travis.yml
index f583839755..f2291e87a6 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -13,12 +13,13 @@ addons:
       - libattr1-dev
       - libbrlapi-dev
       - libcap-ng-dev
+      - libgcc-6-dev
       - libgnutls-dev
       - libgtk-3-dev
       - libiscsi-dev
       - liblttng-ust-dev
-      - libnfs-dev
       - libncurses5-dev
+      - libnfs-dev
       - libnss3-dev
       - libpixman-1-dev
       - libpng12-dev

>
>
>>
>>> +
>>>  typedef struct {
>>>      Coroutine base;
>>>      void *stack;
>>> @@ -59,11 +64,37 @@ union cc_arg {
>>>      int i[2];
>>>  };
>>>
>>> +static void finish_switch_fiber(void *fake_stack_save)
>>> +{
>>> +#ifdef CONFIG_ASAN
>>> +    const void *bottom_old;
>>> +    size_t size_old;
>>> +
>>> +    __sanitizer_finish_switch_fiber(fake_stack_save, &bottom_old, &size_old);
>>> +
>>> +    if (!leader.stack) {
>>> +        leader.stack = (void *)bottom_old;
>>> +        leader.stack_size = size_old;
>>> +    }
>>> +#endif
>>> +}
>>> +
>>> +static void start_switch_fiber(void **fake_stack_save,
>>> +                               const void *bottom, size_t size)
>>> +{
>>> +#ifdef CONFIG_ASAN
>>> +    __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
>>> +#endif
>>> +}
>>> +
>>>  static void coroutine_trampoline(int i0, int i1)
>>>  {
>>>      union cc_arg arg;
>>>      CoroutineUContext *self;
>>>      Coroutine *co;
>>> +    void *fake_stack_save = NULL;
>>> +
>>> +    finish_switch_fiber(NULL);
>>>
>>>      arg.i[0] = i0;
>>>      arg.i[1] = i1;
>>> @@ -72,9 +103,13 @@ static void coroutine_trampoline(int i0, int i1)
>>>
>>>      /* Initialize longjmp environment and switch back the caller */
>>>      if (!sigsetjmp(self->env, 0)) {
>>> +        start_switch_fiber(&fake_stack_save,
>>> +                           leader.stack, leader.stack_size);
>>>          siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
>>>      }
>>>
>>> +    finish_switch_fiber(fake_stack_save);
>>> +
>>>      while (true) {
>>>          co->entry(co->entry_arg);
>>>          qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
>>> @@ -87,6 +122,7 @@ Coroutine *qemu_coroutine_new(void)
>>>      ucontext_t old_uc, uc;
>>>      sigjmp_buf old_env;
>>>      union cc_arg arg = {0};
>>> +    void *fake_stack_save = NULL;
>>>
>>>      /* The ucontext functions preserve signal masks which incurs a
>>>       * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
>>> @@ -122,8 +158,12 @@ Coroutine *qemu_coroutine_new(void)
>>>
>>>      /* swapcontext() in, siglongjmp() back out */
>>>      if (!sigsetjmp(old_env, 0)) {
>>> +        start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
>>>          swapcontext(&old_uc, &uc);
>>>      }
>>> +
>>> +    finish_switch_fiber(fake_stack_save);
>>> +
>>>      return &co->base;
>>>  }
>>>
>>> @@ -169,13 +209,19 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
>>>      CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
>>>      CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
>>>      int ret;
>>> +    void *fake_stack_save = NULL;
>>>
>>>      current = to_;
>>>
>>>      ret = sigsetjmp(from->env, 0);
>>>      if (ret == 0) {
>>> +        start_switch_fiber(action == COROUTINE_TERMINATE ?
>>> +                           NULL : &fake_stack_save, to->stack, to->stack_size);
>>>          siglongjmp(to->env, action);
>>>      }
>>> +
>>> +    finish_switch_fiber(fake_stack_save);
>>> +
>>>      return ret;
>>>  }
>>>
>>>
>>
>
>
>
> --
> Marc-André Lureau



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v3 15/18] ucontext: annotate coroutine stack for ASAN

[Paolo Bonzini]

On 12/01/2018 12:15, Marc-André Lureau wrote:
>>> You simply need to update the .travis.yml, but does that mean your
>> I think we need libgcc-6-dev.
>>
>>> previous patch "Enable ASAN/UBSan by default if the compiler supports
>>> it." isn't correct?
>> No, the problematic patch is "ucontext: annotate coroutine stack for
>> ASAN", it should probably check for asan_interface.h before using it.
>>
>> Print a warning if it's not there during configure, as ASAN
>> experience/reports will be inferior.
>>
> This should fix it:
> 
> (Paolo, would you like me to resend or will you review the diff &
> squash? thanks)

I'll integrate it, thanks.

Paolo