[PATCH 3v] char: hw_random: core.c: Changed from using strncat to strlcat

[PATCH 3v] char: hw_random: core.c: Changed from using strncat to strlcat

[Rickard Strandqvist]

The buf is used to hold the list of hwrng devices registered.
The old code ensures we don't walk off the end of buf as we
fill it, but it's unnecessarily complicated and thus difficult
to maintain. Simplify it by using strlcat.
We also ensure the string within buf is NULL terminated
so the final strlen is ok.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/char/hw_random/core.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index aa30a25..1500cfd 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -281,7 +281,6 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
 					 char *buf)
 {
 	int err;
-	ssize_t ret = 0;
 	struct hwrng *rng;
 
 	err = mutex_lock_interruptible(&rng_mutex);
@@ -289,16 +288,13 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
 		return -ERESTARTSYS;
 	buf[0] = '\0';
 	list_for_each_entry(rng, &rng_list, list) {
-		strncat(buf, rng->name, PAGE_SIZE - ret - 1);
-		ret += strlen(rng->name);
-		strncat(buf, " ", PAGE_SIZE - ret - 1);
-		ret++;
+		strlcat(buf, rng->name, PAGE_SIZE);
+		strlcat(buf, " ", PAGE_SIZE);
 	}
-	strncat(buf, "\n", PAGE_SIZE - ret - 1);
-	ret++;
+	strlcat(buf, "\n", PAGE_SIZE);
 	mutex_unlock(&rng_mutex);
 
-	return ret;
+	return strlen(buf);
 }
 
 static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
-- 
1.7.10.4

Re: [PATCH 3v] char: hw_random: core.c: Changed from using strncat to strlcat

[Joe Perches]

On Thu, 2014-10-16 at 19:15 +0200, Rickard Strandqvist wrote:
> The buf is used to hold the list of hwrng devices registered.
> The old code ensures we don't walk off the end of buf as we
> fill it, but it's unnecessarily complicated and thus difficult
> to maintain. Simplify it by using strlcat.
> We also ensure the string within buf is NULL terminated
> so the final strlen is ok.
[]
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
[]
> @@ -289,16 +288,13 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
>  		return -ERESTARTSYS;
>  	buf[0] = '\0';
>  	list_for_each_entry(rng, &rng_list, list) {
> -		strncat(buf, rng->name, PAGE_SIZE - ret - 1);
> -		ret += strlen(rng->name);
> -		strncat(buf, " ", PAGE_SIZE - ret - 1);
> -		ret++;
> +		strlcat(buf, rng->name, PAGE_SIZE);
> +		strlcat(buf, " ", PAGE_SIZE);
>  	}
> -	strncat(buf, "\n", PAGE_SIZE - ret - 1);
> -	ret++;
> +	strlcat(buf, "\n", PAGE_SIZE);
>  	mutex_unlock(&rng_mutex);
>  
> -	return ret;
> +	return strlen(buf);
>  }
>  
>  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,

Rickard, can you please use some optimizations here
(and elsewhere) so that strlcat doesn't always have
to strlen the first string and the return doesn't
have to do the strlen too?

You could use a temporary for the returned length
of the strlcat so that if it's shorter than
the buffer, the next strlcat can start at the
appropriate known position instead of having
to do the initial strlen again and again.

Re: [PATCH 3v] char: hw_random: core.c: Changed from using strncat to strlcat

[Rickard Strandqvist]

2014-10-16 19:25 GMT+02:00 Joe Perches <joe@perches.com>:
> On Thu, 2014-10-16 at 19:15 +0200, Rickard Strandqvist wrote:
>> The buf is used to hold the list of hwrng devices registered.
>> The old code ensures we don't walk off the end of buf as we
>> fill it, but it's unnecessarily complicated and thus difficult
>> to maintain. Simplify it by using strlcat.
>> We also ensure the string within buf is NULL terminated
>> so the final strlen is ok.
> []
>> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> []
>> @@ -289,16 +288,13 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
>>               return -ERESTARTSYS;
>>       buf[0] = '\0';
>>       list_for_each_entry(rng, &rng_list, list) {
>> -             strncat(buf, rng->name, PAGE_SIZE - ret - 1);
>> -             ret += strlen(rng->name);
>> -             strncat(buf, " ", PAGE_SIZE - ret - 1);
>> -             ret++;
>> +             strlcat(buf, rng->name, PAGE_SIZE);
>> +             strlcat(buf, " ", PAGE_SIZE);
>>       }
>> -     strncat(buf, "\n", PAGE_SIZE - ret - 1);
>> -     ret++;
>> +     strlcat(buf, "\n", PAGE_SIZE);
>>       mutex_unlock(&rng_mutex);
>>
>> -     return ret;
>> +     return strlen(buf);
>>  }
>>
>>  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
>
> Rickard, can you please use some optimizations here
> (and elsewhere) so that strlcat doesn't always have
> to strlen the first string and the return doesn't
> have to do the strlen too?
>
> You could use a temporary for the returned length
> of the strlcat so that if it's shorter than
> the buffer, the next strlcat can start at the
> appropriate known position instead of having
> to do the initial strlen again and again.
>


Hi Joe

Yes that did not take advantage last strlcat was foolish.

But the others, I am very hesitant about. Because strlcat like
snprintf and strlcpy returns the length that would be copied rather
than what is actually copied. Hence such a code to be even more
complex than before.


Kind regards
Rickard Strandqvist

Re: [PATCH 3v] char: hw_random: core.c: Changed from using strncat to strlcat

[Jason Cooper]

On Thu, Oct 16, 2014 at 10:25:03AM -0700, Joe Perches wrote:
> On Thu, 2014-10-16 at 19:15 +0200, Rickard Strandqvist wrote:
> > The buf is used to hold the list of hwrng devices registered.
> > The old code ensures we don't walk off the end of buf as we
> > fill it, but it's unnecessarily complicated and thus difficult
> > to maintain. Simplify it by using strlcat.
> > We also ensure the string within buf is NULL terminated
> > so the final strlen is ok.
> []
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> []
> > @@ -289,16 +288,13 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
> >  		return -ERESTARTSYS;
> >  	buf[0] = '\0';
> >  	list_for_each_entry(rng, &rng_list, list) {
> > -		strncat(buf, rng->name, PAGE_SIZE - ret - 1);
> > -		ret += strlen(rng->name);
> > -		strncat(buf, " ", PAGE_SIZE - ret - 1);
> > -		ret++;
> > +		strlcat(buf, rng->name, PAGE_SIZE);
> > +		strlcat(buf, " ", PAGE_SIZE);
> >  	}
> > -	strncat(buf, "\n", PAGE_SIZE - ret - 1);
> > -	ret++;
> > +	strlcat(buf, "\n", PAGE_SIZE);
> >  	mutex_unlock(&rng_mutex);
> >  
> > -	return ret;
> > +	return strlen(buf);
> >  }
> >  
> >  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
> 
> Rickard, can you please use some optimizations here
> (and elsewhere) so that strlcat doesn't always have
> to strlen the first string and the return doesn't
> have to do the strlen too?

Joe, is further optimization worth the effort?  This function is only
called when the end user reads the sysfs file rng_available...

thx,

Jason.

Re: [PATCH 3v] char: hw_random: core.c: Changed from using strncat to strlcat

[Joe Perches]

On Thu, 2014-10-16 at 19:41 +0200, Rickard Strandqvist wrote:
> 2014-10-16 19:25 GMT+02:00 Joe Perches <joe@perches.com>:
> > On Thu, 2014-10-16 at 19:15 +0200, Rickard Strandqvist wrote:
> >> The buf is used to hold the list of hwrng devices registered.
> >> The old code ensures we don't walk off the end of buf as we
> >> fill it, but it's unnecessarily complicated and thus difficult
> >> to maintain. Simplify it by using strlcat.
> >> We also ensure the string within buf is NULL terminated
> >> so the final strlen is ok.
> > []
> >> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > []
> >> @@ -289,16 +288,13 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
> >>               return -ERESTARTSYS;
> >>       buf[0] = '\0';
> >>       list_for_each_entry(rng, &rng_list, list) {
> >> -             strncat(buf, rng->name, PAGE_SIZE - ret - 1);
> >> -             ret += strlen(rng->name);
> >> -             strncat(buf, " ", PAGE_SIZE - ret - 1);
> >> -             ret++;
> >> +             strlcat(buf, rng->name, PAGE_SIZE);
> >> +             strlcat(buf, " ", PAGE_SIZE);
> >>       }
> >> -     strncat(buf, "\n", PAGE_SIZE - ret - 1);
> >> -     ret++;
> >> +     strlcat(buf, "\n", PAGE_SIZE);
> >>       mutex_unlock(&rng_mutex);
> >>
> >> -     return ret;
> >> +     return strlen(buf);
> >>  }
> >>
> >>  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
> >
> > Rickard, can you please use some optimizations here
> > (and elsewhere) so that strlcat doesn't always have
> > to strlen the first string and the return doesn't
> > have to do the strlen too?
> >
> > You could use a temporary for the returned length
> > of the strlcat so that if it's shorter than
> > the buffer, the next strlcat can start at the
> > appropriate known position instead of having
> > to do the initial strlen again and again.
[]
> But the others, I am very hesitant about. Because strlcat like
> snprintf and strlcpy returns the length that would be copied rather
> than what is actually copied. Hence such a code to be even more
> complex than before.

None of the conversions you've done seem to use the
return value so maybe there should be yet another
function that doesn't return an overrun value.

Re: [PATCH 3v] char: hw_random: core.c: Changed from using strncat to strlcat

[Jason Cooper]

On Thu, Oct 16, 2014 at 07:15:22PM +0200, Rickard Strandqvist wrote:
> The buf is used to hold the list of hwrng devices registered.
> The old code ensures we don't walk off the end of buf as we
> fill it, but it's unnecessarily complicated and thus difficult
> to maintain. Simplify it by using strlcat.
> We also ensure the string within buf is NULL terminated
> so the final strlen is ok.
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>

Reviewed-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.

> ---
>  drivers/char/hw_random/core.c |   12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index aa30a25..1500cfd 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -281,7 +281,6 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
>  					 char *buf)
>  {
>  	int err;
> -	ssize_t ret = 0;
>  	struct hwrng *rng;
>  
>  	err = mutex_lock_interruptible(&rng_mutex);
> @@ -289,16 +288,13 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
>  		return -ERESTARTSYS;
>  	buf[0] = '\0';
>  	list_for_each_entry(rng, &rng_list, list) {
> -		strncat(buf, rng->name, PAGE_SIZE - ret - 1);
> -		ret += strlen(rng->name);
> -		strncat(buf, " ", PAGE_SIZE - ret - 1);
> -		ret++;
> +		strlcat(buf, rng->name, PAGE_SIZE);
> +		strlcat(buf, " ", PAGE_SIZE);
>  	}
> -	strncat(buf, "\n", PAGE_SIZE - ret - 1);
> -	ret++;
> +	strlcat(buf, "\n", PAGE_SIZE);
>  	mutex_unlock(&rng_mutex);
>  
> -	return ret;
> +	return strlen(buf);
>  }
>  
>  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
> -- 
> 1.7.10.4
> 

Re: [PATCH 3v] char: hw_random: core.c: Changed from using strncat to strlcat

[Joe Perches]

On Thu, 2014-10-16 at 13:48 -0400, Jason Cooper wrote:
> On Thu, Oct 16, 2014 at 10:25:03AM -0700, Joe Perches wrote:
> > On Thu, 2014-10-16 at 19:15 +0200, Rickard Strandqvist wrote:
> > > The buf is used to hold the list of hwrng devices registered.
> > > The old code ensures we don't walk off the end of buf as we
> > > fill it, but it's unnecessarily complicated and thus difficult
> > > to maintain. Simplify it by using strlcat.
> > > We also ensure the string within buf is NULL terminated
> > > so the final strlen is ok.
> > []
> > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > []
> > > @@ -289,16 +288,13 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
> > >  		return -ERESTARTSYS;
> > >  	buf[0] = '\0';
> > >  	list_for_each_entry(rng, &rng_list, list) {
> > > -		strncat(buf, rng->name, PAGE_SIZE - ret - 1);
> > > -		ret += strlen(rng->name);
> > > -		strncat(buf, " ", PAGE_SIZE - ret - 1);
> > > -		ret++;
> > > +		strlcat(buf, rng->name, PAGE_SIZE);
> > > +		strlcat(buf, " ", PAGE_SIZE);
> > >  	}
> > > -	strncat(buf, "\n", PAGE_SIZE - ret - 1);
> > > -	ret++;
> > > +	strlcat(buf, "\n", PAGE_SIZE);
> > >  	mutex_unlock(&rng_mutex);
> > >  
> > > -	return ret;
> > > +	return strlen(buf);
> > >  }
> > >  
> > >  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
> > 
> > Rickard, can you please use some optimizations here
> > (and elsewhere) so that strlcat doesn't always have
> > to strlen the first string and the return doesn't
> > have to do the strlen too?
> 
> Joe, is further optimization worth the effort?  This function is only
> called when the end user reads the sysfs file rng_available...

It's unlikely that it matters much here.

I just don't like the repeated and unnecessary strlen.

I do imagine this pattern being repeated though where
it does matter.

Re: [PATCH 3v] char: hw_random: core.c: Changed from using strncat to strlcat

[Jason Cooper]

On Thu, Oct 16, 2014 at 10:55:57AM -0700, Joe Perches wrote:
> On Thu, 2014-10-16 at 13:48 -0400, Jason Cooper wrote:
> > On Thu, Oct 16, 2014 at 10:25:03AM -0700, Joe Perches wrote:
> > > On Thu, 2014-10-16 at 19:15 +0200, Rickard Strandqvist wrote:
> > > > The buf is used to hold the list of hwrng devices registered.
> > > > The old code ensures we don't walk off the end of buf as we
> > > > fill it, but it's unnecessarily complicated and thus difficult
> > > > to maintain. Simplify it by using strlcat.
> > > > We also ensure the string within buf is NULL terminated
> > > > so the final strlen is ok.
> > > []
> > > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > > []
> > > > @@ -289,16 +288,13 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
> > > >  		return -ERESTARTSYS;
> > > >  	buf[0] = '\0';
> > > >  	list_for_each_entry(rng, &rng_list, list) {
> > > > -		strncat(buf, rng->name, PAGE_SIZE - ret - 1);
> > > > -		ret += strlen(rng->name);
> > > > -		strncat(buf, " ", PAGE_SIZE - ret - 1);
> > > > -		ret++;
> > > > +		strlcat(buf, rng->name, PAGE_SIZE);
> > > > +		strlcat(buf, " ", PAGE_SIZE);
> > > >  	}
> > > > -	strncat(buf, "\n", PAGE_SIZE - ret - 1);
> > > > -	ret++;
> > > > +	strlcat(buf, "\n", PAGE_SIZE);
> > > >  	mutex_unlock(&rng_mutex);
> > > >  
> > > > -	return ret;
> > > > +	return strlen(buf);
> > > >  }
> > > >  
> > > >  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
> > > 
> > > Rickard, can you please use some optimizations here
> > > (and elsewhere) so that strlcat doesn't always have
> > > to strlen the first string and the return doesn't
> > > have to do the strlen too?
> > 
> > Joe, is further optimization worth the effort?  This function is only
> > called when the end user reads the sysfs file rng_available...
> 
> It's unlikely that it matters much here.
> 
> I just don't like the repeated and unnecessary strlen.
> 
> I do imagine this pattern being repeated though where
> it does matter.

Agreed.  Back to why I like to see descriptions from the author
demonstrating they grok the code they are changing :)

thx,

Jason.

Re: [PATCH 3v] char: hw_random: core.c: Changed from using strncat to strlcat

[Rickard Strandqvist]

2014-10-16 20:05 GMT+02:00 Jason Cooper <jason@lakedaemon.net>:
> On Thu, Oct 16, 2014 at 10:55:57AM -0700, Joe Perches wrote:
>> On Thu, 2014-10-16 at 13:48 -0400, Jason Cooper wrote:
>> > On Thu, Oct 16, 2014 at 10:25:03AM -0700, Joe Perches wrote:
>> > > On Thu, 2014-10-16 at 19:15 +0200, Rickard Strandqvist wrote:
>> > > > The buf is used to hold the list of hwrng devices registered.
>> > > > The old code ensures we don't walk off the end of buf as we
>> > > > fill it, but it's unnecessarily complicated and thus difficult
>> > > > to maintain. Simplify it by using strlcat.
>> > > > We also ensure the string within buf is NULL terminated
>> > > > so the final strlen is ok.
>> > > []
>> > > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
>> > > []
>> > > > @@ -289,16 +288,13 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
>> > > >                 return -ERESTARTSYS;
>> > > >         buf[0] = '\0';
>> > > >         list_for_each_entry(rng, &rng_list, list) {
>> > > > -               strncat(buf, rng->name, PAGE_SIZE - ret - 1);
>> > > > -               ret += strlen(rng->name);
>> > > > -               strncat(buf, " ", PAGE_SIZE - ret - 1);
>> > > > -               ret++;
>> > > > +               strlcat(buf, rng->name, PAGE_SIZE);
>> > > > +               strlcat(buf, " ", PAGE_SIZE);
>> > > >         }
>> > > > -       strncat(buf, "\n", PAGE_SIZE - ret - 1);
>> > > > -       ret++;
>> > > > +       strlcat(buf, "\n", PAGE_SIZE);
>> > > >         mutex_unlock(&rng_mutex);
>> > > >
>> > > > -       return ret;
>> > > > +       return strlen(buf);
>> > > >  }
>> > > >
>> > > >  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
>> > >
>> > > Rickard, can you please use some optimizations here
>> > > (and elsewhere) so that strlcat doesn't always have
>> > > to strlen the first string and the return doesn't
>> > > have to do the strlen too?
>> >
>> > Joe, is further optimization worth the effort?  This function is only
>> > called when the end user reads the sysfs file rng_available...
>>
>> It's unlikely that it matters much here.
>>
>> I just don't like the repeated and unnecessary strlen.
>>
>> I do imagine this pattern being repeated though where
>> it does matter.
>
> Agreed.  Back to why I like to see descriptions from the author
> demonstrating they grok the code they are changing :)
>


Hi

I will submit a new patch without return strlen.


All these features are more or less stupid really.
You can also read what Linus thinks about this here.

https://plus.google.com/111049168280159033135/posts/1amLbuhWbh5


I keep on trying to get a patch that solves one of these stupid
functions, but the strncpy in that case.


Kind regards
Rickard Strandqvist