[BlueZ,1/4] lib/sdp: Allocate strings in sdp_data_t with NULL termination

Commit Message

Jonas Dreßler Nov. 3, 2023, 6:21 p.m. UTC

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 <[e-mail]>
---
 lib/sdp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

[e-mail] Nov. 3, 2023, 8:27 p.m. UTC | #1

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 <[e-mail]>
---
/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 <[e-mail]>
---
/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 <[e-mail]>
---
/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 <[e-mail]>
---
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

Patch

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;