[PATCH BlueZ 0/2] Fix Ubuntu 24.04 build error

[PATCH BlueZ 0/2] Fix Ubuntu 24.04 build error

[Pablo Montes]

On Ubuntu 24.04: build warnings for buffer read overflow are treated
as errors on emulator. This may be ignored with 
CFLAGS="-Wno-error=stringop-overflow" but discarding packet if offset
is greater than buffer size removes the warning.
Could close GitHub issue #1049.

Also I lost some time using mesh-cfgclient on a fresh Linux with no
.config folder in home: mesh-cfgclient failed but did not explain the
error, so printing a message could be welcome for others.

From instructions I read patches should be separate,
but I thought the minor print error did not deserve a full patch.

Tests are OK, save test-vcp which does not work on master branch
either and it is not related to this.

Pablo Montes (2):
  emulator: Fix Werror=stringop-overflow
  tools: print error on mkdir

 emulator/serial.c      | 11 ++++++++++-
 tools/mesh-cfgclient.c |  4 +++-
 2 files changed, 13 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH BlueZ 1/2] emulator: Fix Werror=stringop-overflow

[Pablo Montes]

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

RE: Fix Ubuntu 24.04 build error

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1260 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=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

Re: [PATCH BlueZ 1/2] emulator: Fix Werror=stringop-overflow

[Luiz Augusto von Dentz]

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

Re: [PATCH BlueZ 1/2] emulator: Fix Werror=stringop-overflow

[Luiz Augusto von Dentz]

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.



-- 
Luiz Augusto von Dentz

RV: [PATCH BlueZ 1/2] emulator: Fix Werror=stringop-overflow

[Pablo Montes Lozano]

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)




________________________________________
De: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Enviado: martes, 4 de febrero de 2025 17:38
Para: Pablo Montes Lozano <pmontes@shsconsultores.es>
Cc: linux-bluetooth@vger.kernel.org <linux-bluetooth@vger.kernel.org>
Asunto: Re: [PATCH BlueZ 1/2] emulator: Fix Werror=stringop-overflow
 
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.



--
Luiz Augusto von Dentz

[PATCH BlueZ 2/2] tools: print error on mkdir

[Pablo Montes]

On some fresh installations where XDG_CONFIG_SESSION is not defined, and $HOME/.config is not created yet, mesh-cfgclient failed with no hints
---
 tools/mesh-cfgclient.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/mesh-cfgclient.c b/tools/mesh-cfgclient.c
index 3bd2b673a..b1d091be7 100644
--- a/tools/mesh-cfgclient.c
+++ b/tools/mesh-cfgclient.c
@@ -2529,8 +2529,10 @@ static bool setup_cfg_storage(void)
 				return false;
 			}
 		} else if (errno == ENOENT) {
-			if (mkdir(mesh_dir, 0700) != 0)
+			if (mkdir(mesh_dir, 0700) != 0) {
+				l_error("Cannot create %s", mesh_dir);
 				return false;
+			}
 		} else {
 			perror("Cannot open config directory");
 			return false;
-- 
2.43.0

Re: [PATCH BlueZ 0/2] Fix Ubuntu 24.04 build error

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Tue,  4 Feb 2025 11:16:10 +0100 you wrote:
> On Ubuntu 24.04: build warnings for buffer read overflow are treated
> as errors on emulator. This may be ignored with
> CFLAGS="-Wno-error=stringop-overflow" but discarding packet if offset
> is greater than buffer size removes the warning.
> Could close GitHub issue #1049.
> 
> Also I lost some time using mesh-cfgclient on a fresh Linux with no
> .config folder in home: mesh-cfgclient failed but did not explain the
> error, so printing a message could be welcome for others.
> 
> [...]

Here is the summary with links:
  - [BlueZ,1/2] emulator: Fix Werror=stringop-overflow
    (no matching commit)
  - [BlueZ,2/2] tools: print error on mkdir
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=9dbc92a360b2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html