There's fairly old oversight in the SDP parsing code where it was forgotten to
add a NULL termination byte to strings that are later handled using strlen().
This series fixes that oversight, with a few commits to better follow best
practices on top.
Found by running with address sanitizer.
Jonas Dreßler (4):
lib/sdp: Allocate strings in sdp_data_t with NULL termination
lib/sdp: Don't assume uint8_t has size 1
lib/sdp: Use correct string length in sdp_copy_seq()
lib/sdp: Pass size_t to sdp_get_string_attr()
lib/sdp.c | 15 ++++++++-------
lib/sdp_lib.h | 14 +++++++-------
2 files changed, 15 insertions(+), 14 deletions(-)
--
2.41.0
sdp_data_t->unitSize for strings in the SDP record is
`sizeof(uint8_t) + strlen(str)`.
The "length" argument of sdp_data_alloc_with_length() is expected to be
only the length of the string (so `sdp_data_t->unitSize - sizeof(uint8_t)`).
Since the last commit, in sdp_copy_seq() we're allocating one byte too much
for strings now, because the `sizeof(uint8_t)` is not subtracted from unitSize
there.
Fix this by making use of the length returned by sdp_data_value() and pass
that on to sdp_data_alloc_with_length().
Co-developed-by: Zander Brown <[email protected]>
---
lib/sdp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lib/sdp.c b/lib/sdp.c
index 006ab057a..4b10d8f67 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -1527,10 +1527,10 @@ static sdp_data_t *sdp_copy_seq(sdp_data_t *data)
for (tmp = data; tmp; tmp = tmp->next) {
sdp_data_t *datatmp;
void *value;
+ uint32_t len = 0;
- value = sdp_data_value(tmp, NULL);
- datatmp = sdp_data_alloc_with_length(tmp->dtd, value,
- tmp->unitSize);
+ value = sdp_data_value(tmp, &len);
+ datatmp = sdp_data_alloc_with_length(tmp->dtd, value, len);
if (cur)
cur->next = datatmp;
--
2.41.0
In extract_str() we create sdp_data_t with strings and allocate
sdp_data_t->val.str an extra 0-byte as NULL termination. In
sdp_data_alloc_with_length() we're missing this, and strlen() in
sdp_get_string_attr() ends up overrunning the sdpdata->val.str buffer
looking for the NULL termination.
Allocate the extra 0-byte for sdp_data_t->val.str to ensure this
overrun can't happen.
Co-developed-by: Zander Brown <[email protected]>
---
lib/sdp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/sdp.c b/lib/sdp.c
index 844ae0d25..1565259a3 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -420,7 +420,7 @@ sdp_data_t *sdp_data_alloc_with_length(uint8_t dtd, const void *value,
d->unitSize += length;
if (length <= USHRT_MAX) {
- d->val.str = malloc(length);
+ d->val.str = bt_malloc0(length + 1);
if (!d->val.str) {
free(d);
return NULL;
--
2.41.0
We're currently type-casting the output of strlen(sdpdata->val.str) into
an int, which is somewhat problematic given that strlen() can return
values larger than sizeof(int).
We can do better here and use size_t instead, so let's do that.
While at it, also add a comment explaining why the check here is "smaller
than" instead of "smaller than or equal".
Co-developed-by: Zander Brown <[email protected]>
---
lib/sdp.c | 5 +++--
lib/sdp_lib.h | 14 +++++++-------
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/lib/sdp.c b/lib/sdp.c
index 4b10d8f67..cff7e09fb 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -2180,13 +2180,14 @@ int sdp_get_int_attr(const sdp_record_t *rec, uint16_t attrid, int *value)
}
int sdp_get_string_attr(const sdp_record_t *rec, uint16_t attrid, char *value,
- int valuelen)
+ size_t valuelen)
{
sdp_data_t *sdpdata = sdp_data_get(rec, attrid);
if (sdpdata)
/* Verify that it is what the caller expects */
if (SDP_IS_TEXT_STR(sdpdata->dtd))
- if ((int) strlen(sdpdata->val.str) < valuelen) {
+ /* Have to copy the NULL terminator too, so check len < valuelen */
+ if (strlen(sdpdata->val.str) < valuelen) {
strcpy(value, sdpdata->val.str);
return 0;
}
diff --git a/lib/sdp_lib.h b/lib/sdp_lib.h
index 22776b678..91d46f59d 100644
--- a/lib/sdp_lib.h
+++ b/lib/sdp_lib.h
@@ -141,7 +141,7 @@ int sdp_general_inquiry(inquiry_info *ii, int dev_num, int duration, uint8_t *fo
/* flexible extraction of basic attributes - Jean II */
int sdp_get_int_attr(const sdp_record_t *rec, uint16_t attr, int *value);
-int sdp_get_string_attr(const sdp_record_t *rec, uint16_t attr, char *value, int valuelen);
+int sdp_get_string_attr(const sdp_record_t *rec, uint16_t attr, char *value, size_t valuelen);
/*
* Basic sdp data functions
@@ -543,32 +543,32 @@ int sdp_get_service_avail(const sdp_record_t *rec, uint8_t *svcAvail);
int sdp_get_service_ttl(const sdp_record_t *rec, uint32_t *svcTTLInfo);
int sdp_get_database_state(const sdp_record_t *rec, uint32_t *svcDBState);
-static inline int sdp_get_service_name(const sdp_record_t *rec, char *str, int len)
+static inline int sdp_get_service_name(const sdp_record_t *rec, char *str, size_t len)
{
return sdp_get_string_attr(rec, SDP_ATTR_SVCNAME_PRIMARY, str, len);
}
-static inline int sdp_get_service_desc(const sdp_record_t *rec, char *str, int len)
+static inline int sdp_get_service_desc(const sdp_record_t *rec, char *str, size_t len)
{
return sdp_get_string_attr(rec, SDP_ATTR_SVCDESC_PRIMARY, str, len);
}
-static inline int sdp_get_provider_name(const sdp_record_t *rec, char *str, int len)
+static inline int sdp_get_provider_name(const sdp_record_t *rec, char *str, size_t len)
{
return sdp_get_string_attr(rec, SDP_ATTR_PROVNAME_PRIMARY, str, len);
}
-static inline int sdp_get_doc_url(const sdp_record_t *rec, char *str, int len)
+static inline int sdp_get_doc_url(const sdp_record_t *rec, char *str, size_t len)
{
return sdp_get_string_attr(rec, SDP_ATTR_DOC_URL, str, len);
}
-static inline int sdp_get_clnt_exec_url(const sdp_record_t *rec, char *str, int len)
+static inline int sdp_get_clnt_exec_url(const sdp_record_t *rec, char *str, size_t len)
{
return sdp_get_string_attr(rec, SDP_ATTR_CLNT_EXEC_URL, str, len);
}
-static inline int sdp_get_icon_url(const sdp_record_t *rec, char *str, int len)
+static inline int sdp_get_icon_url(const sdp_record_t *rec, char *str, size_t len)
{
return sdp_get_string_attr(rec, SDP_ATTR_ICON_URL, str, len);
}
--
2.41.0
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=798710
---Test result---
Test Summary:
CheckPatch FAIL 1.64 seconds
GitLint PASS 0.91 seconds
BuildEll PASS 33.44 seconds
BluezMake PASS 953.81 seconds
MakeCheck PASS 12.88 seconds
MakeDistcheck PASS 200.22 seconds
CheckValgrind PASS 309.63 seconds
CheckSmatch PASS 413.84 seconds
bluezmakeextell PASS 135.45 seconds
IncrementalBuild PASS 3258.43 seconds
ScanBuild WARNING 1227.60 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,1/4] lib/sdp: Allocate strings in sdp_data_t with NULL termination
WARNING:BAD_SIGN_OFF: Co-developed-by: must be immediately followed by Signed-off-by:
#59:
Co-developed-by: Zander Brown <[email protected]>
---
/github/workspace/src/src/13444881.patch total: 0 errors, 1 warnings, 8 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/src/13444881.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,2/4] lib/sdp: Don't assume uint8_t has size 1
WARNING:REPEATED_WORD: Possible repeated word: 'of'
#47:
Assuming the size of of uint8_t is bad practice, we use
WARNING:BAD_SIGN_OFF: Co-developed-by: must be immediately followed by Signed-off-by:
#52:
Co-developed-by: Zander Brown <[email protected]>
---
/github/workspace/src/src/13444882.patch total: 0 errors, 2 warnings, 8 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/src/13444882.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,3/4] lib/sdp: Use correct string length in sdp_copy_seq()
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#52:
only the length of the string (so `sdp_data_t->unitSize - sizeof(uint8_t)`).
WARNING:BAD_SIGN_OFF: Co-developed-by: must be immediately followed by Signed-off-by:
#61:
Co-developed-by: Zander Brown <[email protected]>
---
/github/workspace/src/src/13444883.patch total: 0 errors, 2 warnings, 13 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/src/13444883.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,4/4] lib/sdp: Pass size_t to sdp_get_string_attr()
WARNING:BAD_SIGN_OFF: Co-developed-by: must be immediately followed by Signed-off-by:
#58:
Co-developed-by: Zander Brown <[email protected]>
---
WARNING:LONG_LINE_COMMENT: line length of 91 exceeds 80 columns
#80: FILE: lib/sdp.c:2189:
+ /* Have to copy the NULL terminator too, so check len < valuelen */
WARNING:LONG_LINE: line length of 94 exceeds 80 columns
#94: FILE: lib/sdp_lib.h:144:
+int sdp_get_string_attr(const sdp_record_t *rec, uint16_t attr, char *value, size_t valuelen);
WARNING:LONG_LINE: line length of 86 exceeds 80 columns
#103: FILE: lib/sdp_lib.h:546:
+static inline int sdp_get_service_name(const sdp_record_t *rec, char *str, size_t len)
WARNING:LONG_LINE: line length of 86 exceeds 80 columns
#109: FILE: lib/sdp_lib.h:551:
+static inline int sdp_get_service_desc(const sdp_record_t *rec, char *str, size_t len)
WARNING:LONG_LINE: line length of 87 exceeds 80 columns
#115: FILE: lib/sdp_lib.h:556:
+static inline int sdp_get_provider_name(const sdp_record_t *rec, char *str, size_t len)
WARNING:LONG_LINE: line length of 81 exceeds 80 columns
#121: FILE: lib/sdp_lib.h:561:
+static inline int sdp_get_doc_url(const sdp_record_t *rec, char *str, size_t len)
WARNING:LONG_LINE: line length of 87 exceeds 80 columns
#127: FILE: lib/sdp_lib.h:566:
+static inline int sdp_get_clnt_exec_url(const sdp_record_t *rec, char *str, size_t len)
WARNING:LONG_LINE: line length of 82 exceeds 80 columns
#133: FILE: lib/sdp_lib.h:571:
+static inline int sdp_get_icon_url(const sdp_record_t *rec, char *str, size_t len)
/github/workspace/src/src/13444884.patch total: 0 errors, 9 warnings, 62 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/src/13444884.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.
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
lib/sdp.c:507:16: warning: Dereference of undefined pointer value
int8_t dtd = *(uint8_t *) dtds[i];
^~~~~~~~~~~~~~~~~~~~
lib/sdp.c:535:17: warning: Dereference of undefined pointer value
uint8_t dtd = *(uint8_t *) dtds[i];
^~~~~~~~~~~~~~~~~~~~
lib/sdp.c:580:12: warning: Access to field 'attrId' results in a dereference of a null pointer (loaded from variable 'd')
d->attrId = attr;
~ ^
lib/sdp.c:1870:26: warning: Potential leak of memory pointed to by 'ap'
for (; pdlist; pdlist = pdlist->next) {
^~~~~~
lib/sdp.c:1884:6: warning: Potential leak of memory pointed to by 'pds'
ap = sdp_list_append(ap, pds);
~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
lib/sdp.c:1929:10: warning: Potential leak of memory pointed to by 'u'
*seqp = sdp_list_append(*seqp, u);
~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/sdp.c:2034:4: warning: Potential leak of memory pointed to by 'lang'
sdp_list_free(*langSeq, free);
^~~~~~~~~~~~~
lib/sdp.c:2123:9: warning: Potential leak of memory pointed to by 'profDesc'
return 0;
^
lib/sdp.c:3251:8: warning: Potential leak of memory pointed to by 'pSvcRec'
pSeq = sdp_list_append(pSeq, pSvcRec);
~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/sdp.c:3252:9: warning: Potential leak of memory pointed to by 'pSeq'
pdata += sizeof(uint32_t);
~~~~~~^~~~~~~~~~~~~~~~~~~
lib/sdp.c:4588:13: warning: Potential leak of memory pointed to by 'rec_list'
} while (scanned < attr_list_len && pdata_len > 0);
^~~~~~~
lib/sdp.c:4884:40: warning: Potential leak of memory pointed to by 'tseq'
for (d = sdpdata->val.dataseq; d; d = d->next) {
^
lib/sdp.c:4920:8: warning: Potential leak of memory pointed to by 'subseq'
tseq = sdp_list_append(tseq, subseq);
~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
13 warnings generated.
---
Regards,
Linux Bluetooth