[PATCH] drivers/tty/hvc: using strlcpy instead of strncpy

[PATCH] drivers/tty/hvc: using strlcpy instead of strncpy

[Chen Gang]

  when strlen pi->location_code is larger than HVCS_CLC_LENGTH + 1,
    original implementation can not let hvcsd->p_location_code NUL terminated.
  so need fix it (also can simplify the code)

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 drivers/tty/hvc/hvcs.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index 1956593..81e939e 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -881,17 +881,12 @@ static struct vio_driver hvcs_vio_driver = {
 /* Only called from hvcs_get_pi please */
 static void hvcs_set_pi(struct hvcs_partner_info *pi, struct hvcs_struct *hvcsd)
 {
-	int clclength;
-
 	hvcsd->p_unit_address = pi->unit_address;
 	hvcsd->p_partition_ID  = pi->partition_ID;
-	clclength = strlen(&pi->location_code[0]);
-	if (clclength > HVCS_CLC_LENGTH)
-		clclength = HVCS_CLC_LENGTH;
 
 	/* copy the null-term char too */
-	strncpy(&hvcsd->p_location_code[0],
-			&pi->location_code[0], clclength + 1);
+	strlcpy(&hvcsd->p_location_code[0],
+			&pi->location_code[0], sizeof(hvcsd->p_location_code));
 }
 
 /*
-- 
1.7.7.6

Re: [PATCH] drivers/tty/hvc: using strlcpy instead of strncpy

[Jiri Slaby]

On 02/26/2013 04:43 AM, Chen Gang wrote:
> 
>   when strlen pi->location_code is larger than HVCS_CLC_LENGTH + 1,
>     original implementation can not let hvcsd->p_location_code NUL terminated.
>   so need fix it (also can simplify the code)

It should never be larger because the +1 is exactly for NUL. But it is a
cleanup, so why not...

> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  drivers/tty/hvc/hvcs.c |    9 ++-------
>  1 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
> index 1956593..81e939e 100644
> --- a/drivers/tty/hvc/hvcs.c
> +++ b/drivers/tty/hvc/hvcs.c
> @@ -881,17 +881,12 @@ static struct vio_driver hvcs_vio_driver = {
>  /* Only called from hvcs_get_pi please */
>  static void hvcs_set_pi(struct hvcs_partner_info *pi, struct hvcs_struct *hvcsd)
>  {
> -	int clclength;
> -
>  	hvcsd->p_unit_address = pi->unit_address;
>  	hvcsd->p_partition_ID  = pi->partition_ID;
> -	clclength = strlen(&pi->location_code[0]);
> -	if (clclength > HVCS_CLC_LENGTH)
> -		clclength = HVCS_CLC_LENGTH;
>  
>  	/* copy the null-term char too */
> -	strncpy(&hvcsd->p_location_code[0],
> -			&pi->location_code[0], clclength + 1);
> +	strlcpy(&hvcsd->p_location_code[0],
> +			&pi->location_code[0], sizeof(hvcsd->p_location_code));
>  }
>  
>  /*
> 


-- 
js
suse labs

Re: [PATCH] drivers/tty/hvc: using strlcpy instead of strncpy

[Chen Gang]

于 2013年02月28日 18:41, Jiri Slaby 写道:
> On 02/26/2013 04:43 AM, Chen Gang wrote:
>> > 
>> >   when strlen pi->location_code is larger than HVCS_CLC_LENGTH + 1,
>> >     original implementation can not let hvcsd->p_location_code NUL terminated.
>> >   so need fix it (also can simplify the code)
> It should never be larger because the +1 is exactly for NUL. But it is a
> cleanup, so why not...
>

  when strlen(&pi->location_code[0]) == HVCS_CLC_LENGTH + 2
    then clclength will be reset to HVCS_CLC_LENGTH.

    when call strncpy, the clclength + 1 == HVCS_CLS_LENGTH + 1
      but the '\0' of src buf is located at HVCS_CLS_LENGTH + 2.
      so no '\0' copied to dest buf.

    then the dest buf will not be ended by '\0'.

  is it correct ?

  :-)

gchen.
 
>> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> > ---
>> >  drivers/tty/hvc/hvcs.c |    9 ++-------
>> >  1 files changed, 2 insertions(+), 7 deletions(-)
>> > 
>> > diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
>> > index 1956593..81e939e 100644
>> > --- a/drivers/tty/hvc/hvcs.c
>> > +++ b/drivers/tty/hvc/hvcs.c
>> > @@ -881,17 +881,12 @@ static struct vio_driver hvcs_vio_driver = {
>> >  /* Only called from hvcs_get_pi please */
>> >  static void hvcs_set_pi(struct hvcs_partner_info *pi, struct hvcs_struct *hvcsd)
>> >  {
>> > -	int clclength;
>> > -
>> >  	hvcsd->p_unit_address = pi->unit_address;
>> >  	hvcsd->p_partition_ID  = pi->partition_ID;
>> > -	clclength = strlen(&pi->location_code[0]);
>> > -	if (clclength > HVCS_CLC_LENGTH)
>> > -		clclength = HVCS_CLC_LENGTH;
>> >  
>> >  	/* copy the null-term char too */
>> > -	strncpy(&hvcsd->p_location_code[0],
>> > -			&pi->location_code[0], clclength + 1);
>> > +	strlcpy(&hvcsd->p_location_code[0],
>> > +			&pi->location_code[0], sizeof(hvcsd->p_location_code));
>> >  }
>> >  
>> >  /*
>> > 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] drivers/tty/hvc: using strlcpy instead of strncpy

[Chen Gang]

于 2013年02月28日 19:13, Chen Gang 写道:
> 于 2013年02月28日 18:41, Jiri Slaby 写道:
>> On 02/26/2013 04:43 AM, Chen Gang wrote:
>>>>
>>>>   when strlen pi->location_code is larger than HVCS_CLC_LENGTH + 1,
>>>>     original implementation can not let hvcsd->p_location_code NUL terminated.
>>>>   so need fix it (also can simplify the code)
>> It should never be larger because the +1 is exactly for NUL. But it is a
>> cleanup, so why not...
>>
> 
>   when strlen(&pi->location_code[0]) == HVCS_CLC_LENGTH + 2
>     then clclength will be reset to HVCS_CLC_LENGTH.
> 
>     when call strncpy, the clclength + 1 == HVCS_CLS_LENGTH + 1
>       but the '\0' of src buf is located at HVCS_CLS_LENGTH + 2.
        but the '\0' of src buf is located at HVCS_CLS_LENGTH + 3. (not + 2)

>       so no '\0' copied to dest buf.
> 
>     then the dest buf will not be ended by '\0'.
> 
>   is it correct ?
> 
>   :-)
> 
> gchen.
>  
>>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>>> ---
>>>>  drivers/tty/hvc/hvcs.c |    9 ++-------
>>>>  1 files changed, 2 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
>>>> index 1956593..81e939e 100644
>>>> --- a/drivers/tty/hvc/hvcs.c
>>>> +++ b/drivers/tty/hvc/hvcs.c
>>>> @@ -881,17 +881,12 @@ static struct vio_driver hvcs_vio_driver = {
>>>>  /* Only called from hvcs_get_pi please */
>>>>  static void hvcs_set_pi(struct hvcs_partner_info *pi, struct hvcs_struct *hvcsd)
>>>>  {
>>>> -	int clclength;
>>>> -
>>>>  	hvcsd->p_unit_address = pi->unit_address;
>>>>  	hvcsd->p_partition_ID  = pi->partition_ID;
>>>> -	clclength = strlen(&pi->location_code[0]);
>>>> -	if (clclength > HVCS_CLC_LENGTH)
>>>> -		clclength = HVCS_CLC_LENGTH;
>>>>  
>>>>  	/* copy the null-term char too */
>>>> -	strncpy(&hvcsd->p_location_code[0],
>>>> -			&pi->location_code[0], clclength + 1);
>>>> +	strlcpy(&hvcsd->p_location_code[0],
>>>> +			&pi->location_code[0], sizeof(hvcsd->p_location_code));
>>>>  }
>>>>  
>>>>  /*
>>>>
> 
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] drivers/tty/hvc: using strlcpy instead of strncpy

[Jiri Slaby]

On 02/28/2013 12:15 PM, Chen Gang wrote:
> 于 2013年02月28日 19:13, Chen Gang 写道:
>> 于 2013年02月28日 18:41, Jiri Slaby 写道:
>>> On 02/26/2013 04:43 AM, Chen Gang wrote:
>>>>>
>>>>>   when strlen pi->location_code is larger than HVCS_CLC_LENGTH + 1,
>>>>>     original implementation can not let hvcsd->p_location_code NUL terminated.
>>>>>   so need fix it (also can simplify the code)
>>> It should never be larger because the +1 is exactly for NUL. But it is a
>>> cleanup, so why not...
>>>
>>
>>   when strlen(&pi->location_code[0]) == HVCS_CLC_LENGTH + 2

It cannot, pi->location_code is defined as char[HVCS_CLC_LENGTH + 1].


-- 
js
suse labs

Re: [PATCH] drivers/tty/hvc: using strlcpy instead of strncpy

[Chen Gang]

于 2013年02月28日 21:47, Jiri Slaby 写道:
>>>   when strlen(&pi->location_code[0]) == HVCS_CLC_LENGTH + 2
> It cannot, pi->location_code is defined as char[HVCS_CLC_LENGTH + 1].
> 

  really, it is, I did not notice it.

  but I still prefer to modify it, but the patch should be changed
  such as:
    subject: beautify code: deleting useless judging code.
    comments: src buf len and dest buf len are the same, strcpy is better.
    contents: using strcpy instead of strncpy, and delete judging code.

  is it ok ?

BTW:
  sorry for my reply is too late, and did not notify it, originally before.
    I have to do some urgent things, during these days.
      my father had a serious heart disease, and is in hospital.
      during these days, most of my time has to be in hospital.
      (God Bless, and thank Jesus Christ, my father is safe, now).
    within my company (Asianux), I also have something to do.

  excuse me:
    within this week, maybe can not get my mail reply, too.




-- 
Chen Gang

Asianux Corporation

Re: [PATCH] drivers/tty/hvc: using strlcpy instead of strncpy

[Jiri Slaby]

On 03/05/2013 02:58 AM, Chen Gang wrote:
> 于 2013年02月28日 21:47, Jiri Slaby 写道:
>>>>   when strlen(&pi->location_code[0]) == HVCS_CLC_LENGTH + 2
>> It cannot, pi->location_code is defined as char[HVCS_CLC_LENGTH + 1].
>>
> 
>   really, it is, I did not notice it.
> 
>   but I still prefer to modify it, but the patch should be changed
>   such as:
>     subject: beautify code: deleting useless judging code.
>     comments: src buf len and dest buf len are the same, strcpy is better.
>     contents: using strcpy instead of strncpy, and delete judging code.
> 
>   is it ok ?

Yeah.

> BTW:
>   sorry for my reply is too late, and did not notify it, originally before.
>     I have to do some urgent things, during these days.
>       my father had a serious heart disease, and is in hospital.

No problem, these drivers are not so critical. Neither these code paths
in them. Take care of your relatives first.

-- 
js
suse labs

Re: [PATCH] drivers/tty/hvc: using strlcpy instead of strncpy

[Chen Gang]

于 2013年03月05日 17:36, Jiri Slaby 写道:
> On 03/05/2013 02:58 AM, Chen Gang wrote:
>> > 于 2013年02月28日 21:47, Jiri Slaby 写道:
>>>>> >>>>   when strlen(&pi->location_code[0]) == HVCS_CLC_LENGTH + 2
>>> >> It cannot, pi->location_code is defined as char[HVCS_CLC_LENGTH + 1].
>>> >>
>> > 
>> >   really, it is, I did not notice it.
>> > 
>> >   but I still prefer to modify it, but the patch should be changed
>> >   such as:
>> >     subject: beautify code: deleting useless judging code.
>> >     comments: src buf len and dest buf len are the same, strcpy is better.
>> >     contents: using strcpy instead of strncpy, and delete judging code.
>> > 
>> >   is it ok ?
> Yeah.
> 

  I will send patch v2.


>> > BTW:
>> >   sorry for my reply is too late, and did not notify it, originally before.
>> >     I have to do some urgent things, during these days.
>> >       my father had a serious heart disease, and is in hospital.
> No problem, these drivers are not so critical. Neither these code paths
> in them. Take care of your relatives first.

  thanks.

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] drivers/tty/hvc: using strlcpy instead of strncpy

[Chen Gang]

  oh, this patch has integrated into next-20130307 tree.
    (commit 9276dfd27897a0b29d8b5814f39a1f82f56b6b6b)
  it seems we need a regression for this commit, then I send patch v2

  is it correct ?

  :-)


于 2013年03月07日 12:10, Chen Gang 写道:
> 于 2013年03月05日 17:36, Jiri Slaby 写道:
>> On 03/05/2013 02:58 AM, Chen Gang wrote:
>>>> 于 2013年02月28日 21:47, Jiri Slaby 写道:
>>>>>>>>>>   when strlen(&pi->location_code[0]) == HVCS_CLC_LENGTH + 2
>>>>>> It cannot, pi->location_code is defined as char[HVCS_CLC_LENGTH + 1].
>>>>>>
>>>>
>>>>   really, it is, I did not notice it.
>>>>
>>>>   but I still prefer to modify it, but the patch should be changed
>>>>   such as:
>>>>     subject: beautify code: deleting useless judging code.
>>>>     comments: src buf len and dest buf len are the same, strcpy is better.
>>>>     contents: using strcpy instead of strncpy, and delete judging code.
>>>>
>>>>   is it ok ?
>> Yeah.
>>
> 
>   I will send patch v2.
> 
> 
>>>> BTW:
>>>>   sorry for my reply is too late, and did not notify it, originally before.
>>>>     I have to do some urgent things, during these days.
>>>>       my father had a serious heart disease, and is in hospital.
>> No problem, these drivers are not so critical. Neither these code paths
>> in them. Take care of your relatives first.
> 
>   thanks.
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] drivers/tty/hvc: using strlcpy instead of strncpy

[Benjamin Herrenschmidt]

On Thu, 2013-03-07 at 12:34 +0800, Chen Gang wrote:
>   oh, this patch has integrated into next-20130307 tree.
>     (commit 9276dfd27897a0b29d8b5814f39a1f82f56b6b6b)
>   it seems we need a regression for this commit, then I send patch v2
> 
>   is it correct ?

Just send a fixup patch on top of the existing upstream.

Ben.

>   :-)
> 
> 
> 于 2013年03月07日 12:10, Chen Gang 写道:
> > 于 2013年03月05日 17:36, Jiri Slaby 写道:
> >> On 03/05/2013 02:58 AM, Chen Gang wrote:
> >>>> 于 2013年02月28日 21:47, Jiri Slaby 写道:
> >>>>>>>>>>   when strlen(&pi->location_code[0]) == HVCS_CLC_LENGTH + 2
> >>>>>> It cannot, pi->location_code is defined as char[HVCS_CLC_LENGTH + 1].
> >>>>>>
> >>>>
> >>>>   really, it is, I did not notice it.
> >>>>
> >>>>   but I still prefer to modify it, but the patch should be changed
> >>>>   such as:
> >>>>     subject: beautify code: deleting useless judging code.
> >>>>     comments: src buf len and dest buf len are the same, strcpy is better.
> >>>>     contents: using strcpy instead of strncpy, and delete judging code.
> >>>>
> >>>>   is it ok ?
> >> Yeah.
> >>
> > 
> >   I will send patch v2.
> > 
> > 
> >>>> BTW:
> >>>>   sorry for my reply is too late, and did not notify it, originally before.
> >>>>     I have to do some urgent things, during these days.
> >>>>       my father had a serious heart disease, and is in hospital.
> >> No problem, these drivers are not so critical. Neither these code paths
> >> in them. Take care of your relatives first.
> > 
> >   thanks.
> > 
> 
> 

Re: [PATCH] drivers/tty/hvc: using strlcpy instead of strncpy

[Chen Gang]

于 2013年03月07日 14:05, Benjamin Herrenschmidt 写道:
> On Thu, 2013-03-07 at 12:34 +0800, Chen Gang wrote:
>> >   oh, this patch has integrated into next-20130307 tree.
>> >     (commit 9276dfd27897a0b29d8b5814f39a1f82f56b6b6b)
>> >   it seems we need a regression for this commit, then I send patch v2
>> > 
>> >   is it correct ?
> Just send a fixup patch on top of the existing upstream.

  ok, thanks.

  :-)

-- 
Chen Gang

Asianux Corporation

[PATCH] drivers/tty/hvc: fixup original commit: 9276dfd27897a0b29d8b5814f39a1f82f56b6b6b

[Chen Gang]

  originally I did not notice src buf len and dest buf len are the same.
    so origianlly, it is not a bug issue, it is only for beautify code.
    and now, using strcpy is better.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/hvc/hvcs.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index 81e939e..9330a4b 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -885,8 +885,7 @@ static void hvcs_set_pi(struct hvcs_partner_info
*pi, struct hvcs_struct *hvcsd)
 	hvcsd->p_partition_ID  = pi->partition_ID;

 	/* copy the null-term char too */
-	strlcpy(&hvcsd->p_location_code[0],
-			&pi->location_code[0], sizeof(hvcsd->p_location_code));
+	strcpy(&hvcsd->p_location_code[0], &pi->location_code[0]);
 }

 /*
-- 
1.7.7.6

Re: [PATCH] drivers/tty/hvc: fixup original commit: 9276dfd27897a0b29d8b5814f39a1f82f56b6b6b

[Benjamin Herrenschmidt]

On Fri, 2013-03-08 at 11:38 +0800, Chen Gang wrote:
>   originally I did not notice src buf len and dest buf len are the same.
>     so origianlly, it is not a bug issue, it is only for beautify code.
>     and now, using strcpy is better.

Being the same len doesn't mean it's safe to use strcpy ... the source
might be missing the 0 terminator. In this specific case though, I
believe the source string comes was itself populated with strlcpy
(at least since your patch 6b6680c4ea3952af8ae76915cbca41245147741b) so
strcpy is indeed safe but using strlcpy doesn't hurt does it ?

Ben.

> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
>  drivers/tty/hvc/hvcs.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
> index 81e939e..9330a4b 100644
> --- a/drivers/tty/hvc/hvcs.c
> +++ b/drivers/tty/hvc/hvcs.c
> @@ -885,8 +885,7 @@ static void hvcs_set_pi(struct hvcs_partner_info
> *pi, struct hvcs_struct *hvcsd)
>  	hvcsd->p_partition_ID  = pi->partition_ID;
> 
>  	/* copy the null-term char too */
> -	strlcpy(&hvcsd->p_location_code[0],
> -			&pi->location_code[0], sizeof(hvcsd->p_location_code));
> +	strcpy(&hvcsd->p_location_code[0], &pi->location_code[0]);
>  }
> 
>  /*

Re: [PATCH] drivers/tty/hvc: fixup original commit: 9276dfd27897a0b29d8b5814f39a1f82f56b6b6b

[Chen Gang]

于 2013年03月08日 11:46, Benjamin Herrenschmidt 写道:
> strcpy is indeed safe but using strlcpy doesn't hurt does it ?

  really it is: using strlcpy doesn't hurt.

  the comments and subject of original commit are not quite precision:
    it is not for a bug issue (originally I say it is for bug issue)
    it is really for beautify code.

  can I send a fixup patch only for the comments ?

  thanks.

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] drivers/tty/hvc: fixup original commit: 9276dfd27897a0b29d8b5814f39a1f82f56b6b6b

[Benjamin Herrenschmidt]

On Fri, 2013-03-08 at 12:23 +0800, Chen Gang wrote:
>   really it is: using strlcpy doesn't hurt.
> 
>   the comments and subject of original commit are not quite precision:
>     it is not for a bug issue (originally I say it is for bug issue)
>     it is really for beautify code.
> 
>   can I send a fixup patch only for the comments ?

No just drop it, it's fine as it is.

Ben.

Re: [PATCH] drivers/tty/hvc: fixup original commit: 9276dfd27897a0b29d8b5814f39a1f82f56b6b6b

[Chen Gang]

于 2013年03月08日 12:33, Benjamin Herrenschmidt 写道:
> On Fri, 2013-03-08 at 12:23 +0800, Chen Gang wrote:
>> >   really it is: using strlcpy doesn't hurt.
>> > 
>> >   the comments and subject of original commit are not quite precision:
>> >     it is not for a bug issue (originally I say it is for bug issue)
>> >     it is really for beautify code.
>> > 
>> >   can I send a fixup patch only for the comments ?
> No just drop it, it's fine as it is.

  I guess your meaning is:
    not need additional fix up patch (just drop it).
    original commit 9276dfd27897a0b29d8b5814f39a1f82f56b6b6b is valid
      (not need drop the original commit)

  is it correct ?

  thanks.

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] drivers/tty/hvc: fixup original commit: 9276dfd27897a0b29d8b5814f39a1f82f56b6b6b

[Benjamin Herrenschmidt]

On Fri, 2013-03-08 at 12:40 +0800, Chen Gang wrote:
>   is it correct ?

Yes, the code is fine as it is now.

Ben.

Re: [PATCH] drivers/tty/hvc: fixup original commit: 9276dfd27897a0b29d8b5814f39a1f82f56b6b6b

[Chen Gang]

于 2013年03月08日 13:08, Benjamin Herrenschmidt 写道:
> Yes, the code is fine as it is now.

ok, thanks.

-- 
Chen Gang

Asianux Corporation

RE: [PATCH] drivers/tty/hvc: fixup original commit: 9276dfd27897a0b29d8b5814f39a1f82f56b6b6b

[David Laight]

> On Fri, 2013-03-08 at 11:38 +0800, Chen Gang wrote:
> >   originally I did not notice src buf len and dest buf len are the =
same.
> >     so origianlly, it is not a bug issue, it is only for beautify =
code.
> >     and now, using strcpy is better.
>=20
> Being the same len doesn't mean it's safe to use strcpy ... the source
> might be missing the 0 terminator. In this specific case though, I
> believe the source string comes was itself populated with strlcpy
> (at least since your patch 6b6680c4ea3952af8ae76915cbca41245147741b) =
so
> strcpy is indeed safe but using strlcpy doesn't hurt does it ?

Using strlcpy() also stops someone else having to check it
again in a few years time.

	David

Re: [PATCH] drivers/tty/hvc: fixup original commit: 9276dfd27897a0b29d8b5814f39a1f82f56b6b6b

[Chen Gang]

于 2013年03月08日 19:11, David Laight 写道:
> Using strlcpy() also stops someone else having to check it
> again in a few years time.

  yes.

  :-)

-- 
Chen Gang

Asianux Corporation