Return-Path: <linux-bluetooth-owner@vger.kernel.org>
MIME-Version: 1.0
In-Reply-To: <20160224083550.GA28122@vps217108.ovh.net>
References: <20160224083550.GA28122@vps217108.ovh.net>
Date: Wed, 24 Feb 2016 11:16:40 +0200
Message-ID: <CABBYNZK1Sf_HMCWAikkQ_1AHa0tWHkvGoVGcd673GT0-LhrMvA@mail.gmail.com>
Subject: Re: [PATCH] tools/btgatt-client : fix write-value byte parsing
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
To: Chevallier Maxime <maxime.chevallier@openwide.fr>
Cc: "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
	Johan Hedberg <johan.hedberg@gmail.com>
Content-Type: text/plain; charset=UTF-8
Sender: linux-bluetooth-owner@vger.kernel.org
List-ID: <linux-bluetooth.vger.kernel.org>

Hi Maxime,

On Wed, Feb 24, 2016 at 10:35 AM, Chevallier Maxime
<maxime.chevallier@openwide.fr> wrote:
> write-value, write-long-value and write-prepare were parsing
> bytes using strtol with base '0' and restraining wtring size to
> be exactly 2, forbidding to write values over 99. The string length
> is no more checked, we instead check that the parsed value is in the
> correct range.

There seems to be a space in the subject in between ':' please remove that.

> ---
>  tools/btgatt-client.c | 41 +++++++++++++----------------------------
>  1 file changed, 13 insertions(+), 28 deletions(-)
>
> diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
> index 0f6a1bd..3a0ec62 100644
> --- a/tools/btgatt-client.c
> +++ b/tools/btgatt-client.c
> @@ -658,7 +658,7 @@ static void write_cb(bool success, uint8_t att_ecode, void *user_data)
>
>  static void cmd_write_value(struct client *cli, char *cmd_str)
>  {
> -       int opt, i;
> +       int opt, i, val;
>         char *argvbuf[516];
>         char **argv = argvbuf;
>         int argc = 1;
> @@ -726,19 +726,14 @@ static void cmd_write_value(struct client *cli, char *cmd_str)
>                 }
>
>                 for (i = 1; i < argc; i++) {
> -                       if (strlen(argv[i]) != 2) {
> -                               printf("Invalid value byte: %s\n",
> -                                                               argv[i]);
> -                               goto done;
> -                       }
> -
> -                       value[i-1] = strtol(argv[i], &endptr, 0);
> +                       val = strtol(argv[i], &endptr, 0);
>                         if (endptr == argv[i] || *endptr != '\0'
> -                                                       || errno == ERANGE) {
> +                                                       || errno == ERANGE || val < 0 || val > 255) {
>                                 printf("Invalid value byte: %s\n",
>                                                                 argv[i]);
>                                 goto done;
>                         }
> +                       value[i-1] = val;
>                 }
>         }
>
> @@ -793,7 +788,7 @@ static void write_long_cb(bool success, bool reliable_error, uint8_t att_ecode,
>
>  static void cmd_write_long_value(struct client *cli, char *cmd_str)
>  {
> -       int opt, i;
> +       int opt, i, val;
>         char *argvbuf[516];
>         char **argv = argvbuf;
>         int argc = 1;
> @@ -865,21 +860,15 @@ static void cmd_write_long_value(struct client *cli, char *cmd_str)
>                 }
>
>                 for (i = 2; i < argc; i++) {
> -                       if (strlen(argv[i]) != 2) {
> -                               printf("Invalid value byte: %s\n",
> -                                                               argv[i]);
> -                               free(value);
> -                               return;
> -                       }
> -
> -                       value[i-2] = strtol(argv[i], &endptr, 0);
> +                       val = strtol(argv[i], &endptr, 0);
>                         if (endptr == argv[i] || *endptr != '\0'
> -                                                       || errno == ERANGE) {
> +                                                       || errno == ERANGE || val < 0 || val > 255) {
>                                 printf("Invalid value byte: %s\n",
>                                                                 argv[i]);
>                                 free(value);
>                                 return;
>                         }
> +                       value[i-2] = val;
>                 }
>         }
>
> @@ -909,7 +898,7 @@ static struct option write_prepare_options[] = {
>
>  static void cmd_write_prepare(struct client *cli, char *cmd_str)
>  {
> -       int opt, i;
> +       int opt, i, val;
>         char *argvbuf[516];
>         char **argv = argvbuf;
>         int argc = 0;
> @@ -1002,18 +991,14 @@ static void cmd_write_prepare(struct client *cli, char *cmd_str)
>         }
>
>         for (i = 2; i < argc; i++) {
> -               if (strlen(argv[i]) != 2) {
> -                       printf("Invalid value byte: %s\n", argv[i]);
> -                       free(value);
> -                       return;
> -               }
> -
> -               value[i-2] = strtol(argv[i], &endptr, 0);
> -               if (endptr == argv[i] || *endptr != '\0' || errno == ERANGE) {
> +               val = strtol(argv[i], &endptr, 0);
> +               if (endptr == argv[i] || *endptr != '\0' || errno == ERANGE
> +                                                                                                       || val < 0 || val > 255) {
>                         printf("Invalid value byte: %s\n", argv[i]);
>                         free(value);
>                         return;
>                 }
> +               value[i-2] = val;
>         }
>
>  done:
> --
> 2.1.4

Overall the patch looks pretty good but please fix the following:

WARNING:LONG_LINE: line over 80 characters
#28: FILE: tools/btgatt-client.c:731:
+ || errno == ERANGE || val < 0 || val > 255) {

WARNING:LONG_LINE: line over 80 characters
#61: FILE: tools/btgatt-client.c:865:
+ || errno == ERANGE || val < 0 || val > 255) {

WARNING:LONG_LINE: line over 80 characters
#94: FILE: tools/btgatt-client.c:996:
+ || val < 0 || val > 255) {

-- 
Luiz Augusto von Dentz