| Message ID | 20221125083054.117504-1-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests/qtest/migration-test: Fix unlink error and memory leaks | expand |
On Fri, Nov 25, 2022 at 09:30:54AM +0100, Thomas Huth wrote: > When running the migration test compiled with Clang from Fedora 37 > and sanitizers enabled, there is an error complaining about unlink(): > > ../tests/qtest/migration-test.c:1072: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 > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior > ../tests/qtest/migration-test.c:1072:12 in > (test program exited with status code 1) > TAP parsing error: Too few tests run (expected 33, got 20) > > The data->clientcert and data->clientkey pointers can indeed be unset > in some tests, so we have to check them before calling unlink() with > those. > > While we're at it, I also noticed that the code is only freeing > some but not all of the allocated strings in this function, and > indeed, valgrind is also complaining about memory leaks here. > So let's call g_free() on all allocated strings to avoid leaking > memory here. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > tests/qtest/migration-test.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
Thomas Huth <thuth@redhat.com> wrote: > When running the migration test compiled with Clang from Fedora 37 > and sanitizers enabled, there is an error complaining about unlink(): > > ../tests/qtest/migration-test.c:1072: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 > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior > ../tests/qtest/migration-test.c:1072:12 in > (test program exited with status code 1) > TAP parsing error: Too few tests run (expected 33, got 20) > > The data->clientcert and data->clientkey pointers can indeed be unset > in some tests, so we have to check them before calling unlink() with > those. > > While we're at it, I also noticed that the code is only freeing > some but not all of the allocated strings in this function, and > indeed, valgrind is also complaining about memory leaks here. > So let's call g_free() on all allocated strings to avoid leaking > memory here. > > Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com>
On Fri, Nov 25, 2022 at 4:33 PM Thomas Huth <thuth@redhat.com> wrote: > > When running the migration test compiled with Clang from Fedora 37 > and sanitizers enabled, there is an error complaining about unlink(): > > ../tests/qtest/migration-test.c:1072: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 > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior > ../tests/qtest/migration-test.c:1072:12 in > (test program exited with status code 1) > TAP parsing error: Too few tests run (expected 33, got 20) > > The data->clientcert and data->clientkey pointers can indeed be unset > in some tests, so we have to check them before calling unlink() with > those. > > While we're at it, I also noticed that the code is only freeing > some but not all of the allocated strings in this function, and > indeed, valgrind is also complaining about memory leaks here. > So let's call g_free() on all allocated strings to avoid leaking > memory here. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > tests/qtest/migration-test.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > Tested-by: Bin Meng <bmeng@tinylab.org>
Daniel P. Berrangé <berrange@redhat.com> wrote: > On Fri, Nov 25, 2022 at 09:30:54AM +0100, Thomas Huth wrote: >> When running the migration test compiled with Clang from Fedora 37 >> and sanitizers enabled, there is an error complaining about unlink(): >> >> ../tests/qtest/migration-test.c:1072: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 >> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior >> ../tests/qtest/migration-test.c:1072:12 in >> (test program exited with status code 1) >> TAP parsing error: Too few tests run (expected 33, got 20) >> >> The data->clientcert and data->clientkey pointers can indeed be unset >> in some tests, so we have to check them before calling unlink() with >> those. >> >> While we're at it, I also noticed that the code is only freeing >> some but not all of the allocated strings in this function, and >> indeed, valgrind is also complaining about memory leaks here. >> So let's call g_free() on all allocated strings to avoid leaking >> memory here. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com>
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 442998d9eb..dbde726adf 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -1066,15 +1066,27 @@ test_migrate_tls_x509_finish(QTestState *from, TestMigrateTLSX509Data *data = opaque; test_tls_cleanup(data->keyfile); + g_free(data->keyfile); + unlink(data->cacert); + g_free(data->cacert); unlink(data->servercert); + g_free(data->servercert); unlink(data->serverkey); - unlink(data->clientcert); - unlink(data->clientkey); - rmdir(data->workdir); + g_free(data->serverkey); + if (data->clientcert) { + unlink(data->clientcert); + g_free(data->clientcert); + } + if (data->clientkey) { + unlink(data->clientkey); + g_free(data->clientkey); + } + + rmdir(data->workdir); g_free(data->workdir); - g_free(data->keyfile); + g_free(data); } #endif /* CONFIG_TASN1 */
When running the migration test compiled with Clang from Fedora 37 and sanitizers enabled, there is an error complaining about unlink(): ../tests/qtest/migration-test.c:1072: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 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../tests/qtest/migration-test.c:1072:12 in (test program exited with status code 1) TAP parsing error: Too few tests run (expected 33, got 20) The data->clientcert and data->clientkey pointers can indeed be unset in some tests, so we have to check them before calling unlink() with those. While we're at it, I also noticed that the code is only freeing some but not all of the allocated strings in this function, and indeed, valgrind is also complaining about memory leaks here. So let's call g_free() on all allocated strings to avoid leaking memory here. Signed-off-by: Thomas Huth <thuth@redhat.com> --- tests/qtest/migration-test.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)