Re: [PATCH BlueZ v1 2/3] unit/test-sdp-xml: Give each test its own test_data

On Fri, 2026-08-14 at 13:47 -0400, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <[email protected]>
> 
> DEFINE_TEST assigned the filename and expected result to a single
> test_data and passed its address to tester_add(), so every test
> shared
> one struct and ran with whatever the last registered test left in it.
> 
> All the file based tests therefore parsed the same file, and a test
> pointing at a file that does not exist still passed.
> 
> That in particular meant compute-seq-size-type-confusion.xml, the
> regression test for GHSA-7mmr-gwqx-vc34, was never actually parsed,
> and
> passed only because it inherited the expected result of the test
> registered after it.

That's not quite correct. The fix introduced memory leaks, which you're
fixing in "sdp-xml: Fix leaking the parse stack on malformed input",
but it fixed the original crash.

This test would only fail when run under ASan.

I just want to be clear that this fix doesn't have an impact on GHSA-
7mmr-gwqx-vc34 being resolved earlier.

> 
> Use an array with one entry per test instead.
> 
> Assisted-by: Claude:claude-opus-5
> ---
>  unit/test-sdp-xml.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/unit/test-sdp-xml.c b/unit/test-sdp-xml.c
> index b338788aa295..72e01f5f5806 100644
> --- a/unit/test-sdp-xml.c
> +++ b/unit/test-sdp-xml.c
> @@ -114,25 +114,35 @@ static void
> sequence_on_squared_teardown(gconstpointer data)
>  	tester_teardown_complete();
>  }
>  
> -#define DEFINE_TEST(fname, res)
> {					\
> -	data.expected_result =
> res;					\
> -	data.filename =
> fname;						\
> -	tester_add("/" fname, &data,
> NULL,				\
> -			parse_xml_for_filename,
> NULL);			\
> -	}
> +#define DEFINE_TEST(fname, res) { .filename = fname,
> .expected_result = res }
> +
> +/*
> + * Each test needs its own test_data, sharing a single one would
> make every
> + * test run with the values assigned by the last one registered.
> + */
> +static struct test_data file_tests[] = {
> +	DEFINE_TEST("Bluetooth_HID-sdp_record.xml", TRUE),
> +	DEFINE_TEST("qt-SerialPortSDPRecord.xml", TRUE),
> +	/* From
> https://github.com/bluez/bluez/security/advisories/GHSA-7mmr-gwqx-vc34
>  */
> +	DEFINE_TEST("compute-seq-size-type-confusion.xml", FALSE),
> +	/* From
> https://github.com/bluez/bluez/security/advisories/GHSA-75v6-6q44-57hc
>  */
> +	DEFINE_TEST("duplicate-attribute.xml", TRUE),
> +};
>  
>  int main(int argc, char *argv[])
>  {
>  	struct test_data data;
> +	unsigned int i;
>  
>  	tester_init(&argc, &argv);
>  
> -	DEFINE_TEST("Bluetooth_HID-sdp_record.xml", TRUE);
> -	DEFINE_TEST("qt-SerialPortSDPRecord.xml", TRUE);
> -	/* From
> https://github.com/bluez/bluez/security/advisories/GHSA-7mmr-gwqx-vc34
>  */
> -	DEFINE_TEST("compute-seq-size-type-confusion.xml", FALSE);
> -	/* From
> https://github.com/bluez/bluez/security/advisories/GHSA-75v6-6q44-57hc
>  */
> -	DEFINE_TEST("duplicate-attribute.xml", TRUE);
> +	for (i = 0; i < G_N_ELEMENTS(file_tests); i++) {
> +		char *name = g_strdup_printf("/%s",
> file_tests[i].filename);
> +
> +		tester_add(name, &file_tests[i], NULL,
> +				parse_xml_for_filename, NULL);
> +		g_free(name);
> +	}
>  
>  	tester_add("/sequence_on_squared", &data,
>  		   sequence_on_squared_setup,