From 82e258db15dc563e1d015c19b3af2d585695a7b1 Mon Sep 17 00:00:00 2001 From: Li Qiang Date: Tue, 14 Jul 2020 08:35:36 -0700 Subject: [PATCH 1/6] qtest: bios-tables-test: fix a memory leak Fixes: 5da7c35e25a("bios-tables-test: Add Q35/TPM-TIS test") Signed-off-by: Li Qiang Message-Id: <20200714153536.66060-1-liq3ea@163.com> Reviewed-by: Eric Auger Reviewed-by: Igor Mammedov Signed-off-by: Thomas Huth --- tests/qtest/bios-tables-test.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index c315156858..d49b3988ec 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -924,6 +924,7 @@ static void test_acpi_tcg_tpm(const char *machine, const char *tpm_if, g_free(variant); g_free(tmp_path); g_free(tmp_dir_name); + g_free(args); free_test_data(&data); #else g_test_skip("TPM disabled"); From 15c51f724ec61ab6a6910c41113d6d07164b6ca4 Mon Sep 17 00:00:00 2001 From: Li Qiang Date: Wed, 15 Jul 2020 08:41:17 -0700 Subject: [PATCH 2/6] tests: qmp-cmd-test: fix memory leak Properly free each test response to avoid memory leak and separate qtest_qmp() calls with spare lines, in a consistent manner. Fixes: 5b88849e7b9("tests/qmp-cmd-test: Add qmp/object-add-failure-modes") Reviewed-by: Eric Auger Signed-off-by: Li Qiang Message-Id: <20200715154117.15456-1-liq3ea@163.com> Fixes: 9fc719b869 ("tests/qmp-cmd-test: Add qmp/object-add-duplicate-id") Reviewed-by: Markus Armbruster Signed-off-by: Thomas Huth --- tests/qtest/qmp-cmd-test.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c index c68f99f659..f7b1aa7fdc 100644 --- a/tests/qtest/qmp-cmd-test.c +++ b/tests/qtest/qmp-cmd-test.c @@ -230,6 +230,8 @@ static void test_object_add_failure_modes(void) " 'props': {'size': 1048576 } } }"); g_assert_nonnull(resp); g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); + resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':" " {'qom-type': 'memory-backend-ram', 'id': 'ram1'," " 'props': {'size': 1048576 } } }"); @@ -241,6 +243,7 @@ static void test_object_add_failure_modes(void) " {'id': 'ram1' } }"); g_assert_nonnull(resp); g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); /* attempt to create an object with a property of a wrong type */ resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':" @@ -249,17 +252,20 @@ static void test_object_add_failure_modes(void) g_assert_nonnull(resp); /* now do it right */ qmp_assert_error_class(resp, "GenericError"); + resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':" " {'qom-type': 'memory-backend-ram', 'id': 'ram1'," " 'props': {'size': 1048576 } } }"); g_assert_nonnull(resp); g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); /* delete ram1 object */ resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':" " {'id': 'ram1' } }"); g_assert_nonnull(resp); g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); /* attempt to create an object without the id */ resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':" @@ -267,18 +273,21 @@ static void test_object_add_failure_modes(void) " 'props': {'size': 1048576 } } }"); g_assert_nonnull(resp); qmp_assert_error_class(resp, "GenericError"); + /* now do it right */ resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':" " {'qom-type': 'memory-backend-ram', 'id': 'ram1'," " 'props': {'size': 1048576 } } }"); g_assert_nonnull(resp); g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); /* delete ram1 object */ resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':" " {'id': 'ram1' } }"); g_assert_nonnull(resp); g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); /* attempt to set a non existing property */ resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':" @@ -286,23 +295,27 @@ static void test_object_add_failure_modes(void) " 'props': {'sized': 1048576 } } }"); g_assert_nonnull(resp); qmp_assert_error_class(resp, "GenericError"); + /* now do it right */ resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':" " {'qom-type': 'memory-backend-ram', 'id': 'ram1'," " 'props': {'size': 1048576 } } }"); g_assert_nonnull(resp); g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); /* delete ram1 object without id */ resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':" " {'ida': 'ram1' } }"); g_assert_nonnull(resp); + qobject_unref(resp); /* delete ram1 object */ resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':" " {'id': 'ram1' } }"); g_assert_nonnull(resp); g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); /* delete ram1 object that does not exist anymore*/ resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':" From f5ec79f5e0a3a307fc2a11b3ba8066a2d0a90233 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Tue, 14 Jul 2020 13:46:16 -0400 Subject: [PATCH 3/6] fuzz: Expect the cmdline in a freeable GString In the initial FuzzTarget, get_init_cmdline returned a char *. With this API, we had no guarantee about where the string came from. For example, i440fx-qtest-reboot-fuzz simply returned a pointer to a string literal, while the QOS-based targets build the arguments out in a GString an return the gchar *str pointer. Since we did not try to free the cmdline, we have a leak for any targets that do not simply return string literals. Clean up this mess by forcing fuzz-targets to return a GString, that we can free. Signed-off-by: Alexander Bulekov Message-Id: <20200714174616.20709-1-alxndr@bu.edu> Reviewed-by: Darren Kenny Signed-off-by: Thomas Huth --- tests/qtest/fuzz/fuzz.c | 13 ++++++------- tests/qtest/fuzz/fuzz.h | 6 +++--- tests/qtest/fuzz/i440fx_fuzz.c | 4 ++-- tests/qtest/fuzz/qos_fuzz.c | 6 +++--- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c index 0b66e43409..6bc17ef313 100644 --- a/tests/qtest/fuzz/fuzz.c +++ b/tests/qtest/fuzz/fuzz.c @@ -199,16 +199,15 @@ int LLVMFuzzerInitialize(int *argc, char ***argv, char ***envp) } /* Run QEMU's softmmu main with the fuzz-target dependent arguments */ - const char *init_cmdline = fuzz_target->get_init_cmdline(fuzz_target); - init_cmdline = g_strdup_printf("%s -qtest /dev/null -qtest-log %s", - init_cmdline, - getenv("QTEST_LOG") ? "/dev/fd/2" - : "/dev/null"); - + GString *cmd_line = fuzz_target->get_init_cmdline(fuzz_target); + g_string_append_printf(cmd_line, + " -qtest /dev/null -qtest-log %s", + getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null"); /* Split the runcmd into an argv and argc */ wordexp_t result; - wordexp(init_cmdline, &result, 0); + wordexp(cmd_line->str, &result, 0); + g_string_free(cmd_line, true); qemu_init(result.we_wordc, result.we_wordv, NULL); diff --git a/tests/qtest/fuzz/fuzz.h b/tests/qtest/fuzz/fuzz.h index 72d5710f6c..9ca3d107c5 100644 --- a/tests/qtest/fuzz/fuzz.h +++ b/tests/qtest/fuzz/fuzz.h @@ -50,10 +50,10 @@ typedef struct FuzzTarget { /* - * returns the arg-list that is passed to qemu/softmmu init() - * Cannot be NULL + * Returns the arguments that are passed to qemu/softmmu init(). Freed by + * the caller. */ - const char* (*get_init_cmdline)(struct FuzzTarget *); + GString *(*get_init_cmdline)(struct FuzzTarget *); /* * will run once, prior to running qemu/softmmu init. diff --git a/tests/qtest/fuzz/i440fx_fuzz.c b/tests/qtest/fuzz/i440fx_fuzz.c index e2f31e56f9..bf966d478b 100644 --- a/tests/qtest/fuzz/i440fx_fuzz.c +++ b/tests/qtest/fuzz/i440fx_fuzz.c @@ -158,9 +158,9 @@ static void i440fx_fuzz_qos_fork(QTestState *s, static const char *i440fx_qtest_argv = TARGET_NAME " -machine accel=qtest" " -m 0 -display none"; -static const char *i440fx_argv(FuzzTarget *t) +static GString *i440fx_argv(FuzzTarget *t) { - return i440fx_qtest_argv; + return g_string_new(i440fx_qtest_argv); } static void fork_init(void) diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c index 0c68f5361f..d52f3ebd83 100644 --- a/tests/qtest/fuzz/qos_fuzz.c +++ b/tests/qtest/fuzz/qos_fuzz.c @@ -66,7 +66,7 @@ void *qos_allocate_objects(QTestState *qts, QGuestAllocator **p_alloc) return allocate_objects(qts, current_path + 1, p_alloc); } -static const char *qos_build_main_args(void) +static GString *qos_build_main_args(void) { char **path = fuzz_path_vec; QOSGraphNode *test_node; @@ -88,7 +88,7 @@ static const char *qos_build_main_args(void) /* Prepend the arguments that we need */ g_string_prepend(cmd_line, TARGET_NAME " -display none -machine accel=qtest -m 64 "); - return cmd_line->str; + return cmd_line; } /* @@ -189,7 +189,7 @@ static void walk_path(QOSGraphNode *orig_path, int len) g_free(path_str); } -static const char *qos_get_cmdline(FuzzTarget *t) +static GString *qos_get_cmdline(FuzzTarget *t) { /* * Set a global variable that we use to identify the qos_path for our From 12a9b8d8240aaddf14774b9fb03af5e2e0b9f60a Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Thu, 16 Jul 2020 07:12:22 +0200 Subject: [PATCH 4/6] configure: Fix for running with --enable-werror on macOS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The configure script currently refuses to succeed when run on macOS with --enable-werror: ERROR: configure test passed without -Werror but failed with -Werror. The information in config.log indicates: config-temp/qemu-conf.c:3:55: error: control reaches end of non-void function [-Werror,-Wreturn-type] static void *f(void *p) { pthread_setname_np("QEMU"); } ^ And indeed, the return statement is missing here. Fixes: 479a57475e ("util: Implement debug-threads for macOS") Message-Id: <20200716055655.24507-1-thuth@redhat.com> Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Thomas Huth --- configure | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure b/configure index b751c853f5..e93836aaae 100755 --- a/configure +++ b/configure @@ -4198,7 +4198,7 @@ pthread_setname_np_wo_tid=no cat > $TMPC << EOF #include -static void *f(void *p) { pthread_setname_np("QEMU"); } +static void *f(void *p) { pthread_setname_np("QEMU"); return NULL; } int main(void) { pthread_t thread; From ab2d185d6b1d3aa1ab2416a896347d235e32ac3d Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 14 Jul 2020 18:01:59 +0200 Subject: [PATCH 5/6] qom: Plug memory leak in "info qom-tree" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted" created a memory leak, because I didn't realize object_get_canonical_path_component()'s value needs to be freed. Reproducer: $ qemu-system-x86_64 -nodefaults -display none -S -monitor stdio QEMU 5.0.50 monitor - type 'help' for more information (qemu) info qom-tree This leaks some 4500 path components, 12-13 characters on average, i.e. roughly 100kBytes depending on the allocator. A couple of hundred "info qom-tree" here, a couple of hundred there, and soon enough we're talking about real memory. Plug the leak. Fixes: e8c9e65816f5dbfe18ad3b2be938d0d8192d459a Signed-off-by: Markus Armbruster Reported-by: Reviewed-by: Li Qiang [sent same patch] Message-Id: <20200714160202.3121879-3-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Thomas Huth --- qom/qom-hmp-cmds.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c index 9ed8bb1c9f..aaacadacca 100644 --- a/qom/qom-hmp-cmds.c +++ b/qom/qom-hmp-cmds.c @@ -96,8 +96,10 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent); static int qom_composition_compare(const void *a, const void *b, void *ignore) { - return g_strcmp0(a ? object_get_canonical_path_component(a) : NULL, - b ? object_get_canonical_path_component(b) : NULL); + g_autofree char *ac = object_get_canonical_path_component(a); + g_autofree char *bc = object_get_canonical_path_component(b); + + return g_strcmp0(ac, bc); } static int insert_qom_composition_child(Object *obj, void *opaque) From b610eba335d5c8ac7484dbb1c886b125e2dea058 Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Wed, 15 Jul 2020 06:32:48 +0200 Subject: [PATCH 6/6] gitlab-ci.yml: Add fuzzer tests So far we neither compile-tested nor run any of the new fuzzers in our CI, which led to some build failures of the fuzzer code in the past weeks. To avoid this problem, add a job to compile the fuzzer code and run some loops (which likely don't find any new bugs via fuzzing, but at least we know that the code can still be run). A nice side-effect of this test is that the leak tests are enabled here, so we should now notice some of the memory leaks in our code base earlier. Message-Id: <20200716100950.27396-1-thuth@redhat.com> Reviewed-by: Alexander Bulekov Signed-off-by: Thomas Huth --- .gitlab-ci.yml | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 5eeba2791b..41597c3603 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -161,9 +161,27 @@ build-clang: IMAGE: fedora CONFIGURE_ARGS: --cc=clang --cxx=clang++ TARGETS: alpha-softmmu arm-softmmu m68k-softmmu mips64-softmmu - ppc-softmmu s390x-softmmu x86_64-softmmu arm-linux-user + ppc-softmmu s390x-softmmu arm-linux-user MAKE_CHECK_ARGS: check +build-fuzzer: + <<: *native_build_job_definition + variables: + IMAGE: fedora + script: + - mkdir build + - cd build + - ../configure --cc=clang --cxx=clang++ --enable-fuzzing + --enable-sanitizers --target-list=x86_64-softmmu + - make -j"$JOBS" all check-build x86_64-softmmu/fuzz + - make check + - for fuzzer in i440fx-qos-fork-fuzz i440fx-qos-noreset-fuzz + i440fx-qtest-reboot-fuzz virtio-scsi-flags-fuzz virtio-scsi-fuzz ; do + echo Testing ${fuzzer} ... ; + x86_64-softmmu/qemu-fuzz-x86_64 --fuzz-target=${fuzzer} -runs=1000 + || exit 1 ; + done + build-tci: <<: *native_build_job_definition variables: