[PATCH 1/2] bluetooth/hidhost: Fix using feature event as output event

[PATCH 1/2] bluetooth/hidhost: Fix using feature event as output event

[Szymon Janc]

Rename handle_uhid_event to handle_uhid_output and make it handle only
output events.
---
 android/hidhost.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/android/hidhost.c b/android/hidhost.c
index 8bfdfed..0e0391a 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -153,7 +153,8 @@ static void hid_device_free(struct hid_device *dev)
 	g_free(dev);
 }
 
-static void handle_uhid_event(struct hid_device *dev, struct uhid_event *ev)
+static void handle_uhid_output(struct hid_device *dev,
+						struct uhid_output_req *output)
 {
 	int fd, i;
 	uint8_t *req = NULL;
@@ -162,15 +163,14 @@ static void handle_uhid_event(struct hid_device *dev, struct uhid_event *ev)
 	if (!(dev->ctrl_io))
 		return;
 
-	req_size = 1 + (ev->u.output.size / 2);
+	req_size = 1 + (output->size / 2);
 	req = g_try_malloc0(req_size);
 	if (!req)
 		return;
 
-	req[0] = HID_MSG_SET_REPORT | ev->u.output.rtype;
+	req[0] = HID_MSG_SET_REPORT | output->rtype;
 	for (i = 0; i < (req_size - 1); i++)
-		sscanf((char *) &(ev->u.output.data)[i * 2],
-							"%hhx", &req[1 + i]);
+		sscanf((char *) &(output->data)[i * 2], "%hhx", &req[1 + i]);
 
 	fd = g_io_channel_unix_get_fd(dev->ctrl_io);
 
@@ -224,8 +224,10 @@ static gboolean uhid_event_cb(GIOChannel *io, GIOCondition cond,
 		 * asleep This is optional, though. */
 		break;
 	case UHID_OUTPUT:
+		handle_uhid_output(dev, &ev.u.output);
+		break;
 	case UHID_FEATURE:
-		handle_uhid_event(dev, &ev);
+		/* TODO */
 		break;
 	case UHID_OUTPUT_EV:
 		/* This is only sent by kernels prior to linux-3.11. It
-- 
1.8.3.2

[PATCH 2/2] android/hidhost: Simplify handle_uhid_output

[Szymon Janc]

Make it use VLA for req buffer. This makes function simpler and also
fix cutting req to 255 bytes (req_len was uint8_t)
---
 android/hidhost.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/android/hidhost.c b/android/hidhost.c
index 0e0391a..76322af 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -156,29 +156,22 @@ static void hid_device_free(struct hid_device *dev)
 static void handle_uhid_output(struct hid_device *dev,
 						struct uhid_output_req *output)
 {
-	int fd, i;
-	uint8_t *req = NULL;
-	uint8_t req_size = 0;
+	int fd;
+	unsigned int i;
+	uint8_t req[1 + (output->size / 2)];
 
 	if (!(dev->ctrl_io))
 		return;
 
-	req_size = 1 + (output->size / 2);
-	req = g_try_malloc0(req_size);
-	if (!req)
-		return;
-
 	req[0] = HID_MSG_SET_REPORT | output->rtype;
-	for (i = 0; i < (req_size - 1); i++)
+	for (i = 0; i < (sizeof(req) - 1); i++)
 		sscanf((char *) &(output->data)[i * 2], "%hhx", &req[1 + i]);
 
 	fd = g_io_channel_unix_get_fd(dev->ctrl_io);
 
-	if (write(fd, req, req_size) < 0)
+	if (write(fd, req, sizeof(req)) < 0)
 		error("error writing set_report: %s (%d)",
 						strerror(errno), errno);
-
-	g_free(req);
 }
 
 static gboolean uhid_event_cb(GIOChannel *io, GIOCondition cond,
-- 
1.8.3.2

Re: [PATCH 2/2] android/hidhost: Simplify handle_uhid_output

[Luiz Augusto von Dentz]

Hi Szymon,

On Mon, Dec 16, 2013 at 4:37 PM, Szymon Janc <szymon.janc@tieto.com> wrote:
> Make it use VLA for req buffer. This makes function simpler and also
> fix cutting req to 255 bytes (req_len was uint8_t)
> ---
>  android/hidhost.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/android/hidhost.c b/android/hidhost.c
> index 0e0391a..76322af 100644
> --- a/android/hidhost.c
> +++ b/android/hidhost.c
> @@ -156,29 +156,22 @@ static void hid_device_free(struct hid_device *dev)
>  static void handle_uhid_output(struct hid_device *dev,
>                                                 struct uhid_output_req *output)
>  {
> -       int fd, i;
> -       uint8_t *req = NULL;
> -       uint8_t req_size = 0;
> +       int fd;
> +       unsigned int i;
> +       uint8_t req[1 + (output->size / 2)];

Im not a fan of VLA and Ive seem even some static analyzer that would
warn about its use without first checking > 0 and have a check of
upper bond limit, so perhaps just having it set to UHID_DATA_MAX or
dynamically allocate a buffer to match the output MTU size of the
control channel would better imo.



-- 
Luiz Augusto von Dentz

Re: [PATCH 2/2] android/hidhost: Simplify handle_uhid_output

[Johan Hedberg]

Hi Luiz & Szymon,

On Mon, Dec 16, 2013, Luiz Augusto von Dentz wrote:
> On Mon, Dec 16, 2013 at 4:37 PM, Szymon Janc <szymon.janc@tieto.com> wrote:
> > Make it use VLA for req buffer. This makes function simpler and also
> > fix cutting req to 255 bytes (req_len was uint8_t)
> > ---
> >  android/hidhost.c | 17 +++++------------
> >  1 file changed, 5 insertions(+), 12 deletions(-)
> >
> > diff --git a/android/hidhost.c b/android/hidhost.c
> > index 0e0391a..76322af 100644
> > --- a/android/hidhost.c
> > +++ b/android/hidhost.c
> > @@ -156,29 +156,22 @@ static void hid_device_free(struct hid_device *dev)
> >  static void handle_uhid_output(struct hid_device *dev,
> >                                                 struct uhid_output_req *output)
> >  {
> > -       int fd, i;
> > -       uint8_t *req = NULL;
> > -       uint8_t req_size = 0;
> > +       int fd;
> > +       unsigned int i;
> > +       uint8_t req[1 + (output->size / 2)];
> 
> Im not a fan of VLA and Ive seem even some static analyzer that would
> warn about its use without first checking > 0 and have a check of
> upper bond limit, so perhaps just having it set to UHID_DATA_MAX or
> dynamically allocate a buffer to match the output MTU size of the
> control channel would better imo.

I've applied the first patch in this set, but was there some conclusion
on how to proceed here?

If output->size was an uint8_t this would be a non-issue, but as it's
uint16_t I'm a bit uncomfortable with the idea of potentially having a
huge variable on the stack.

Johan