Patchwork Bluetooth: hidp: using strlcpy or strcpy instead of strncpy

login
register
mail settings
Submitter Chen Gang
Date May 7, 2013, 1:50 p.m.
Message ID <518906A8.7060708@asianux.com>
Download mbox | patch
Permalink /patch/242323/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Chen Gang - May 7, 2013, 1:50 p.m.
For NUL terminated string, need always let it ended by zero.

Since have already called memcpy() to initialize 'ci', so need not
redundent initializations.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 net/bluetooth/hidp/core.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)
Chen Gang - May 7, 2013, 2:08 p.m.
Hello Maintainers:

In net/bluetooth/hidp/core.c, for hidp_copy_session(), the
'session->input' and 'session->hid' are conflict with each other.

And excuse me, I do not quit know the details, but I think we have 2
choices for fixing it:

  one is ''if (session->input) { } else if (session->hid) { };''
  the other is ''if (seesion->hid) { } else if (session->input) { };''

The first choice assumes the original code has a logical issue; the
second choice assumes the original code has redundant initialization.

Please help check.

Thanks.

  71 static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
  72 {
  73         memset(ci, 0, sizeof(*ci));
  74         bacpy(&ci->bdaddr, &session->bdaddr);
  75 
  76         ci->flags = session->flags;
  77         ci->state = BT_CONNECTED;
  78 
  79         ci->vendor  = 0x0000;
  80         ci->product = 0x0000;
  81         ci->version = 0x0000;
  82 
  83         if (session->input) {
  84                 ci->vendor  = session->input->id.vendor;
  85                 ci->product = session->input->id.product;
  86                 ci->version = session->input->id.version;
  87                 if (session->input->name)
  88                         strncpy(ci->name, session->input->name, 128);
  89                 else
  90                         strncpy(ci->name, "HID Boot Device", 128);
  91         }
  92 
  93         if (session->hid) {
  94                 ci->vendor  = session->hid->vendor;
  95                 ci->product = session->hid->product;
  96                 ci->version = session->hid->version;
  97                 strncpy(ci->name, session->hid->name, 128);
  98         }
  99 }

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Herrmann - May 7, 2013, 7:31 p.m.
Hi

On Tue, May 7, 2013 at 3:50 PM, Chen Gang <gang.chen@asianux.com> wrote:
>
> For NUL terminated string, need always let it ended by zero.
>
> Since have already called memcpy() to initialize 'ci', so need not
> redundent initializations.
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  net/bluetooth/hidp/core.c |   10 +++-------
>  1 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 940f5ac..9a8ae63 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -76,25 +76,21 @@ static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo
>         ci->flags = session->flags;
>         ci->state = BT_CONNECTED;
>
> -       ci->vendor  = 0x0000;
> -       ci->product = 0x0000;
> -       ci->version = 0x0000;
> -
>         if (session->input) {
>                 ci->vendor  = session->input->id.vendor;
>                 ci->product = session->input->id.product;
>                 ci->version = session->input->id.version;
>                 if (session->input->name)
> -                       strncpy(ci->name, session->input->name, 128);
> +                       strlcpy(ci->name, session->input->name, 128);
>                 else
> -                       strncpy(ci->name, "HID Boot Device", 128);
> +                       strcpy(ci->name, "HID Boot Device");

I'd actually prefer strlcpy() here, too (better be safe). Other than
that the patch looks fine.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Regards
David

>         }
>
>         if (session->hid) {
>                 ci->vendor  = session->hid->vendor;
>                 ci->product = session->hid->product;
>                 ci->version = session->hid->version;
> -               strncpy(ci->name, session->hid->name, 128);
> +               strlcpy(ci->name, session->hid->name, 128);
>         }
>  }
>
> --
> 1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Herrmann - May 7, 2013, 7:37 p.m.
Hi

On Tue, May 7, 2013 at 4:08 PM, Chen Gang <gang.chen@asianux.com> wrote:
> Hello Maintainers:
>
> In net/bluetooth/hidp/core.c, for hidp_copy_session(), the
> 'session->input' and 'session->hid' are conflict with each other.
>
> And excuse me, I do not quit know the details, but I think we have 2
> choices for fixing it:
>
>   one is ''if (session->input) { } else if (session->hid) { };''
>   the other is ''if (seesion->hid) { } else if (session->input) { };''
>
> The first choice assumes the original code has a logical issue; the
> second choice assumes the original code has redundant initialization.

The code is fine. Only one of "->input" or "->hid" can be valid at a
time. And exactly one of them is guaranteed to be valid. See
hidp_session_dev_init().

I fixed all code that I changed during the rework to say:

if (session->hid)
...
else if (session->input)
...

It makes the code more clear. But I avoided touching all the other
places that I didn't change, as the code is technically right. Anyway,
I don't care whether we want to fix all other occurrences to use "else
if". Feel free to send a patch.

Thanks
David

> Please help check.
>
> Thanks.
>
>   71 static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
>   72 {
>   73         memset(ci, 0, sizeof(*ci));
>   74         bacpy(&ci->bdaddr, &session->bdaddr);
>   75
>   76         ci->flags = session->flags;
>   77         ci->state = BT_CONNECTED;
>   78
>   79         ci->vendor  = 0x0000;
>   80         ci->product = 0x0000;
>   81         ci->version = 0x0000;
>   82
>   83         if (session->input) {
>   84                 ci->vendor  = session->input->id.vendor;
>   85                 ci->product = session->input->id.product;
>   86                 ci->version = session->input->id.version;
>   87                 if (session->input->name)
>   88                         strncpy(ci->name, session->input->name, 128);
>   89                 else
>   90                         strncpy(ci->name, "HID Boot Device", 128);
>   91         }
>   92
>   93         if (session->hid) {
>   94                 ci->vendor  = session->hid->vendor;
>   95                 ci->product = session->hid->product;
>   96                 ci->version = session->hid->version;
>   97                 strncpy(ci->name, session->hid->name, 128);
>   98         }
>   99 }
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen Gang - May 8, 2013, 1:02 a.m.
On 2013年05月08日 03:31, David Herrmann wrote:
> On Tue, May 7, 2013 at 3:50 PM, Chen Gang <gang.chen@asianux.com> wrote:
>> >
>> > For NUL terminated string, need always let it ended by zero.
>> >
>> > Since have already called memcpy() to initialize 'ci', so need not
>> > redundent initializations.
>> >
>> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> > ---
>> >  net/bluetooth/hidp/core.c |   10 +++-------
>> >  1 files changed, 3 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
>> > index 940f5ac..9a8ae63 100644
>> > --- a/net/bluetooth/hidp/core.c
>> > +++ b/net/bluetooth/hidp/core.c
>> > @@ -76,25 +76,21 @@ static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo
>> >         ci->flags = session->flags;
>> >         ci->state = BT_CONNECTED;
>> >
>> > -       ci->vendor  = 0x0000;
>> > -       ci->product = 0x0000;
>> > -       ci->version = 0x0000;
>> > -
>> >         if (session->input) {
>> >                 ci->vendor  = session->input->id.vendor;
>> >                 ci->product = session->input->id.product;
>> >                 ci->version = session->input->id.version;
>> >                 if (session->input->name)
>> > -                       strncpy(ci->name, session->input->name, 128);
>> > +                       strlcpy(ci->name, session->input->name, 128);
>> >                 else
>> > -                       strncpy(ci->name, "HID Boot Device", 128);
>> > +                       strcpy(ci->name, "HID Boot Device");
> I'd actually prefer strlcpy() here, too (better be safe). Other than
> that the patch looks fine.

OK, thanks. I will send patch v2.
Chen Gang - May 8, 2013, 1:50 a.m.
On 2013年05月08日 03:37, David Herrmann wrote:
> Hi
> 
> On Tue, May 7, 2013 at 4:08 PM, Chen Gang <gang.chen@asianux.com> wrote:
>> Hello Maintainers:
>>
>> In net/bluetooth/hidp/core.c, for hidp_copy_session(), the
>> 'session->input' and 'session->hid' are conflict with each other.
>>
>> And excuse me, I do not quit know the details, but I think we have 2
>> choices for fixing it:
>>
>>   one is ''if (session->input) { } else if (session->hid) { };''
>>   the other is ''if (seesion->hid) { } else if (session->input) { };''
>>
>> The first choice assumes the original code has a logical issue; the
>> second choice assumes the original code has redundant initialization.
> 
> The code is fine. Only one of "->input" or "->hid" can be valid at a
> time. And exactly one of them is guaranteed to be valid. See
> hidp_session_dev_init().
> 

Oh, really it is, thanks.

> I fixed all code that I changed during the rework to say:
> 
> if (session->hid)
> ..
> else if (session->input)
> ..
> 
> It makes the code more clear. But I avoided touching all the other
> places that I didn't change, as the code is technically right. Anyway,
> I don't care whether we want to fix all other occurrences to use "else
> if". Feel free to send a patch.
> 

Me too: "avoided touching all the other places that I didn't change, as
the code is technically right".

Thanks.

Patch

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 940f5ac..9a8ae63 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -76,25 +76,21 @@  static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo
 	ci->flags = session->flags;
 	ci->state = BT_CONNECTED;
 
-	ci->vendor  = 0x0000;
-	ci->product = 0x0000;
-	ci->version = 0x0000;
-
 	if (session->input) {
 		ci->vendor  = session->input->id.vendor;
 		ci->product = session->input->id.product;
 		ci->version = session->input->id.version;
 		if (session->input->name)
-			strncpy(ci->name, session->input->name, 128);
+			strlcpy(ci->name, session->input->name, 128);
 		else
-			strncpy(ci->name, "HID Boot Device", 128);
+			strcpy(ci->name, "HID Boot Device");
 	}
 
 	if (session->hid) {
 		ci->vendor  = session->hid->vendor;
 		ci->product = session->hid->product;
 		ci->version = session->hid->version;
-		strncpy(ci->name, session->hid->name, 128);
+		strlcpy(ci->name, session->hid->name, 128);
 	}
 }