[PATCH BlueZ v1 1/3] sdp-xml: Fix leaking the parse stack on malformed input

[PATCH BlueZ v1 1/3] sdp-xml: Fix leaking the parse stack on malformed input

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

sdp_xml_parse_record() frees its context but never the elements left on
ctx_data->stack_head.

element_end() returns early without popping the stack when it rejects a
document, for instance on a mismatched </sequence> close, so a malformed
record leaves its elements behind and they are never freed.

Free the remaining stack elements before returning. Found with the
compute-seq-size-type-confusion.xml test:

56 (direct) + 1,072 (indirect) bytes in 1 blocks are definitely lost
   at calloc (vg_replace_malloc.c:1678)
   by sdp_xml_data_alloc (sdp-xml.c:73)
   by element_start (sdp-xml.c:473)
   by g_markup_parse_context_parse (gmarkup.c:1369)
   by sdp_xml_parse_record (sdp-xml.c:696)

Assisted-by: Claude:claude-opus-5
---
 src/sdp-xml.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/src/sdp-xml.c b/src/sdp-xml.c
index bad9e289344f..bcd5785f87ca 100644
--- a/src/sdp-xml.c
+++ b/src/sdp-xml.c
@@ -551,6 +551,17 @@ static void sdp_xml_data_free(struct sdp_xml_data *elem)
 	free(elem);
 }
 
+/* Free the elements left on the stack, e.g. by a document that is malformed */
+static void sdp_xml_data_free_stack(struct sdp_xml_data *elem)
+{
+	while (elem) {
+		struct sdp_xml_data *next = elem->next;
+
+		sdp_xml_data_free(elem);
+		elem = next;
+	}
+}
+
 static void element_end(GMarkupParseContext *context,
 		const char *element_name, gpointer user_data, GError **err)
 {
@@ -696,6 +707,7 @@ sdp_record_t *sdp_xml_parse_record(const char *data, int size)
 	if (g_markup_parse_context_parse(ctx, data, size, NULL) == FALSE) {
 		error("XML parsing error");
 		g_markup_parse_context_free(ctx);
+		sdp_xml_data_free_stack(ctx_data->stack_head);
 		sdp_record_free(record);
 		free(ctx_data);
 		return NULL;
@@ -703,6 +715,8 @@ sdp_record_t *sdp_xml_parse_record(const char *data, int size)
 
 	g_markup_parse_context_free(ctx);
 
+	sdp_xml_data_free_stack(ctx_data->stack_head);
+
 	free(ctx_data);
 
 	return record;
-- 
2.54.0

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

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

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.

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,
-- 
2.54.0

[PATCH BlueZ v1 3/3] sdp: Fix memory leak when freeing alternates

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

sdp_data_alloc_with_length() stores the members of SDP_ALT8, SDP_ALT16
and SDP_ALT32 in val.dataseq, exactly like it does for the SDP_SEQ8,
SDP_SEQ16 and SDP_SEQ32 sequences.

sdp_data_free() only calls data_seq_free() for the sequences though, so
freeing an alternate frees the alternate itself and leaks every one of
its members, along with anything they own in turn.

Free the members of alternates as well, and add a test parsing a record
containing an alternate, which leaks without this change:

209 (48 direct, 161 indirect) bytes in 1 blocks are definitely lost
   at calloc (vg_replace_malloc.c:1678)
   by sdp_data_alloc_with_length (sdp.c:350)
   by sdp_data_alloc (sdp.c:486)
   by sdp_xml_parse_int (sdp-xml.c:243)
   by sdp_xml_parse_datatype (sdp-xml.c:421)
   by element_start (sdp-xml.c:507)

Assisted-by: Claude:claude-opus-5
---
 Makefile.am                |  1 +
 lib/bluetooth/sdp.c        |  3 +++
 unit/sdp-xml/alternate.xml | 12 ++++++++++++
 unit/test-sdp-xml.c        |  1 +
 4 files changed, 17 insertions(+)
 create mode 100644 unit/sdp-xml/alternate.xml

diff --git a/Makefile.am b/Makefile.am
index 3c6cf92ab403..2754e1b7f2da 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -648,6 +648,7 @@ unit_test_sdp_xml_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS) -DTOP_SRCDIR=\""$(srcdir)
 unit_test_sdp_xml_CPPFLAGS = -I$(srcdir)/lib
 
 EXTRA_DIST += unit/sdp-xml/Bluetooth_HID-sdp_record.xml		\
+	unit/sdp-xml/alternate.xml				\
 	unit/sdp-xml/compute-seq-size-type-confusion.xml	\
 	unit/sdp-xml/duplicate-attribute.xml			\
 	unit/sdp-xml/qt-SerialPortSDPRecord.xml
diff --git a/lib/bluetooth/sdp.c b/lib/bluetooth/sdp.c
index 8c0865398519..1e027f9ebe6d 100644
--- a/lib/bluetooth/sdp.c
+++ b/lib/bluetooth/sdp.c
@@ -972,6 +972,9 @@ void sdp_data_free(sdp_data_t *d)
 	case SDP_SEQ8:
 	case SDP_SEQ16:
 	case SDP_SEQ32:
+	case SDP_ALT8:
+	case SDP_ALT16:
+	case SDP_ALT32:
 		data_seq_free(d);
 		break;
 	case SDP_URL_STR8:
diff --git a/unit/sdp-xml/alternate.xml b/unit/sdp-xml/alternate.xml
new file mode 100644
index 000000000000..a35ebccbc71f
--- /dev/null
+++ b/unit/sdp-xml/alternate.xml
@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<record>
+	<attribute id="0x0004">
+		<alternate>
+			<uint32 value="0x11223344" />
+			<text value="alternate-member" />
+			<sequence>
+				<uint16 value="0x0100" />
+			</sequence>
+		</alternate>
+	</attribute>
+</record>
diff --git a/unit/test-sdp-xml.c b/unit/test-sdp-xml.c
index 72e01f5f5806..069b064b7ae8 100644
--- a/unit/test-sdp-xml.c
+++ b/unit/test-sdp-xml.c
@@ -127,6 +127,7 @@ static struct test_data file_tests[] = {
 	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),
+	DEFINE_TEST("alternate.xml", TRUE),
 };
 
 int main(int argc, char *argv[])
-- 
2.54.0

RE: [BlueZ,v1,1/3] sdp-xml: Fix leaking the parse stack on malformed input

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3752 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1146225

---Test result---

Test Summary:
CheckPatch                    FAIL      1.60 seconds
GitLint                       PASS      1.63 seconds
BuildEll                      PASS      20.52 seconds
BluezMake                     PASS      569.56 seconds
MakeCheck                     PASS      18.91 seconds
MakeDistcheck                 PASS      153.33 seconds
CheckValgrind                 PASS      222.76 seconds
CheckSmatch                   PASS      296.58 seconds
bluezmakeextell               PASS      101.80 seconds
IncrementalBuild              PASS      595.61 seconds
ScanBuild                     PASS      876.15 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,v1,1/3] sdp-xml: Fix leaking the parse stack on malformed input
WARNING:BAD_SIGN_OFF: Non-standard signature: Assisted-by:
#115: 
Assisted-by: Claude:claude-opus-5

ERROR:BAD_SIGN_OFF: Unrecognized email address: 'Claude:claude-opus-5'
#115: 
Assisted-by: Claude:claude-opus-5

/github/workspace/src/patch/14750740.patch total: 1 errors, 1 warnings, 32 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14750740.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,v1,2/3] unit/test-sdp-xml: Give each test its own test_data
WARNING:BAD_SIGN_OFF: Non-standard signature: Assisted-by:
#116: 
Assisted-by: Claude:claude-opus-5

ERROR:BAD_SIGN_OFF: Unrecognized email address: 'Claude:claude-opus-5'
#116: 
Assisted-by: Claude:claude-opus-5

/github/workspace/src/patch/14750741.patch total: 1 errors, 1 warnings, 47 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14750741.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,v1,3/3] sdp: Fix memory leak when freeing alternates
WARNING:BAD_SIGN_OFF: Non-standard signature: Assisted-by:
#120: 
Assisted-by: Claude:claude-opus-5

ERROR:BAD_SIGN_OFF: Unrecognized email address: 'Claude:claude-opus-5'
#120: 
Assisted-by: Claude:claude-opus-5

/github/workspace/src/patch/14750742.patch total: 1 errors, 1 warnings, 35 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14750742.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.




https://github.com/bluez/bluez/pull/2403

---
Regards,
Linux Bluetooth