[PATCH for-9.2 1/9] tests/qtest/migration-test: Fix bootfile cleanup handling

Peter Maydell posted 9 patches 3 months ago
[PATCH for-9.2 1/9] tests/qtest/migration-test: Fix bootfile cleanup handling
Posted by Peter Maydell 3 months ago
If you invoke the migration-test binary in such a way that it doesn't run
any tests, then we never call bootfile_create(), and at the end of
main() bootfile_delete() will try to unlink(NULL), which is not valid.
This can happen if for instance you tell the test binary to run a
subset of tests that turns out to be empty, like this:

 (cd build/asan && QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --tap -k -p bang)
 # random seed: R02S6501b289ff8ced4231ba452c3a87bc6f
 # Skipping test: userfaultfd not available
 1..0
 ../../tests/qtest/migration-test.c:182:12: runtime error: null pointer passed as argument 1, which is declared to never be null
 /usr/include/unistd.h:858:48: note: nonnull attribute specified here

Conversely, because we call bootfile_create() once per test
but only call bootfile_delete() at the end of the whole test
run, we will leak the memory we used for bootpath when we
overwrite it.

Handle these by:
 * making bootfile_delete() handle not needing to do anything
   because bootfile_create() was never called
 * making bootfile_create() call bootfile_delete() first to
   tidy up any previous bootfile before it creates a fresh one

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I spotted this because I was trying to run a single subtest and
messed up the test name so it didn't match anything :-)
The second part was noticed by LeakSanitizer.
---
 tests/qtest/migration-test.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 70b606b8886..5cf238a4f05 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -144,12 +144,23 @@ static char *bootpath;
 #include "tests/migration/ppc64/a-b-kernel.h"
 #include "tests/migration/s390x/a-b-bios.h"
 
+static void bootfile_delete(void)
+{
+    if (!bootpath) {
+        return;
+    }
+    unlink(bootpath);
+    g_free(bootpath);
+    bootpath = NULL;
+}
+
 static void bootfile_create(char *dir, bool suspend_me)
 {
     const char *arch = qtest_get_arch();
     unsigned char *content;
     size_t len;
 
+    bootfile_delete();
     bootpath = g_strdup_printf("%s/bootsect", dir);
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         /* the assembled x86 boot sector should be exactly one sector large */
@@ -177,13 +188,6 @@ static void bootfile_create(char *dir, bool suspend_me)
     fclose(bootfile);
 }
 
-static void bootfile_delete(void)
-{
-    unlink(bootpath);
-    g_free(bootpath);
-    bootpath = NULL;
-}
-
 /*
  * Wait for some output in the serial output file,
  * we get an 'A' followed by an endless string of 'B's
-- 
2.34.1
Re: [PATCH for-9.2 1/9] tests/qtest/migration-test: Fix bootfile cleanup handling
Posted by Fabiano Rosas 3 months ago
Peter Maydell <peter.maydell@linaro.org> writes:

> If you invoke the migration-test binary in such a way that it doesn't run
> any tests, then we never call bootfile_create(), and at the end of
> main() bootfile_delete() will try to unlink(NULL), which is not valid.
> This can happen if for instance you tell the test binary to run a
> subset of tests that turns out to be empty, like this:
>
>  (cd build/asan && QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --tap -k -p bang)
>  # random seed: R02S6501b289ff8ced4231ba452c3a87bc6f
>  # Skipping test: userfaultfd not available
>  1..0
>  ../../tests/qtest/migration-test.c:182:12: runtime error: null pointer passed as argument 1, which is declared to never be null
>  /usr/include/unistd.h:858:48: note: nonnull attribute specified here
>
> Conversely, because we call bootfile_create() once per test
> but only call bootfile_delete() at the end of the whole test
> run, we will leak the memory we used for bootpath when we
> overwrite it.
>
> Handle these by:
>  * making bootfile_delete() handle not needing to do anything
>    because bootfile_create() was never called
>  * making bootfile_create() call bootfile_delete() first to
>    tidy up any previous bootfile before it creates a fresh one
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>