| Message ID | 20240820144912.320744-2-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | tests/qtest/migration-test: Fix various leaks | expand |
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>
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
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(-)