| Message ID | 20250204101612.66823-2-pmontes@shsconsultores.es (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix Ubuntu 24.04 build error | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/BuildEll | success | Build ELL PASS |
| tedd_an/BluezMake | success | Bluez Make PASS |
| tedd_an/MakeCheck | success | Bluez Make Check PASS |
| tedd_an/MakeDistcheck | success | Make Distcheck PASS |
| tedd_an/CheckValgrind | success | Check Valgrind PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/bluezmakeextell | success | Make External ELL PASS |
| tedd_an/ScanBuild | success | Scan Build PASS |
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=930379 ---Test result--- Test Summary: CheckPatch PENDING 0.73 seconds GitLint PENDING 0.34 seconds BuildEll PASS 20.05 seconds BluezMake PASS 1422.28 seconds MakeCheck PASS 19.13 seconds MakeDistcheck PASS 156.15 seconds CheckValgrind PASS 214.21 seconds CheckSmatch PASS 267.65 seconds bluezmakeextell PASS 96.89 seconds IncrementalBuild PENDING 0.33 seconds ScanBuild PASS 852.84 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth
Hi Pablo, On Tue, Feb 4, 2025 at 5:17 AM Pablo Montes <pmontes@shsconsultores.es> wrote: > > Warning on read for a possible packet offset > greater than buffer size is treated as error. > > I suggest using ssize_t so it is always positive. > Returning if packet offset makes no sense might > not discard the whole packet and start again > > --- > emulator/serial.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/emulator/serial.c b/emulator/serial.c > index b74556b13..13b844033 100644 > --- a/emulator/serial.c > +++ b/emulator/serial.c > @@ -79,6 +79,7 @@ static void serial_read_callback(int fd, uint32_t events, void *user_data) > uint8_t *ptr = buf; > ssize_t len; > uint16_t count; > + ssize_t available; > > if (events & (EPOLLERR | EPOLLHUP)) { > mainloop_remove_fd(serial->fd); > @@ -87,8 +88,16 @@ static void serial_read_callback(int fd, uint32_t events, void *user_data) > } > > again: > + > + if(serial->pkt_offset > sizeof(buf)) { > + printf("packet offset overflow\n"); > + serial->pkt_offset = 0; > + return; > + } > + > + available = sizeof(buf) - serial->pkt_offset; > len = read(serial->fd, buf + serial->pkt_offset, > - sizeof(buf) - serial->pkt_offset); > + available); > if (len < 0) { > if (errno == EAGAIN) > goto again; > -- > 2.43.0 I suspect the whole idea of buf being static is not really necessary since pkt_data exists we can probably remove the static from buf: diff --git a/emulator/serial.c b/emulator/serial.c index b74556b13547..f8062ae5eac3 100644 --- a/emulator/serial.c +++ b/emulator/serial.c @@ -75,7 +75,7 @@ static void serial_write_callback(const struct iovec *iov, int iovlen, static void serial_read_callback(int fd, uint32_t events, void *user_data) { struct serial *serial = user_data; - static uint8_t buf[4096]; + uint8_t buf[4096]; uint8_t *ptr = buf; ssize_t len; uint16_t count; @@ -87,8 +87,7 @@ static void serial_read_callback(int fd, uint32_t events, void *user_data) } again: - len = read(serial->fd, buf + serial->pkt_offset, - sizeof(buf) - serial->pkt_offset); + len = read(serial->fd, buf, sizeof(buf)); if (len < 0) { if (errno == EAGAIN) goto again; @@ -98,7 +97,7 @@ again: if (!serial->btdev) return; - count = serial->pkt_offset + len; + count = len; while (count > 0) { hci_command_hdr *cmd_hdr;
Hi Pablo, On Tue, Feb 4, 2025 at 10:59 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Pablo, > > On Tue, Feb 4, 2025 at 5:17 AM Pablo Montes <pmontes@shsconsultores.es> wrote: > > > > Warning on read for a possible packet offset > > greater than buffer size is treated as error. > > > > I suggest using ssize_t so it is always positive. > > Returning if packet offset makes no sense might > > not discard the whole packet and start again > > > > --- > > emulator/serial.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/emulator/serial.c b/emulator/serial.c > > index b74556b13..13b844033 100644 > > --- a/emulator/serial.c > > +++ b/emulator/serial.c > > @@ -79,6 +79,7 @@ static void serial_read_callback(int fd, uint32_t events, void *user_data) > > uint8_t *ptr = buf; > > ssize_t len; > > uint16_t count; > > + ssize_t available; > > > > if (events & (EPOLLERR | EPOLLHUP)) { > > mainloop_remove_fd(serial->fd); > > @@ -87,8 +88,16 @@ static void serial_read_callback(int fd, uint32_t events, void *user_data) > > } > > > > again: > > + > > + if(serial->pkt_offset > sizeof(buf)) { > > + printf("packet offset overflow\n"); > > + serial->pkt_offset = 0; > > + return; > > + } > > + > > + available = sizeof(buf) - serial->pkt_offset; > > len = read(serial->fd, buf + serial->pkt_offset, > > - sizeof(buf) - serial->pkt_offset); > > + available); > > if (len < 0) { > > if (errno == EAGAIN) > > goto again; > > -- > > 2.43.0 > > I suspect the whole idea of buf being static is not really necessary > since pkt_data exists we can probably remove the static from buf: > > diff --git a/emulator/serial.c b/emulator/serial.c > index b74556b13547..f8062ae5eac3 100644 > --- a/emulator/serial.c > +++ b/emulator/serial.c > @@ -75,7 +75,7 @@ static void serial_write_callback(const struct iovec > *iov, int iovlen, > static void serial_read_callback(int fd, uint32_t events, void *user_data) > { > struct serial *serial = user_data; > - static uint8_t buf[4096]; > + uint8_t buf[4096]; > uint8_t *ptr = buf; > ssize_t len; > uint16_t count; > @@ -87,8 +87,7 @@ static void serial_read_callback(int fd, uint32_t > events, void *user_data) > } > > again: > - len = read(serial->fd, buf + serial->pkt_offset, > - sizeof(buf) - serial->pkt_offset); > + len = read(serial->fd, buf, sizeof(buf)); > if (len < 0) { > if (errno == EAGAIN) > goto again; > @@ -98,7 +97,7 @@ again: > if (!serial->btdev) > return; > > - count = serial->pkt_offset + len; > + count = len; > > while (count > 0) { > hci_command_hdr *cmd_hdr; > > > -- > Luiz Augusto von Dentz I'm trying to reproduce this, but even with -Wstringop-overflow it doesn't catch this case, perhaps I need -D_FORTIFY_SOURCE as well? Or maybe there is a way to detect what ubuntu is using so we can start including this as well.
De: Pablo Montes Lozano <pmontes@shsconsultores.es> Enviado: miércoles, 5 de febrero de 2025 9:57 Para: Luiz Augusto von Dentz <luiz.dentz@gmail.com> Cc: linux-bluetooth@vger.kernel.org <linux-bluetooth@vger.kernel.org> Asunto: RE: [PATCH BlueZ 1/2] emulator: Fix Werror=stringop-overflow Hi Luiz, I use uname -a Linux PCMALAGA-UB 6.8.0-52-generic #53-Ubuntu SMP PREEMPT_DYNAMIC Sat Jan 11 00:06:25 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux make --version GNU Make 4.3 Built for x86_64-pc-linux-gnu Copyright (C) 1988-2020 Free Software Foundation, Inc. Licence GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Full build steps: ============ After patch -------------- sudo apt build-dep bluez sudo apt install libsbc-dev libspeexdsp-dev <git clone and pull latest changes> cd bluez ./bootstrap-configure make distclean mkdir -p ../build && cd ../build ../bluez/configure --prefix=/home/bluez/usr --mandir=/home/bluez/man --sysconfdir=/home/bluez/etc \ --localstatedir=/home/bluez/var --enable-experimental --enable-mesh \ --enable-testing --enable-tools --enable-logger make make check Using latest changes make ends successfully. After the latest patches the warning does not appear to me. Checking out previous commit -------------------------------------- cd ../bluez git checkout e77884accdb22268eb65374fc96c35d9f8788d32 cd ../build-bluez && rm -rf * ../bluez/configure --prefix=/home/bluez/usr --mandir=/home/bluez/man --sysconfdir=/home/bluez/etc \ --localstatedir=/home/bluez/var --enable-experimental --enable-mesh \ --enable-testing --enable-tools --enable-logger make make check Make warns: CC emulator/serial.o In file included from /usr/include/features.h:502, from /usr/include/x86_64-linux-gnu/bits/libc-header-start.h:33, from /usr/include/stdio.h:28, from ../bluez/emulator/serial.c:17: In function ‘read’, inlined from ‘serial_read_callback’ at ../bluez/emulator/serial.c:90:8: /usr/include/x86_64-linux-gnu/bits/unistd.h:28:10: warning: ‘__read_alias’ specified size between 18446744073709490177 and 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=] 28 | return __glibc_fortify (read, __nbytes, sizeof (char), | ^~~~~~~~~~~~~~~ ../bluez/emulator/serial.c: In function ‘serial_read_callback’: ../bluez/emulator/serial.c:78:24: note: destination object allocated here 78 | static uint8_t buf[4096]; | ^~~ /usr/include/x86_64-linux-gnu/bits/unistd-decl.h:29:16: note: in a call to function ‘__read_alias’ declared with attribute ‘access (write_only, 2, 3)’ 29 | extern ssize_t __REDIRECT_FORTIFY (__read_alias, (int __fd, void *__buf, Maintainer build (previous commit) -------------------------------------------- Managed to reproduce the error cd ../build && make distclean && rm -rf * ../bluez/configure --prefix=/home/bluez/usr --mandir=/home/bluez/man --sysconfdir=/home/bluez/etc \ --localstatedir=/home/bluez/var --enable-experimental --enable-mesh \ --enable-testing --enable-tools --enable-logger --enable-maintainer-mode make CC emulator/serial.o In file included from /usr/include/features.h:502, from /usr/include/x86_64-linux-gnu/bits/libc-header-start.h:33, from /usr/include/stdio.h:28, from ../bluez/emulator/serial.c:17: In function ‘read’, inlined from ‘serial_read_callback’ at ../bluez/emulator/serial.c:90:8: /usr/include/x86_64-linux-gnu/bits/unistd.h:28:10: error: ‘__read_alias’ specified size between 18446744073709490177 and 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=] 28 | return __glibc_fortify (read, __nbytes, sizeof (char), | ^~~~~~~~~~~~~~~ ../bluez/emulator/serial.c: In function ‘serial_read_callback’: ../bluez/emulator/serial.c:78:24: note: destination object allocated here 78 | static uint8_t buf[4096]; | ^~~ /usr/include/x86_64-linux-gnu/bits/unistd-decl.h:29:16: note: in a call to function ‘__read_alias’ declared with attribute ‘access (write_only, 2, 3)’ 29 | extern ssize_t __REDIRECT_FORTIFY (__read_alias, (int __fd, void *__buf, | ^~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors Tests ------- A unit test fails in any case, but I don't think it is related to this issue PASS: unit/test-bass CC unit/test-vcp.o CCLD unit/test-vcp ../bluez/test-driver: line 112: 36129 Aborted (core dumped) "$@" >> "$log_file" 2>&1 FAIL: unit/test-vcp CC unit/test_mesh_crypto-test-mesh-crypto.o CCLD unit/test-mesh-crypto PASS: unit/test-mesh-crypto ============================================================================ Testsuite summary for bluez 5.79 ============================================================================ # TOTAL: 31 # PASS: 30 # SKIP: 0 # XFAIL: 0 # FAIL: 1 # XPASS: 0 # ERROR: 0 ============================================================================ See ./test-suite.log ============================================================================ cat ./test-suite.log AICS/SR/SGGIT/CP/BI-01-C - init AICS/SR/SGGIT/CP/BI-01-C - setup AICS/SR/SGGIT/CP/BI-01-C - setup complete AICS/SR/SGGIT/CP/BI-01-C - run AICS/SR/SGGIT/CP/BI-01-C - test passed AICS/SR/SGGIT/CP/BI-01-C - teardown AICS/SR/SGGIT/CP/BI-01-C - teardown complete AICS/SR/SGGIT/CP/BI-01-C - done AICS/SR/SGGIT/CP/BI-02-C - init AICS/SR/SGGIT/CP/BI-02-C - setup AICS/SR/SGGIT/CP/BI-02-C - setup complete AICS/SR/SGGIT/CP/BI-02-C - run AICS/SR/SGGIT/CP/BI** ERROR:../bluez/src/shared/tester.c:981:test_io_recv: assertion failed (len == iov->iov_len): (5 == 6) -02-C - test passed AICS/SR/SGGIT/CP/BI-02-C - teardown AICS/SR/SGGIT/CP/BI-02-C - teardown complete AICS/SR/SGGIT/CP/BI-02-C - done AICS/SR/CP/BV-01-C - init AICS/SR/CP/BV-01-C - setup AICS/SR/CP/BV-01-C - setup complete AICS/SR/CP/BV-01-C - run gatt_notify_cb: Failed to send notification Bail out! ERROR:../bluez/src/shared/tester.c:981:test_io_recv: assertion failed (len == iov->iov_len): (5 == 6) FAIL unit/test-vcp (exit status: 134)
diff --git a/emulator/serial.c b/emulator/serial.c index b74556b13..13b844033 100644 --- a/emulator/serial.c +++ b/emulator/serial.c @@ -79,6 +79,7 @@ static void serial_read_callback(int fd, uint32_t events, void *user_data) uint8_t *ptr = buf; ssize_t len; uint16_t count; + ssize_t available; if (events & (EPOLLERR | EPOLLHUP)) { mainloop_remove_fd(serial->fd); @@ -87,8 +88,16 @@ static void serial_read_callback(int fd, uint32_t events, void *user_data) } again: + + if(serial->pkt_offset > sizeof(buf)) { + printf("packet offset overflow\n"); + serial->pkt_offset = 0; + return; + } + + available = sizeof(buf) - serial->pkt_offset; len = read(serial->fd, buf + serial->pkt_offset, - sizeof(buf) - serial->pkt_offset); + available); if (len < 0) { if (errno == EAGAIN) goto again;