Hi Pablo,
On Tue, Feb 4, 2025 at 10:59 AM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Pablo,
>
> On Tue, Feb 4, 2025 at 5:17 AM Pablo Montes <pmontes@xxxxxxxxxxxxxxxxx> 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
