[Spice-commits] 3 commits - Makefile.am cfg.mk configure.ac m4/ax_valgrind_check.m4 server/Makefile.am server/reds-stream.c server/tests
Christophe Fergau
teuf at kemper.freedesktop.org
Tue Mar 21 15:35:09 UTC 2017
Makefile.am | 3
cfg.mk | 3
configure.ac | 9
m4/ax_valgrind_check.m4 | 236 +++++++++++++++++++
server/Makefile.am | 6
server/reds-stream.c | 4
server/tests/Makefile.am | 5
server/tests/test-qxl-parsing.c | 215 ++++++++++++-----
server/tests/valgrind/glib.supp | 493 ++++++++++++++++++++++++++++++++++++++++
9 files changed, 911 insertions(+), 63 deletions(-)
New commits:
commit 844544f8bea342130468ab45a404809fc26e7700
Author: Christophe Fergeau <cfergeau at redhat.com>
Date: Fri Mar 10 10:43:22 2017 +0100
build-sys: Add make check-valgrind target
This allows to run automatically our test-suite with valgrind to test
for memory leaks.
Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
Acked-by: Frediano Ziglio <fziglio at redhat.com>
diff --git a/Makefile.am b/Makefile.am
index 5272d5ae..9a073a12 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3,6 +3,9 @@ ACLOCAL_AMFLAGS = -I m4
SUBDIRS = spice-common server docs tools
+check-valgrind:
+ $(MAKE) -C server check-valgrind
+
pkgconfigdir = $(libdir)/pkgconfig
pkgconfig_DATA = spice-server.pc
diff --git a/cfg.mk b/cfg.mk
index 40683095..93d7040c 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -147,3 +147,6 @@ exclude_file_name_regexp--sc_prohibit_path_max_allocation = server/tests/test-di
exclude_file_name_regexp--sc_cast_of_argument_to_free = server/red-replay-qxl.c
exclude_file_name_regexp--sc_avoid_attribute_unused_in_header = server/stat.h
+
+# this contains a VALGRIND_CHECK_RULES occurrence wrapped in @ which is expected
+exclude_file_name_regexp--sc_makefile_at_at_check = server/tests/Makefile.am
diff --git a/configure.ac b/configure.ac
index f04585f8..9fd455b7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -116,6 +116,15 @@ AC_ARG_ENABLE([automated_tests],
AS_IF([test x"$enable_automated_tests" != "xno"], [enable_automated_tests="yes"])
AM_CONDITIONAL(HAVE_AUTOMATED_TESTS, test "x$enable_automated_tests" != "xno")
+dnl Check for the presence of Valgrind and do the plumbing to allow
+dnl the running of "make check-valgrind".
+AX_VALGRIND_DFLT(memcheck, on)
+AX_VALGRIND_DFLT(helgrind, off)
+AX_VALGRIND_DFLT(drd, off)
+AX_VALGRIND_DFLT(sgcheck, off)
+
+AX_VALGRIND_CHECK
+
SPICE_CHECK_LZ4
SPICE_CHECK_SASL
diff --git a/m4/ax_valgrind_check.m4 b/m4/ax_valgrind_check.m4
new file mode 100644
index 00000000..3761fd5e
--- /dev/null
+++ b/m4/ax_valgrind_check.m4
@@ -0,0 +1,236 @@
+# ===========================================================================
+# https://www.gnu.org/software/autoconf-archive/ax_valgrind_check.html
+# ===========================================================================
+#
+# SYNOPSIS
+#
+# AX_VALGRIND_DFLT(memcheck|helgrind|drd|sgcheck, on|off)
+# AX_VALGRIND_CHECK()
+#
+# DESCRIPTION
+#
+# AX_VALGRIND_CHECK checks whether Valgrind is present and, if so, allows
+# running `make check` under a variety of Valgrind tools to check for
+# memory and threading errors.
+#
+# Defines VALGRIND_CHECK_RULES which should be substituted in your
+# Makefile; and $enable_valgrind which can be used in subsequent configure
+# output. VALGRIND_ENABLED is defined and substituted, and corresponds to
+# the value of the --enable-valgrind option, which defaults to being
+# enabled if Valgrind is installed and disabled otherwise. Individual
+# Valgrind tools can be disabled via --disable-valgrind-<tool>, the
+# default is configurable via the AX_VALGRIND_DFLT command or is to use
+# all commands not disabled via AX_VALGRIND_DFLT. All AX_VALGRIND_DFLT
+# calls must be made before the call to AX_VALGRIND_CHECK.
+#
+# If unit tests are written using a shell script and automake's
+# LOG_COMPILER system, the $(VALGRIND) variable can be used within the
+# shell scripts to enable Valgrind, as described here:
+#
+# https://www.gnu.org/software/gnulib/manual/html_node/Running-self_002dtests-under-valgrind.html
+#
+# Usage example:
+#
+# configure.ac:
+#
+# AX_VALGRIND_DFLT([sgcheck], [off])
+# AX_VALGRIND_CHECK
+#
+# Makefile.am:
+#
+# @VALGRIND_CHECK_RULES@
+# VALGRIND_SUPPRESSIONS_FILES = my-project.supp
+# EXTRA_DIST = my-project.supp
+#
+# This results in a "check-valgrind" rule being added to any Makefile.am
+# which includes "@VALGRIND_CHECK_RULES@" (assuming the module has been
+# configured with --enable-valgrind). Running `make check-valgrind` in
+# that directory will run the module's test suite (`make check`) once for
+# each of the available Valgrind tools (out of memcheck, helgrind and drd)
+# while the sgcheck will be skipped unless enabled again on the
+# commandline with --enable-valgrind-sgcheck. The results for each check
+# will be output to test-suite-$toolname.log. The target will succeed if
+# there are zero errors and fail otherwise.
+#
+# Alternatively, a "check-valgrind-$TOOL" rule will be added, for $TOOL in
+# memcheck, helgrind, drd and sgcheck. These are useful because often only
+# some of those tools can be ran cleanly on a codebase.
+#
+# The macro supports running with and without libtool.
+#
+# LICENSE
+#
+# Copyright (c) 2014, 2015, 2016 Philip Withnall <philip.withnall at collabora.co.uk>
+#
+# Copying and distribution of this file, with or without modification, are
+# permitted in any medium without royalty provided the copyright notice
+# and this notice are preserved. This file is offered as-is, without any
+# warranty.
+
+#serial 14
+
+dnl Configured tools
+m4_define([valgrind_tool_list], [[memcheck], [helgrind], [drd], [sgcheck]])
+m4_set_add_all([valgrind_exp_tool_set], [sgcheck])
+m4_foreach([vgtool], [valgrind_tool_list],
+ [m4_define([en_dflt_valgrind_]vgtool, [on])])
+
+AC_DEFUN([AX_VALGRIND_DFLT],[
+ m4_define([en_dflt_valgrind_$1], [$2])
+])dnl
+
+AC_DEFUN([AX_VALGRIND_CHECK],[
+ dnl Check for --enable-valgrind
+ AC_ARG_ENABLE([valgrind],
+ [AS_HELP_STRING([--enable-valgrind], [Whether to enable Valgrind on the unit tests])],
+ [enable_valgrind=$enableval],[enable_valgrind=])
+
+ AS_IF([test "$enable_valgrind" != "no"],[
+ # Check for Valgrind.
+ AC_CHECK_PROG([VALGRIND],[valgrind],[valgrind])
+ AS_IF([test "$VALGRIND" = ""],[
+ AS_IF([test "$enable_valgrind" = "yes"],[
+ AC_MSG_ERROR([Could not find valgrind; either install it or reconfigure with --disable-valgrind])
+ ],[
+ enable_valgrind=no
+ ])
+ ],[
+ enable_valgrind=yes
+ ])
+ ])
+
+ AM_CONDITIONAL([VALGRIND_ENABLED],[test "$enable_valgrind" = "yes"])
+ AC_SUBST([VALGRIND_ENABLED],[$enable_valgrind])
+
+ # Check for Valgrind tools we care about.
+ [valgrind_enabled_tools=]
+ m4_foreach([vgtool],[valgrind_tool_list],[
+ AC_ARG_ENABLE([valgrind-]vgtool,
+ m4_if(m4_defn([en_dflt_valgrind_]vgtool),[off],dnl
+[AS_HELP_STRING([--enable-valgrind-]vgtool, [Whether to use ]vgtool[ during the Valgrind tests])],dnl
+[AS_HELP_STRING([--disable-valgrind-]vgtool, [Whether to skip ]vgtool[ during the Valgrind tests])]),
+ [enable_valgrind_]vgtool[=$enableval],
+ [enable_valgrind_]vgtool[=])
+ AS_IF([test "$enable_valgrind" = "no"],[
+ enable_valgrind_]vgtool[=no],
+ [test "$enable_valgrind_]vgtool[" ]dnl
+m4_if(m4_defn([en_dflt_valgrind_]vgtool), [off], [= "yes"], [!= "no"]),[
+ AC_CACHE_CHECK([for Valgrind tool ]vgtool,
+ [ax_cv_valgrind_tool_]vgtool,[
+ ax_cv_valgrind_tool_]vgtool[=no
+ m4_set_contains([valgrind_exp_tool_set],vgtool,
+ [m4_define([vgtoolx],[exp-]vgtool)],
+ [m4_define([vgtoolx],vgtool)])
+ AS_IF([`$VALGRIND --tool=]vgtoolx[ --help >/dev/null 2>&1`],[
+ ax_cv_valgrind_tool_]vgtool[=yes
+ ])
+ ])
+ AS_IF([test "$ax_cv_valgrind_tool_]vgtool[" = "no"],[
+ AS_IF([test "$enable_valgrind_]vgtool[" = "yes"],[
+ AC_MSG_ERROR([Valgrind does not support ]vgtool[; reconfigure with --disable-valgrind-]vgtool)
+ ],[
+ enable_valgrind_]vgtool[=no
+ ])
+ ],[
+ enable_valgrind_]vgtool[=yes
+ ])
+ ])
+ AS_IF([test "$enable_valgrind_]vgtool[" = "yes"],[
+ valgrind_enabled_tools="$valgrind_enabled_tools ]m4_bpatsubst(vgtool,[^exp-])["
+ ])
+ AC_SUBST([ENABLE_VALGRIND_]vgtool,[$enable_valgrind_]vgtool)
+ ])
+ AC_SUBST([valgrind_tools],["]m4_join([ ], valgrind_tool_list)["])
+ AC_SUBST([valgrind_enabled_tools],[$valgrind_enabled_tools])
+
+[VALGRIND_CHECK_RULES='
+# Valgrind check
+#
+# Optional:
+# - VALGRIND_SUPPRESSIONS_FILES: Space-separated list of Valgrind suppressions
+# files to load. (Default: empty)
+# - VALGRIND_FLAGS: General flags to pass to all Valgrind tools.
+# (Default: --num-callers=30)
+# - VALGRIND_$toolname_FLAGS: Flags to pass to Valgrind $toolname (one of:
+# memcheck, helgrind, drd, sgcheck). (Default: various)
+
+# Optional variables
+VALGRIND_SUPPRESSIONS ?= $(addprefix --suppressions=,$(VALGRIND_SUPPRESSIONS_FILES))
+VALGRIND_FLAGS ?= --num-callers=30
+VALGRIND_memcheck_FLAGS ?= --leak-check=full --show-reachable=no
+VALGRIND_helgrind_FLAGS ?= --history-level=approx
+VALGRIND_drd_FLAGS ?=
+VALGRIND_sgcheck_FLAGS ?=
+
+# Internal use
+valgrind_log_files = $(addprefix test-suite-,$(addsuffix .log,$(valgrind_tools)))
+
+valgrind_memcheck_flags = --tool=memcheck $(VALGRIND_memcheck_FLAGS)
+valgrind_helgrind_flags = --tool=helgrind $(VALGRIND_helgrind_FLAGS)
+valgrind_drd_flags = --tool=drd $(VALGRIND_drd_FLAGS)
+valgrind_sgcheck_flags = --tool=exp-sgcheck $(VALGRIND_sgcheck_FLAGS)
+
+valgrind_quiet = $(valgrind_quiet_$(V))
+valgrind_quiet_ = $(valgrind_quiet_$(AM_DEFAULT_VERBOSITY))
+valgrind_quiet_0 = --quiet
+valgrind_v_use = $(valgrind_v_use_$(V))
+valgrind_v_use_ = $(valgrind_v_use_$(AM_DEFAULT_VERBOSITY))
+valgrind_v_use_0 = @echo " USE " $(patsubst check-valgrind-%,%,$''@):;
+
+# Support running with and without libtool.
+ifneq ($(LIBTOOL),)
+valgrind_lt = $(LIBTOOL) $(AM_LIBTOOLFLAGS) $(LIBTOOLFLAGS) --mode=execute
+else
+valgrind_lt =
+endif
+
+# Use recursive makes in order to ignore errors during check
+check-valgrind:
+ifeq ($(VALGRIND_ENABLED),yes)
+ -$(A''M_V_at)$(foreach tool,$(valgrind_enabled_tools), \
+ $(MAKE) $(AM_MAKEFLAGS) -k check-valgrind-$(tool); \
+ )
+else
+ @echo "Need to reconfigure with --enable-valgrind"
+endif
+
+# Valgrind running
+VALGRIND_TESTS_ENVIRONMENT = \
+ $(TESTS_ENVIRONMENT) \
+ env VALGRIND=$(VALGRIND) \
+ G_SLICE=always-malloc,debug-blocks \
+ G_DEBUG=fatal-warnings,fatal-criticals,gc-friendly
+
+VALGRIND_LOG_COMPILER = \
+ $(valgrind_lt) \
+ $(VALGRIND) $(VALGRIND_SUPPRESSIONS) --error-exitcode=1 $(VALGRIND_FLAGS)
+
+define valgrind_tool_rule =
+check-valgrind-$(1):
+ifeq ($$(VALGRIND_ENABLED)-$$(ENABLE_VALGRIND_$(1)),yes-yes)
+ $$(valgrind_v_use)$$(MAKE) check-TESTS \
+ TESTS_ENVIRONMENT="$$(VALGRIND_TESTS_ENVIRONMENT)" \
+ LOG_COMPILER="$$(VALGRIND_LOG_COMPILER)" \
+ LOG_FLAGS="$$(valgrind_$(1)_flags)" \
+ TEST_SUITE_LOG=test-suite-$(1).log
+else ifeq ($$(VALGRIND_ENABLED),yes)
+ @echo "Need to reconfigure with --enable-valgrind-$(1)"
+else
+ @echo "Need to reconfigure with --enable-valgrind"
+endif
+endef
+
+$(foreach tool,$(valgrind_tools),$(eval $(call valgrind_tool_rule,$(tool))))
+
+A''M_DISTCHECK_CONFIGURE_FLAGS ?=
+A''M_DISTCHECK_CONFIGURE_FLAGS += --disable-valgrind
+
+MOSTLYCLEANFILES ?=
+MOSTLYCLEANFILES += $(valgrind_log_files)
+
+.PHONY: check-valgrind $(add-prefix check-valgrind-,$(valgrind_tools))
+']
+
+ AC_SUBST([VALGRIND_CHECK_RULES])
+ m4_ifdef([_AM_SUBST_NOTMAKE], [_AM_SUBST_NOTMAKE([VALGRIND_CHECK_RULES])])
+])
diff --git a/server/Makefile.am b/server/Makefile.am
index 840f63e6..772d8197 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -1,6 +1,12 @@
NULL =
SUBDIRS = . tests
+# Use 'check-valgrind-memcheck' rather than 'check-valgrind' to get a
+# non-0 exit code on failures, see
+# https://savannah.gnu.org/patch/index.php?9286
+check-valgrind:
+ $(MAKE) -C tests check-valgrind-memcheck
+
AM_CPPFLAGS = \
-DSPICE_SERVER_INTERNAL \
$(COMMON_CFLAGS) \
diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
index fc9e1f23..6cd6f493 100644
--- a/server/tests/Makefile.am
+++ b/server/tests/Makefile.am
@@ -1,5 +1,9 @@
NULL =
+ at VALGRIND_CHECK_RULES@
+VALGRIND_SUPPRESSIONS_FILES = $(top_srcdir)/server/tests/valgrind/glib.supp
+EXTRA_DIST = $(VALGRIND_SUPPRESSIONS_FILES)
+
AM_CPPFLAGS = \
-DSPICE_TOP_SRCDIR=\"$(abs_top_srcdir)\"\
-I$(top_srcdir) \
diff --git a/server/tests/valgrind/glib.supp b/server/tests/valgrind/glib.supp
new file mode 100644
index 00000000..ccfab67d
--- /dev/null
+++ b/server/tests/valgrind/glib.supp
@@ -0,0 +1,493 @@
+# GLib Valgrind suppressions file
+#
+# This provides a list of suppressions for all of GLib (including GIO), for all
+# Valgrind tools (memcheck, drd, helgrind, etc.) for the false positives and
+# deliberate one-time leaks which GLib causes to be reported when running under
+# Valgrind.
+#
+# When running an application which links to GLib under Valgrind, you can pass
+# this suppression file to Valgrind using --suppressions=/path/to/glib-2.0.supp.
+#
+# http://valgrind.org/docs/manual/manual-core.html#manual-core.suppress
+#
+# Note that there is currently no way for Valgrind to load this automatically
+# (https://bugs.kde.org/show_bug.cgi?id=160905), so the best GLib can currently
+# do is to install this file as part of its development package.
+#
+# This file should be updated if GLib introduces a new deliberate one-time leak,
+# or another false race positive in Valgrind: please file bugs at:
+#
+# https://bugzilla.gnome.org/enter_bug.cgi?product=glib
+
+{
+ gnutls-init-calloc
+ Memcheck:Leak
+ fun:calloc
+ ...
+ fun:gtls_gnutls_init
+}
+
+{
+ gnutls-init-realloc
+ Memcheck:Leak
+ fun:realloc
+ ...
+ fun:gtls_gnutls_init
+}
+
+{
+ g-tls-backend-gnutls-init
+ Memcheck:Leak
+ fun:g_once_impl
+ fun:g_tls_backend_gnutls_init
+}
+
+{
+ p11-tokens-init
+ Memcheck:Leak
+ fun:calloc
+ ...
+ fun:create_tokens_inlock
+ fun:initialize_module_inlock_reentrant
+}
+
+{
+ gobject-init-malloc
+ Memcheck:Leak
+ fun:malloc
+ ...
+ fun:gobject_init_ctor
+}
+
+{
+ gobject-init-realloc
+ Memcheck:Leak
+ fun:realloc
+ ...
+ fun:gobject_init_ctor
+}
+
+{
+ gobject-init-calloc
+ Memcheck:Leak
+ fun:calloc
+ ...
+ fun:gobject_init_ctor
+}
+
+{
+ g-type-register-dynamic
+ Memcheck:Leak
+ fun:malloc
+ ...
+ fun:g_type_register_dynamic
+}
+
+{
+ g-type-register-static
+ Memcheck:Leak
+ fun:malloc
+ ...
+ fun:g_type_register_static
+}
+
+{
+ g-type-register-static-realloc
+ Memcheck:Leak
+ fun:realloc
+ ...
+ fun:g_type_register_static
+}
+
+{
+ g-type-register-static-calloc
+ Memcheck:Leak
+ fun:calloc
+ ...
+ fun:g_type_register_static
+}
+
+{
+ g-type-add-interface-dynamic
+ Memcheck:Leak
+ fun:malloc
+ ...
+ fun:g_type_add_interface_dynamic
+}
+
+{
+ g-type-add-interface-static
+ Memcheck:Leak
+ fun:malloc
+ ...
+ fun:g_type_add_interface_static
+}
+
+{
+ g-test-rand-init
+ Memcheck:Leak
+ fun:calloc
+ ...
+ fun:g_rand_new_with_seed_array
+ fun:test_run_seed
+ ...
+ fun:g_test_run
+}
+
+{
+ g-test-rand-init2
+ Memcheck:Leak
+ fun:calloc
+ ...
+ fun:g_rand_new_with_seed_array
+ ...
+ fun:get_global_random
+ ...
+ fun:g_test_init
+}
+
+{
+ g-quark-table-new
+ Memcheck:Leak
+ fun:g_hash_table_new
+ ...
+ fun:quark_new
+}
+
+{
+ g-quark-table-resize
+ Memcheck:Leak
+ fun:g_hash_table_resize
+ ...
+ fun:quark_new
+}
+
+{
+ g-type-interface-init
+ Memcheck:Leak
+ fun:malloc
+ ...
+ fun:type_iface_vtable_base_init_Wm
+}
+
+{
+ g-type-class-init
+ Memcheck:Leak
+ fun:calloc
+ ...
+ fun:type_class_init_Wm
+}
+
+{
+ g-io-module-default-singleton-malloc
+ Memcheck:Leak
+ fun:malloc
+ ...
+ fun:g_type_create_instance
+ ...
+ fun:_g_io_module_get_default
+}
+
+{
+ g-io-module-default-singleton-module
+ Memcheck:Leak
+ fun:calloc
+ ...
+ fun:g_module_open
+ ...
+ fun:_g_io_module_get_default
+}
+
+{
+ g-get-language-names
+ Memcheck:Leak
+ fun:malloc
+ ...
+ fun:g_get_language_names
+}
+
+{
+ g-static-mutex
+ Memcheck:Leak
+ fun:malloc
+ ...
+ fun:g_static_mutex_get_mutex_impl
+}
+
+{
+ g-system-thread-init
+ Memcheck:Leak
+ fun:calloc
+ ...
+ fun:g_system_thread_new
+}
+
+{
+ g-io-module-default-proxy-resolver-gnome
+ Memcheck:Leak
+ fun:calloc
+ ...
+ fun:g_proxy_resolver_gnome_init
+ ...
+ fun:_g_io_module_get_default
+}
+
+{
+ g-private-get
+ drd:ConflictingAccess
+ fun:g_private_get
+}
+{
+ g-private-get-helgrind
+ Helgrind:Race
+ fun:g_private_get
+}
+
+
+{
+ g-private-set
+ drd:ConflictingAccess
+ fun:g_private_set
+}
+{
+ g-private-set-helgrind
+ Helgrind:Race
+ fun:g_private_set
+}
+
+{
+ g-type-construct-free
+ drd:ConflictingAccess
+ fun:g_type_free_instance
+}
+{
+ g-type-construct-free-helgrind
+ Helgrind:Race
+ fun:g_type_free_instance
+}
+
+{
+ g-variant-unref
+ drd:ConflictingAccess
+ fun:g_variant_unref
+}
+{
+ g-variant-unref-helgrind
+ Helgrind:Race
+ fun:g_variant_unref
+}
+
+{
+ g-unix-signals-main
+ drd:ConflictingAccess
+ fun:_g_main_create_unix_signal_watch
+}
+{
+ g-unix-signals-dispatch
+ drd:ConflictingAccess
+ ...
+ fun:dispatch_unix_signals*
+}
+{
+ g-unix-signals-dispatch-helgrind
+ Helgrind:Race
+ ...
+ fun:dispatch_unix_signals*
+}
+{
+ g-unix-signals-other
+ drd:ConflictingAccess
+ fun:g_unix_signal_watch*
+}
+{
+ g-unix-signals-other-helgrind
+ Helgrind:Race
+ fun:g_unix_signal_watch*
+}
+{
+ g-unix-signals-handler
+ drd:ConflictingAccess
+ fun:g_unix_signal_handler*
+}
+{
+ g-unix-signals-handler-helgrind
+ Helgrind:Race
+ fun:g_unix_signal_handler*
+}
+{
+ g-unix-signals-worker
+ drd:ConflictingAccess
+ fun:glib_worker_main
+}
+{
+ g-unix-signals-worker-helgrind
+ Helgrind:Race
+ fun:glib_worker_main
+}
+
+{
+ g-wakeup-acknowledge
+ drd:ConflictingAccess
+ fun:read
+ fun:g_wakeup_acknowledge
+}
+
+{
+ g-type-fundamental
+ drd:ConflictingAccess
+ fun:g_type_fundamental
+}
+{
+ g-type-fundamental-helgrind
+ Helgrind:Race
+ fun:g_type_fundamental
+}
+{
+ g-type-class-peek-static
+ drd:ConflictingAccess
+ fun:g_type_class_peek_static
+}
+{
+ g-type-class-peek-static-helgrind
+ Helgrind:Race
+ fun:g_type_class_peek_static
+}
+{
+ g-type-is-a
+ drd:ConflictingAccess
+ ...
+ fun:g_type_is_a
+}
+{
+ g-type-is-a-helgrind
+ Helgrind:Race
+ ...
+ fun:g_type_is_a
+}
+
+{
+ g-inet-address-get-type
+ drd:ConflictingAccess
+ fun:g_inet_address_get_type
+}
+{
+ g-inet-address-get-type-helgrind
+ Helgrind:Race
+ fun:g_inet_address_get_type
+}
+
+# From: https://github.com/fredericgermain/valgrind/blob/master/glibc-2.X-drd.supp
+{
+ drd-libc-stdio
+ drd:ConflictingAccess
+ obj:*/lib*/libc-*
+}
+{
+ drd-libc-recv
+ drd:ConflictingAccess
+ fun:recv
+}
+{
+ drd-libc-send
+ drd:ConflictingAccess
+ fun:send
+}
+
+# GSources do an opportunistic ref count check
+{
+ g-source-set-ready-time
+ drd:ConflictingAccess
+ fun:g_source_set_ready_time
+}
+{
+ g-source-set-ready-time-helgrind
+ Helgrind:Race
+ fun:g_source_set_ready_time
+}
+
+{
+ g-source-iter-next
+ Helgrind:Race
+ fun:g_source_iter_next
+ fun:g_main_context_*
+ fun:g_main_context_iterate
+}
+
+{
+ g-object-instance-private
+ drd:ConflictingAccess
+ fun:*_get_instance_private
+}
+{
+ g-object-instance-private-helgrind
+ Helgrind:Race
+ fun:*_get_instance_private
+}
+
+# GLib legitimately calls pthread_cond_signal without a mutex held
+{
+ g-task-thread-complete
+ drd:CondErr
+ ...
+ fun:g_cond_signal
+ fun:g_task_thread_complete
+}
+{
+ g-task-thread-complete
+ Helgrind:Misc
+ ...
+ fun:g_cond_signal
+ fun:g_task_thread_complete
+}
+
+# False positive, but I can't explain how (FIXME)
+{
+ g-task-cond
+ Helgrind:Misc
+ ...
+ fun:g_cond_clear
+ fun:g_task_finalize
+}
+
+# Real race, but is_cancelled() is an opportunistic function anyway
+{
+ g-cancellable-is-cancelled
+ Helgrind:Race
+ fun:g_cancellable_is_cancelled
+}
+
+# False positive
+{
+ g-main-context-cond
+ Helgrind:Misc
+ ...
+ fun:g_cond_clear
+ fun:g_main_context_unref
+}
+
+# False positives
+{
+ g-source-unlocked
+ Helgrind:Race
+ fun:g_source_*_unlocked
+}
+{
+ g-source-internal
+ Helgrind:Race
+ fun:g_source_*_internal
+}
+
+# False positive
+{
+ g_object_real_dispose
+ Helgrind:Race
+ fun:g_object_real_dispose
+}
+
+# False positive
+{
+ g_object_new_valist
+ Helgrind:Race
+ ...
+ fun:g_object_new_valist
+}
commit 02d42d4f00e48517869efc721abc7c0f1d68299f
Author: Christophe Fergeau <cfergeau at redhat.com>
Date: Tue Mar 14 17:56:51 2017 +0100
tests: Port test-qxl-parsing to GTest
test-qxl-parsing is really a series of several tests. Porting it to
GTest makes this more obvious. This also has the side-effect of making
it more friendly to 'make check-valgrind' (which would fail on SIGALRM,
and on unexpected g_warning()).
Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
Acked-by: Frediano Ziglio <fziglio at redhat.com>
diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
index 3f61faab..fc9e1f23 100644
--- a/server/tests/Makefile.am
+++ b/server/tests/Makefile.am
@@ -124,6 +124,7 @@ test_vdagent_CPPFLAGS = \
-UGLIB_VERSION_MAX_ALLOWED \
$(NULL)
test_codecs_parsing_CPPFLAGS = $(test_vdagent_CPPFLAGS)
+test_qxl_parsing_CPPFLAGS = $(test_vdagent_CPPFLAGS)
if HAVE_GSTREAMER
test_gst_SOURCES = test-gst.c \
diff --git a/server/tests/test-qxl-parsing.c b/server/tests/test-qxl-parsing.c
index dd991503..5cbcf487 100644
--- a/server/tests/test-qxl-parsing.c
+++ b/server/tests/test-qxl-parsing.c
@@ -27,35 +27,22 @@
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
-#include <assert.h>
#include <spice/macros.h>
#include "memslot.h"
#include "red-parse-qxl.h"
+#include "test-glib-compat.h"
-static int exit_code = EXIT_SUCCESS;
-static const char *test_name = NULL;
-
-static void
-failure(void)
+static QXLPHYSICAL
+to_physical(const void *ptr)
{
- assert(test_name);
- printf("Test %s FAILED!\n", test_name);
- exit_code = EXIT_FAILURE;
+ return (uintptr_t) ptr;
}
-static void
-test(const char *desc)
+static void *
+from_physical(QXLPHYSICAL physical)
{
- test_name = desc;
- printf("Starting test %s\n", desc);
- alarm(5);
-}
-
-static inline QXLPHYSICAL
-to_physical(const void *ptr)
-{
- return (uintptr_t) ptr;
+ return (void *)(uintptr_t) physical;
}
static void*
@@ -71,45 +58,77 @@ create_chunk(size_t prefix, uint32_t size, QXLDataChunk* prev, int fill)
return ptr;
}
-int main(int argc, char **argv)
+static void init_meminfo(RedMemSlotInfo *mem_info)
{
- RedMemSlotInfo mem_info;
- memslot_info_init(&mem_info, 1 /* groups */, 1 /* slots */, 1, 1, 0);
- memslot_info_add_slot(&mem_info, 0, 0, 0 /* delta */, 0 /* start */, ~0ul /* end */, 0 /* generation */);
+ memslot_info_init(mem_info, 1 /* groups */, 1 /* slots */, 1, 1, 0);
+ memslot_info_add_slot(mem_info, 0, 0, 0 /* delta */, 0 /* start */, ~0ul /* end */, 0 /* generation */);
+}
+
+static void init_qxl_surface(QXLSurfaceCmd *qxl)
+{
+ void *surface_mem;
+ memset(qxl, 0, sizeof(*qxl));
+
+ qxl->surface_id = 123;
+
+ qxl->u.surface_create.format = SPICE_SURFACE_FMT_32_xRGB;
+ qxl->u.surface_create.width = 128;
+ qxl->u.surface_create.stride = 512;
+ qxl->u.surface_create.height = 128;
+ surface_mem = malloc(0x10000);
+ qxl->u.surface_create.data = to_physical(surface_mem);
+}
+
+static void deinit_qxl_surface(QXLSurfaceCmd *qxl)
+{
+ free(from_physical(qxl->u.surface_create.data));
+}
+
+static void test_no_issues(void)
+{
+ RedMemSlotInfo mem_info;
RedSurfaceCmd cmd;
QXLSurfaceCmd qxl;
- RedCursorCmd red_cursor_cmd;
- QXLCursorCmd cursor_cmd;
- QXLCursor *cursor;
- QXLDataChunk *chunks[2];
+ init_meminfo(&mem_info);
+ init_qxl_surface(&qxl);
- void *surface_mem;
+ /* try to create a surface with no issues, should succeed */
+ g_assert_false(red_get_surface_cmd(&mem_info, 0, &cmd, to_physical(&qxl)));
- memset(&qxl, 0, sizeof(qxl));
+ deinit_qxl_surface(&qxl);
+ memslot_info_destroy(&mem_info);
+}
- qxl.surface_id = 123;
+static void test_stride_too_small(void)
+{
+ RedMemSlotInfo mem_info;
+ RedSurfaceCmd cmd;
+ QXLSurfaceCmd qxl;
- /* try to create a surface with no issues, should succeed */
- test("no issues");
- qxl.u.surface_create.format = SPICE_SURFACE_FMT_32_xRGB;
- qxl.u.surface_create.width = 128;
- qxl.u.surface_create.stride = 512;
- qxl.u.surface_create.height = 128;
- surface_mem = malloc(0x10000);
- qxl.u.surface_create.data = to_physical(surface_mem);
- if (red_get_surface_cmd(&mem_info, 0, &cmd, to_physical(&qxl)))
- failure();
+ init_meminfo(&mem_info);
+ init_qxl_surface(&qxl);
/* try to create a surface with a stride too small to fit
* the entire width.
* This can be used to cause buffer overflows so refuse it.
*/
- test("stride too small");
qxl.u.surface_create.stride = 256;
- if (!red_get_surface_cmd(&mem_info, 0, &cmd, to_physical(&qxl)))
- failure();
+ g_assert_true(red_get_surface_cmd(&mem_info, 0, &cmd, to_physical(&qxl)));
+
+ deinit_qxl_surface(&qxl);
+ memslot_info_destroy(&mem_info);
+}
+
+static void test_too_big_image(void)
+{
+ RedMemSlotInfo mem_info;
+ RedSurfaceCmd cmd;
+ QXLSurfaceCmd qxl;
+
+ init_meminfo(&mem_info);
+ init_qxl_surface(&qxl);
/* try to create a surface quite large.
* The sizes (width and height) were chosen so the multiplication
@@ -118,15 +137,25 @@ int main(int argc, char **argv)
* overflows. Also the total memory for the card is not enough to
* hold the surface so surely can't be accepted.
*/
- test("too big image");
qxl.u.surface_create.stride = 0x08000004 * 4;
qxl.u.surface_create.width = 0x08000004;
qxl.u.surface_create.height = 0x40000020;
- if (!red_get_surface_cmd(&mem_info, 0, &cmd, to_physical(&qxl)))
- failure();
+ g_assert_true(red_get_surface_cmd(&mem_info, 0, &cmd, to_physical(&qxl)));
+
+ deinit_qxl_surface(&qxl);
+ memslot_info_destroy(&mem_info);
+}
+
+static void test_cursor_command(void)
+{
+ RedMemSlotInfo mem_info;
+ RedCursorCmd red_cursor_cmd;
+ QXLCursorCmd cursor_cmd;
+ QXLCursor *cursor;
+
+ init_meminfo(&mem_info);
/* test base cursor with no problems */
- test("base cursor command");
memset(&cursor_cmd, 0, sizeof(cursor_cmd));
cursor_cmd.type = QXL_CURSOR_SET;
@@ -138,13 +167,25 @@ int main(int argc, char **argv)
cursor_cmd.u.set.shape = to_physical(cursor);
- if (red_get_cursor_cmd(&mem_info, 0, &red_cursor_cmd, to_physical(&cursor_cmd)))
- failure();
+ g_assert_false(red_get_cursor_cmd(&mem_info, 0, &red_cursor_cmd, to_physical(&cursor_cmd)));
free(red_cursor_cmd.u.set.shape.data);
free(cursor);
+ memslot_info_destroy(&mem_info);
+}
+
+static void test_circular_empty_chunks(void)
+{
+ RedMemSlotInfo mem_info;
+ RedCursorCmd red_cursor_cmd;
+ QXLCursorCmd cursor_cmd;
+ QXLCursor *cursor;
+ QXLDataChunk *chunks[2];
+
+ init_meminfo(&mem_info);
+ g_test_expect_message(G_LOG_DOMAIN, G_LOG_LEVEL_WARNING,
+ "*red_get_data_chunks_ptr: data split in too many chunks, avoiding DoS*");
/* a circular list of empty chunks should not be a problems */
- test("circular empty chunks");
memset(&cursor_cmd, 0, sizeof(cursor_cmd));
cursor_cmd.type = QXL_CURSOR_SET;
@@ -162,16 +203,31 @@ int main(int argc, char **argv)
memset(&red_cursor_cmd, 0xaa, sizeof(red_cursor_cmd));
if (!red_get_cursor_cmd(&mem_info, 0, &red_cursor_cmd, to_physical(&cursor_cmd))) {
/* function does not return errors so there should be no data */
- assert(red_cursor_cmd.type == QXL_CURSOR_SET);
- assert(red_cursor_cmd.u.set.position.x == 0);
- assert(red_cursor_cmd.u.set.position.y == 0);
- assert(red_cursor_cmd.u.set.shape.data_size == 0);
+ g_assert_cmpuint(red_cursor_cmd.type, ==, QXL_CURSOR_SET);
+ g_assert_cmpuint(red_cursor_cmd.u.set.position.x, ==, 0);
+ g_assert_cmpuint(red_cursor_cmd.u.set.position.y, ==, 0);
+ g_assert_cmpuint(red_cursor_cmd.u.set.shape.data_size, ==, 0);
}
+ g_test_assert_expected_messages();
+
free(cursor);
free(chunks[0]);
+ memslot_info_destroy(&mem_info);
+}
+
+static void test_circular_small_chunks(void)
+{
+ RedMemSlotInfo mem_info;
+ RedCursorCmd red_cursor_cmd;
+ QXLCursorCmd cursor_cmd;
+ QXLCursor *cursor;
+ QXLDataChunk *chunks[2];
+
+ init_meminfo(&mem_info);
+ g_test_expect_message(G_LOG_DOMAIN, G_LOG_LEVEL_WARNING,
+ "*red_get_data_chunks_ptr: data split in too many chunks, avoiding DoS*");
/* a circular list of small chunks should not be a problems */
- test("circular small chunks");
memset(&cursor_cmd, 0, sizeof(cursor_cmd));
cursor_cmd.type = QXL_CURSOR_SET;
@@ -189,16 +245,49 @@ int main(int argc, char **argv)
memset(&red_cursor_cmd, 0xaa, sizeof(red_cursor_cmd));
if (!red_get_cursor_cmd(&mem_info, 0, &red_cursor_cmd, to_physical(&cursor_cmd))) {
/* function does not return errors so there should be no data */
- assert(red_cursor_cmd.type == QXL_CURSOR_SET);
- assert(red_cursor_cmd.u.set.position.x == 0);
- assert(red_cursor_cmd.u.set.position.y == 0);
- assert(red_cursor_cmd.u.set.shape.data_size == 0);
+ g_assert_cmpuint(red_cursor_cmd.type, ==, QXL_CURSOR_SET);
+ g_assert_cmpuint(red_cursor_cmd.u.set.position.x, ==, 0);
+ g_assert_cmpuint(red_cursor_cmd.u.set.position.y, ==, 0);
+ g_assert_cmpuint(red_cursor_cmd.u.set.shape.data_size, ==, 0);
}
+ g_test_assert_expected_messages();
+
free(cursor);
free(chunks[0]);
-
memslot_info_destroy(&mem_info);
- free(surface_mem);
+}
+
+
+int main(int argc, char *argv[])
+{
+ g_test_init(&argc, &argv, NULL);
+
+ /* try to create a surface with no issues, should succeed */
+ g_test_add_func("/server/qxl-parsing-no-issues", test_no_issues);
+
+ /* try to create a surface with a stride too small to fit
+ * the entire width.
+ * This can be used to cause buffer overflows so refuse it.
+ */
+ g_test_add_func("/server/qxl-parsing/stride-too-small", test_stride_too_small);
+
+ /* try to create a surface quite large.
+ * The sizes (width and height) were chosen so the multiplication
+ * using 32 bit values gives a very small value.
+ * These kind of values should be refused as they will cause
+ * overflows. Also the total memory for the card is not enough to
+ * hold the surface so surely can't be accepted.
+ */
+ g_test_add_func("/server/qxl-parsing/too-big-image", test_too_big_image);
+
+ /* test base cursor with no problems */
+ g_test_add_func("/server/qxl-parsing/base-cursor-command", test_cursor_command);
+
+ /* a circular list of empty chunks should not be a problems */
+ g_test_add_func("/server/qxl-parsing/circular-empty-chunks", test_circular_empty_chunks);
+
+ /* a circular list of small chunks should not be a problems */
+ g_test_add_func("/server/qxl-parsing/circular-small-chunks", test_circular_small_chunks);
- return exit_code;
+ return g_test_run();
}
commit 8be1120904b47feded4e743d79179f2a63c3f718
Author: Christophe Fergeau <cfergeau at redhat.com>
Date: Tue Mar 14 15:41:38 2017 +0100
reds-stream: Don't use sendmsg with uninitialized memory
On my 64 bit Fedora 25, CMSG_SPACE() adds 4 bytes of padding after the
file descriptor in the control data. This causes warnings when ran under
valgrind as we set msg_controllen to CMSG_SPACE().
This commit fills the control data to 0 to avoid these warnings.
==30301== Syscall param sendmsg(msg.msg_control) points to uninitialised byte(s)
==30301== at 0x8127367: sendmsg (sendmsg.c:28)
==30301== by 0x41880B: reds_stream_send_msgfd (reds-stream.c:295)
==30301== by 0x40953F: main (test-stream.c:121)
==30301== Address 0xffefff1b4 is on thread 1's stack
==30301== in frame #1, created by reds_stream_send_msgfd (reds-stream.c:263)
Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
Acked-by: Pavel Grunt <pgrunt at redhat.com>
diff --git a/server/reds-stream.c b/server/reds-stream.c
index a813a8b8..8ac296d2 100644
--- a/server/reds-stream.c
+++ b/server/reds-stream.c
@@ -283,6 +283,10 @@ int reds_stream_send_msgfd(RedsStream *stream, int fd)
if (fd != -1) {
msgh.msg_control = control.data;
msgh.msg_controllen = sizeof(control.data);
+ /* CMSG_SPACE() might be larger than CMSG_LEN() as it can include some
+ * padding. We set the whole control data to 0 to avoid valgrind warnings
+ */
+ memset(control.data, 0, sizeof(control.data));
cmsg = CMSG_FIRSTHDR(&msgh);
cmsg->cmsg_len = CMSG_LEN(fd_size);
More information about the Spice-commits
mailing list