[PATCHv2 01/10] monitor: Fix segmentation fault with malformed packet

[PATCHv2 01/10] monitor: Fix segmentation fault with malformed packet

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

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;
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)
+
 struct btsnoop_opcode_new_index {
 	uint8_t  type;
 	uint8_t  bus;
-- 
1.9.1

[PATCHv2 02/10] monitor: Handle default switch case

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Fixes issues with junk packets.
---
 monitor/analyze.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/monitor/analyze.c b/monitor/analyze.c
index 7aa98b0..9ec6b06 100644
--- a/monitor/analyze.c
+++ b/monitor/analyze.c
@@ -339,6 +339,9 @@ void analyze_trace(const char *path)
 		case BTSNOOP_OPCODE_SCO_RX_PKT:
 			sco_pkt(&tv, index, true, buf, pktlen);
 			break;
+		default:
+			fprintf(stderr, "Wrong opcode %u", opcode);
+			goto done;
 		}
 
 		num_packets++;
-- 
1.9.1

Re: [PATCHv2 02/10] monitor: Handle default switch case

[Johan Hedberg]

Hi Andrei,

On Mon, Aug 11, 2014, Andrei Emeltchenko wrote:
> Fixes issues with junk packets.
> ---
>  monitor/analyze.c | 3 +++
>  1 file changed, 3 insertions(+)

This one doesn't apply cleanly. Please rebase and resend:

Applying: monitor: Handle default switch case
error: patch failed: monitor/analyze.c:339
error: monitor/analyze.c: patch does not apply
Patch failed at 0001 monitor: Handle default switch case

Johan

[PATCHv2 03/10] emulator: Fix resource leak closing fd

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

---
 emulator/b1ee.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/emulator/b1ee.c b/emulator/b1ee.c
index 17a60fc..3fb867d 100644
--- a/emulator/b1ee.c
+++ b/emulator/b1ee.c
@@ -203,6 +203,7 @@ static int do_connect(const char *node, const char *service)
 
 		if (connect(fd, res->ai_addr, res->ai_addrlen) < 0) {
 			perror("Failed to connect");
+			close(fd);
 			continue;
 		}
 
-- 
1.9.1

[PATCHv2 04/10] tools: Fix resource leak closing sk

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

---
 tools/avinfo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/avinfo.c b/tools/avinfo.c
index 7d58e25..d9f809b 100644
--- a/tools/avinfo.c
+++ b/tools/avinfo.c
@@ -583,6 +583,7 @@ static int l2cap_connect(bdaddr_t *src, bdaddr_t *dst)
 
 	if (bind(sk, (struct sockaddr *) &l2a, sizeof(l2a)) < 0) {
 		printf("Bind failed. %s (%d)\n", strerror(errno), errno);
+		close(sk);
 		return -errno;
 	}
 
@@ -593,6 +594,7 @@ static int l2cap_connect(bdaddr_t *src, bdaddr_t *dst)
 
 	if (connect(sk, (struct sockaddr *) &l2a, sizeof(l2a)) < 0) {
 		printf("Connect failed. %s(%d)\n", strerror(errno), errno);
+		close(sk);
 		return -errno;
 	}
 
-- 
1.9.1

[PATCHv2 05/10] gobex: Fix use after free

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

It is better not to dereference freed pointer
---
 gobex/gobex.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gobex/gobex.c b/gobex/gobex.c
index 3848884..16a4aae 100644
--- a/gobex/gobex.c
+++ b/gobex/gobex.c
@@ -250,6 +250,7 @@ static gboolean req_timeout(gpointer user_data)
 
 	g_assert(p != NULL);
 
+	p->timeout_id = 0;
 	obex->pending_req = NULL;
 
 	err = g_error_new(G_OBEX_ERROR, G_OBEX_ERROR_TIMEOUT,
@@ -263,8 +264,6 @@ static gboolean req_timeout(gpointer user_data)
 	g_error_free(err);
 	pending_pkt_free(p);
 
-	p->timeout_id = 0;
-
 	return FALSE;
 }
 
-- 
1.9.1

[PATCHv2 06/10] gobex: Fix use after free

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

---
 gobex/gobex-transfer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c
index 6dc7d9f..efae72b 100644
--- a/gobex/gobex-transfer.c
+++ b/gobex/gobex-transfer.c
@@ -406,6 +406,7 @@ static void transfer_put_req(GObex *obex, GObexPacket *req, gpointer user_data)
 	if (!g_obex_send(obex, rsp, &err)) {
 		transfer_complete(transfer, err);
 		g_error_free(err);
+		return;
 	}
 
 done:
-- 
1.9.1

[PATCHv2 07/10] gobex: Fix use after free

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Refactor function transfer_get_req_first() to avoid use after free.
---
 gobex/gobex-transfer.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c
index efae72b..d7707f9 100644
--- a/gobex/gobex-transfer.c
+++ b/gobex/gobex-transfer.c
@@ -553,7 +553,8 @@ static gssize get_get_data(void *buf, gsize len, gpointer user_data)
 	return ret;
 }
 
-static void transfer_get_req_first(struct transfer *transfer, GObexPacket *rsp)
+static gboolean transfer_get_req_first(struct transfer *transfer,
+							GObexPacket *rsp)
 {
 	GError *err = NULL;
 
@@ -564,7 +565,10 @@ static void transfer_get_req_first(struct transfer *transfer, GObexPacket *rsp)
 	if (!g_obex_send(transfer->obex, rsp, &err)) {
 		transfer_complete(transfer, err);
 		g_error_free(err);
+		return FALSE;
 	}
+
+	return TRUE;
 }
 
 static void transfer_get_req(GObex *obex, GObexPacket *req, gpointer user_data)
@@ -596,7 +600,8 @@ guint g_obex_get_rsp_pkt(GObex *obex, GObexPacket *rsp,
 	transfer = transfer_new(obex, G_OBEX_OP_GET, complete_func, user_data);
 	transfer->data_producer = data_func;
 
-	transfer_get_req_first(transfer, rsp);
+	if (!transfer_get_req_first(transfer, rsp))
+		return 0;
 
 	if (!g_slist_find(transfers, transfer))
 		return 0;
-- 
1.9.1

[PATCHv2 08/10] proximity: Fix use after free

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

First remove object from the list and then remove it.
---
 profiles/proximity/monitor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/profiles/proximity/monitor.c b/profiles/proximity/monitor.c
index f2e0739..b05cdd7 100644
--- a/profiles/proximity/monitor.c
+++ b/profiles/proximity/monitor.c
@@ -606,13 +606,13 @@ static void monitor_destroy(gpointer user_data)
 {
 	struct monitor *monitor = user_data;
 
+	monitors = g_slist_remove(monitors, monitor);
+
 	btd_device_unref(monitor->device);
 	g_free(monitor->linklosslevel);
 	g_free(monitor->immediatelevel);
 	g_free(monitor->signallevel);
 	g_free(monitor);
-
-	monitors = g_slist_remove(monitors, monitor);
 }
 
 static struct monitor *register_monitor(struct btd_device *device)
-- 
1.9.1

[PATCHv2 09/10] gdbus: Fix use after free

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Refactor filter_data_remove_callback so that we do not iterate over
freed pointer.
---
 gdbus/watch.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index 0f99f4f..474d3d4 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -362,6 +362,7 @@ static void service_data_free(struct service_data *data)
 	callback->data = NULL;
 }
 
+/* Returns TRUE if data is freed */
 static gboolean filter_data_remove_callback(struct filter_data *data,
 						struct filter_callback *cb)
 {
@@ -383,7 +384,7 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
 	/* Don't remove the filter if other callbacks exist or data is lock
 	 * processing callbacks */
 	if (data->callbacks || data->lock)
-		return TRUE;
+		return FALSE;
 
 	if (data->registered && !remove_match(data))
 		return FALSE;
@@ -405,7 +406,9 @@ static DBusHandlerResult signal_filter(DBusConnection *connection,
 
 		if (cb->signal_func && !cb->signal_func(connection, message,
 							cb->user_data)) {
-			filter_data_remove_callback(data, cb);
+			if (filter_data_remove_callback(data, cb))
+				break;
+
 			continue;
 		}
 
@@ -489,7 +492,9 @@ static DBusHandlerResult service_filter(DBusConnection *connection,
 		/* Only auto remove if it is a bus name watch */
 		if (data->argument[0] == ':' &&
 				(cb->conn_func == NULL || cb->disc_func == NULL)) {
-			filter_data_remove_callback(data, cb);
+			if (filter_data_remove_callback(data, cb))
+				break;
+
 			continue;
 		}
 
-- 
1.9.1

[PATCHv2 10/10] obexd: Fix possible NULL dereference

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

In a case snprintf fails we have NULL dereference. Fixes clang warnings
below:
...
obexd/client/map.c:471:9: warning: Access to field 'message' results in
a dereference of a null pointer (loaded from variable 'err')
                                                           err->message);
                                                           ^~~~~~~~~~~~
obexd/client/map.c:772:9: warning: Access to field 'message' results in
a dereference of a null pointer (loaded from variable 'err')
                                                           err->message);
                                                           ^~~~~~~~~~~~
...
---
 obexd/client/map.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/obexd/client/map.c b/obexd/client/map.c
index 47afc31..ed535e2 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -468,7 +468,7 @@ static DBusMessage *map_msg_get(DBusConnection *connection,
 
 fail:
 	reply = g_dbus_create_error(message, ERROR_INTERFACE ".Failed", "%s",
-								err->message);
+						err ? err->message : "");
 	g_error_free(err);
 	return reply;
 }
@@ -769,7 +769,7 @@ static void set_status(const GDBusPropertyTable *property,
 
 fail:
 	g_dbus_pending_property_error(id, ERROR_INTERFACE ".Failed", "%s",
-								err->message);
+						err ? err->message : "");
 	g_error_free(err);
 }
 
-- 
1.9.1

Re: [PATCHv2 10/10] obexd: Fix possible NULL dereference

[Johan Hedberg]

Hi Andrei,

On Mon, Aug 11, 2014, Andrei Emeltchenko wrote:
> In a case snprintf fails we have NULL dereference. Fixes clang warnings
> below:
> ...
> obexd/client/map.c:471:9: warning: Access to field 'message' results in
> a dereference of a null pointer (loaded from variable 'err')
>                                                            err->message);
>                                                            ^~~~~~~~~~~~
> obexd/client/map.c:772:9: warning: Access to field 'message' results in
> a dereference of a null pointer (loaded from variable 'err')
>                                                            err->message);
>                                                            ^~~~~~~~~~~~
> ...
> ---
>  obexd/client/map.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

I've applied patches 3-9 (1 & 2 already had feedback). This one needs a
bit more consideration too:

> diff --git a/obexd/client/map.c b/obexd/client/map.c
> index 47afc31..ed535e2 100644
> --- a/obexd/client/map.c
> +++ b/obexd/client/map.c
> @@ -468,7 +468,7 @@ static DBusMessage *map_msg_get(DBusConnection *connection,
>  
>  fail:
>  	reply = g_dbus_create_error(message, ERROR_INTERFACE ".Failed", "%s",
> -								err->message);
> +						err ? err->message : "");
>  	g_error_free(err);
>  	return reply;
>  }
> @@ -769,7 +769,7 @@ static void set_status(const GDBusPropertyTable *property,
>  
>  fail:
>  	g_dbus_pending_property_error(id, ERROR_INTERFACE ".Failed", "%s",
> -								err->message);
> +						err ? err->message : "");
>  	g_error_free(err);
>  }

It seems to me that the only code path that can lead to err being still
NULL is this one:

        if (snprintf(handle, sizeof(handle), "%" PRIx64, msg->handle) < 0)
                goto fail;

All others should have err != NULL. I don't really see how snprintf
could ever fail in this case, so probably the simplest solution would be
to just remove the error check there?

Johan

Re: [PATCHv2 10/10] obexd: Fix possible NULL dereference

[Andrei Emeltchenko]

Hi Johan,

On Mon, Aug 11, 2014 at 04:37:20PM +0300, Johan Hedberg wrote:
> Hi Andrei,
> 
> On Mon, Aug 11, 2014, Andrei Emeltchenko wrote:
> > In a case snprintf fails we have NULL dereference. Fixes clang warnings
> > below:
> > ...
> > obexd/client/map.c:471:9: warning: Access to field 'message' results in
> > a dereference of a null pointer (loaded from variable 'err')
> >                                                            err->message);
> >                                                            ^~~~~~~~~~~~
> > obexd/client/map.c:772:9: warning: Access to field 'message' results in
> > a dereference of a null pointer (loaded from variable 'err')
> >                                                            err->message);
> >                                                            ^~~~~~~~~~~~
> > ...
> > ---
> >  obexd/client/map.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> I've applied patches 3-9 (1 & 2 already had feedback). This one needs a
> bit more consideration too:
> 
> > diff --git a/obexd/client/map.c b/obexd/client/map.c
> > index 47afc31..ed535e2 100644
> > --- a/obexd/client/map.c
> > +++ b/obexd/client/map.c
> > @@ -468,7 +468,7 @@ static DBusMessage *map_msg_get(DBusConnection *connection,
> >  
> >  fail:
> >  	reply = g_dbus_create_error(message, ERROR_INTERFACE ".Failed", "%s",
> > -								err->message);
> > +						err ? err->message : "");
> >  	g_error_free(err);
> >  	return reply;
> >  }
> > @@ -769,7 +769,7 @@ static void set_status(const GDBusPropertyTable *property,
> >  
> >  fail:
> >  	g_dbus_pending_property_error(id, ERROR_INTERFACE ".Failed", "%s",
> > -								err->message);
> > +						err ? err->message : "");
> >  	g_error_free(err);
> >  }
> 
> It seems to me that the only code path that can lead to err being still
> NULL is this one:
> 
>         if (snprintf(handle, sizeof(handle), "%" PRIx64, msg->handle) < 0)
>                 goto fail;
> 
> All others should have err != NULL. I don't really see how snprintf
> could ever fail in this case, so probably the simplest solution would be
> to just remove the error check there?

OK, I will remove check.

Best regards 
Andrei Emeltchenko 

Re: [PATCHv2 01/10] monitor: Fix segmentation fault with malformed packet

[Johan Hedberg]

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.

> 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_*.

Johan

Re: [PATCHv2 01/10] monitor: Fix segmentation fault with malformed packet

[Andrei Emeltchenko]

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 

Re: [PATCHv2 01/10] monitor: Fix segmentation fault with malformed packet

[Johan Hedberg]

Hi Andrei,

On Tue, Aug 12, 2014, Andrei Emeltchenko wrote:
> 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)

Let's fix that then and explain it in the code.

> Maybe insetad of btsnoop.h I define it in src/shared/btsnoop.c

Sounds to me like all those places include btsnoop.h, meaning a common
define in btsnoop.h might be the best way forward.

Johan

[PATCHv3 1/5] monitor: Use common maximum packet size definition

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

---
 android/bluetoothd-snoop.c | 4 +---
 monitor/analyze.c          | 4 +---
 monitor/control.c          | 6 ++----
 src/shared/btsnoop.h       | 2 ++
 4 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/android/bluetoothd-snoop.c b/android/bluetoothd-snoop.c
index 57f97f4..dc34869 100644
--- a/android/bluetoothd-snoop.c
+++ b/android/bluetoothd-snoop.c
@@ -42,10 +42,8 @@
 
 #define DEFAULT_SNOOP_FILE "/sdcard/btsnoop_hci.log"
 
-#define MAX_PACKET_SIZE (1486 + 4)
-
 static struct btsnoop *snoop = NULL;
-static uint8_t monitor_buf[MAX_PACKET_SIZE];
+static uint8_t monitor_buf[BTSNOOP_MAX_PACKET_SIZE];
 static int monitor_fd = -1;
 
 static void signal_callback(int signum, void *user_data)
diff --git a/monitor/analyze.c b/monitor/analyze.c
index 5288cf3..b027b3c 100644
--- a/monitor/analyze.c
+++ b/monitor/analyze.c
@@ -35,8 +35,6 @@
 #include "monitor/bt.h"
 #include "analyze.h"
 
-#define MAX_PACKET_SIZE		(1486 + 4)
-
 struct hci_dev {
 	uint16_t index;
 	uint8_t type;
@@ -282,7 +280,7 @@ void analyze_trace(const char *path)
 	}
 
 	while (1) {
-		unsigned char buf[MAX_PACKET_SIZE];
+		unsigned char buf[BTSNOOP_MAX_PACKET_SIZE];
 		struct timeval tv;
 		uint16_t index, opcode, pktlen;
 
diff --git a/monitor/control.c b/monitor/control.c
index 8ee4431..36ebcbc 100644
--- a/monitor/control.c
+++ b/monitor/control.c
@@ -52,12 +52,10 @@
 static struct btsnoop *btsnoop_file = NULL;
 static bool hcidump_fallback = false;
 
-#define MAX_PACKET_SIZE		(1486 + 4)
-
 struct control_data {
 	uint16_t channel;
 	int fd;
-	unsigned char buf[MAX_PACKET_SIZE];
+	unsigned char buf[BTSNOOP_MAX_PACKET_SIZE];
 	uint16_t offset;
 };
 
@@ -1005,7 +1003,7 @@ bool control_writer(const char *path)
 
 void control_reader(const char *path)
 {
-	unsigned char buf[MAX_PACKET_SIZE];
+	unsigned char buf[BTSNOOP_MAX_PACKET_SIZE];
 	uint16_t pktlen;
 	uint32_t type;
 	struct timeval tv;
diff --git a/src/shared/btsnoop.h b/src/shared/btsnoop.h
index 2c55d02..9675980 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 BTSNOOP_MAX_PACKET_SIZE		(1486 + 4)
+
 struct btsnoop_opcode_new_index {
 	uint8_t  type;
 	uint8_t  bus;
-- 
1.9.1

[PATCHv3 2/5] shared/btsnoop: Fix segmentation fault with malformed packet

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Do not allow to read more then buffer size.
This fixes segmentation fault reading capture from target (apparently
end of the trace was broken).
---
 src/shared/btsnoop.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/shared/btsnoop.c b/src/shared/btsnoop.c
index 17a872c..3592c2e 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 > BTSNOOP_MAX_PACKET_SIZE) {
+		btsnoop->aborted = true;
+		return false;
+	}
+
 	flags = be32toh(pkt.flags);
 
 	ts = be64toh(pkt.ts) - 0x00E03AB44A676000ll;
-- 
1.9.1

[PATCHv3 3/5] btmon: Fix segmentation fault with malformed packet

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

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 +++++++
 1 file changed, 7 insertions(+)

diff --git a/monitor/btsnoop.c b/monitor/btsnoop.c
index fafeff8..83aaee5 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 > BTSNOOP_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;
-- 
1.9.1

[PATCHv3 4/5] monitor: Handle default switch case

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Fixes issues with junk packets.
---
 monitor/analyze.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/monitor/analyze.c b/monitor/analyze.c
index b027b3c..4c52d12 100644
--- a/monitor/analyze.c
+++ b/monitor/analyze.c
@@ -309,6 +309,9 @@ void analyze_trace(const char *path)
 		case BTSNOOP_OPCODE_SCO_RX_PKT:
 			sco_pkt(&tv, index, buf, pktlen);
 			break;
+		default:
+			fprintf(stderr, "Wrong opcode %u", opcode);
+			goto done;
 		}
 
 		num_packets++;
-- 
1.9.1

Re: [PATCHv3 4/5] monitor: Handle default switch case

[Johan Hedberg]

Hi Andrei,

On Tue, Aug 12, 2014, Andrei Emeltchenko wrote:
> Fixes issues with junk packets.
> ---
>  monitor/analyze.c | 3 +++
>  1 file changed, 3 insertions(+)

I've applied the four other patches, but there's a small fix needed for
this one:

> +		default:
> +			fprintf(stderr, "Wrong opcode %u", opcode);
> +			goto done;

Seems like you've forgotten a \n at the end of the string.

Johan

[PATCHv4] monitor: Handle default switch case

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Fixes issues with junk packets.
---
 monitor/analyze.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/monitor/analyze.c b/monitor/analyze.c
index b027b3c..eea3c9e 100644
--- a/monitor/analyze.c
+++ b/monitor/analyze.c
@@ -309,6 +309,9 @@ void analyze_trace(const char *path)
 		case BTSNOOP_OPCODE_SCO_RX_PKT:
 			sco_pkt(&tv, index, buf, pktlen);
 			break;
+		default:
+			fprintf(stderr, "Wrong opcode %u\n", opcode);
+			goto done;
 		}
 
 		num_packets++;
-- 
1.9.1

Re: [PATCHv4] monitor: Handle default switch case

[Johan Hedberg]

Hi Andrei,

On Wed, Aug 13, 2014, Andrei Emeltchenko wrote:
> Fixes issues with junk packets.
> ---
>  monitor/analyze.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied. Thanks.

Johan

[PATCHv3 5/5] obexd: Fix possible NULL dereference

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Remove snprintf error check. Fixes clang warnings below:
...
obexd/client/map.c:471:9: warning: Access to field 'message' results in
a dereference of a null pointer (loaded from variable 'err')
                                                           err->message);
                                                           ^~~~~~~~~~~~
obexd/client/map.c:772:9: warning: Access to field 'message' results in
a dereference of a null pointer (loaded from variable 'err')
                                                           err->message);
                                                           ^~~~~~~~~~~~
...
---
 obexd/client/map.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/obexd/client/map.c b/obexd/client/map.c
index 47afc31..520e492 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -447,8 +447,7 @@ static DBusMessage *map_msg_get(DBusConnection *connection,
 		return g_dbus_create_error(message,
 				ERROR_INTERFACE ".InvalidArguments", NULL);
 
-	if (snprintf(handle, sizeof(handle), "%" PRIx64, msg->handle) < 0)
-		goto fail;
+	snprintf(handle, sizeof(handle), "%" PRIx64, msg->handle);
 
 	transfer = obc_transfer_get("x-bt/message", handle, target_file, &err);
 	if (transfer == NULL)
@@ -746,8 +745,7 @@ static void set_status(const GDBusPropertyTable *property,
 
 	contents[0] = FILLER_BYTE;
 
-	if (snprintf(handle, sizeof(handle), "%" PRIx64, msg->handle) < 0)
-		goto fail;
+	snprintf(handle, sizeof(handle), "%" PRIx64, msg->handle);
 
 	transfer = obc_transfer_put("x-bt/messageStatus", handle, NULL,
 					contents, sizeof(contents), &err);
-- 
1.9.1