Hi Johan,
On Mon, Aug 11, 2014 at 04:22:33PM +0300, Johan Hedberg wrote:
> Hi Andrei,
>
> On Mon, Aug 11, 2014, Andrei Emeltchenko wrote:
> > Do not allow to read more then buffer size.
> > This fixes segmentation fault reading capture from target (apparently
> > end of the trace was broken).
> > ---
> > monitor/btsnoop.c | 7 +++++++
> > src/shared/btsnoop.c | 5 +++++
> > src/shared/btsnoop.h | 2 ++
> > 3 files changed, 14 insertions(+)
> >
> > diff --git a/monitor/btsnoop.c b/monitor/btsnoop.c
> > index fafeff8..ec19812 100644
> > --- a/monitor/btsnoop.c
> > +++ b/monitor/btsnoop.c
> > @@ -304,6 +304,13 @@ int btsnoop_read_hci(struct timeval *tv, uint16_t *index, uint16_t *opcode,
> > }
> >
> > toread = be32toh(pkt.size);
> > + if (toread > MAX_PACKET_SIZE) {
> > + perror("Packet len suspicially big: %u", toread);
> > + close(btsnoop_fd);
> > + btsnoop_fd = -1;
> > + return -1;
> > + }
> > +
> > flags = be32toh(pkt.flags);
> >
> > ts = be64toh(pkt.ts) - 0x00E03AB44A676000ll;
> > diff --git a/src/shared/btsnoop.c b/src/shared/btsnoop.c
> > index 17a872c..521be10 100644
> > --- a/src/shared/btsnoop.c
> > +++ b/src/shared/btsnoop.c
> > @@ -415,6 +415,11 @@ bool btsnoop_read_hci(struct btsnoop *btsnoop, struct timeval *tv,
> > }
> >
> > toread = be32toh(pkt.size);
> > + if (toread > MAX_PACKET_SIZE) {
> > + btsnoop->aborted = true;
> > + return false;
> > + }
> > +
> > flags = be32toh(pkt.flags);
> >
> > ts = be64toh(pkt.ts) - 0x00E03AB44A676000ll;
>
> The above two chunks should probably be in separate patches. One for
> shared/btsnoop and the other for btmon.
OK.
>
> > diff --git a/src/shared/btsnoop.h b/src/shared/btsnoop.h
> > index 2c55d02..9f73913 100644
> > --- a/src/shared/btsnoop.h
> > +++ b/src/shared/btsnoop.h
> > @@ -44,6 +44,8 @@
> > #define BTSNOOP_OPCODE_SCO_TX_PKT 6
> > #define BTSNOOP_OPCODE_SCO_RX_PKT 7
> >
> > +#define MAX_PACKET_SIZE (1486 + 4)
>
> Where does this number come from? At least provide an explanation in the
> form of a code comment so that the reader can determine that it is
> correct. Also, you're violating the name space used by this header file.
> Everything else in it is prefixed by btsnoop_* or BTSNOOP_*.
MAX_PACKET_SIZE is defined in many places and NEVER explained.
1 45 android/bluetoothd-snoop.c <<MAX_PACKET_SIZE>>
#define MAX_PACKET_SIZE (1486 + 4)
2 42 monitor/analyze.c <<MAX_PACKET_SIZE>>
#define MAX_PACKET_SIZE (1486 + 4)
3 55 monitor/control.c <<MAX_PACKET_SIZE>>
#define MAX_PACKET_SIZE (1486 + 4)
4 47 src/shared/btsnoop.h <<MAX_PACKET_SIZE>>
#define MAX_PACKET_SIZE (1486 + 4)
Maybe insetad of btsnoop.h I define it in src/shared/btsnoop.c
Best regards
Andrei Emeltchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
