On Tue, 2024-08-06 at 14:38 -0400, Luiz Augusto von Dentz wrote:
> Hi Bastien,
>
> On Mon, Aug 5, 2024 at 10:09 AM Bastien Nocera <hadess@xxxxxxxxxx>
> wrote:
> >
> > Error: INTEGER_OVERFLOW (CWE-190): [#def1] [important]
> > bluez-5.77/lib/sdp.c:1685:2: tainted_data_argument: The check "sent
> > < size" contains the tainted expression "sent" which causes "size"
> > to be considered tainted.
> > bluez-5.77/lib/sdp.c:1686:3: overflow: The expression "size - sent"
> > is deemed overflowed because at least one of its arguments has
> > overflowed.
> > bluez-5.77/lib/sdp.c:1686:3: overflow_sink: "size - sent", which
> > might have underflowed, is passed to "send(session->sock, buf +
> > sent, size - sent, 0)".
> > 1684|
> > 1685| while (sent < size) {
> > 1686|-> int n = send(session->sock, buf + sent,
> > size - sent, 0);
> > 1687| if (n < 0)
> > 1688| return -1;
> > ---
> > lib/sdp.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/sdp.c b/lib/sdp.c
> > index 411a95b8a7d3..8a15ad803db1 100644
> > --- a/lib/sdp.c
> > +++ b/lib/sdp.c
> > @@ -1678,13 +1678,13 @@ sdp_data_t *sdp_data_get(const sdp_record_t
> > *rec, uint16_t attrId)
> > return NULL;
> > }
> >
> > -static int sdp_send_req(sdp_session_t *session, uint8_t *buf,
> > uint32_t size)
> > +static int sdp_send_req(sdp_session_t *session, uint8_t *buf,
> > size_t size)
> > {
> > - uint32_t sent = 0;
> > + size_t sent = 0;
> >
> > while (sent < size) {
> > int n = send(session->sock, buf + sent, size -
> > sent, 0);
> > - if (n < 0)
> > + if (n < 0 || sent > SIZE_MAX - n)
> > return -1;
>
> Not really following you here, it seems the problem is that sent + n
> could overflow causing sent to loop around but if that happens don't
> we get a problem with send syscall itself? Using size_t makes sense
> but I guess we want n to also be ssize_t then, and perhap use if (n <
> 0 || n > size - sent) logic if we cannot trust syscalls returns the
> correct number of bytes.
I don't think that the static analysis is this clever, it just wants to
make sure that bounds checking is done to avoid integer overflows.
I'm fine with quieting Coverity on my end, after fixing the data types
if that's alright with you.
Also goes for patch 8 of this series.
>
> > sent += n;
> > }
> > --
> > 2.45.2
> >
> >
>
>
