Re: [PATCH] tests/qtest/migration/tls-tests.c: Don't use tls_psk end hook for no_tls tests

[Fabiano Rosas <farosas@suse.de>]

Peter Maydell <peter.maydell@linaro.org> writes:

> If you run the TLS tests under a clang undefined-behaviour sanitizer build
> it will fall over like this:
>
> ../../tests/unit/crypto-tls-psk-helpers.c:53: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
>     #0 0x62bd810762ee in test_tls_psk_cleanup /home/pm215/qemu/build/clang/../../tests/unit/crypto-tls-psk-helpers.c:53:5
>     #1 0x62bd81073f89 in migrate_hook_end_tls_psk /home/pm215/qemu/build/clang/../../tests/qtest/migration/tls-tests.c:101:5
>     #2 0x62bd81062ef0 in test_precopy_common /home/pm215/qemu/build/clang/../../tests/qtest/migration/framework.c:947:9
>
> This happens because test_precopy_tcp_no_tls() uses a custom start_hook
> that only sets a couple of parameters, but reuses the tsk_psk end_hook.
> However, the end_hook runs cleanup that assumes that the data was set
> up by migrate_hook_start_tls_psk_common(). In particular, it will
> unconditionally call test_tls_psk_cleanup(data->pskfile), and
> test_tls_psk_cleanup() will unconditionally unlink() the filename it
> is passed, which is undefined behaviour if you pass it a NULL pointer.
>
> Instead of creating a TestMigrateTLSPSKData struct which we never set
> any fields in and requiring the migrate_hook_end_tls_psk() hook to
> cope with that, don't allocate the struct in the start_hook. Then
> there is nothing we need to clean up, and we can set the end_hook
> to NULL (which the test framework will interpret as "don't call
> any end_hook").
>

Thanks! My pull-request script should have caught this. I recently
excluded the TLS tests because they break ASAN in weird ways.

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

> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Not sure when this would have appeared, so cc'ing stable as it
> seems safe enough to backport just in case.

It's from this devel cycle.