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

[Rickard Strandqvist]

2014-10-15 16:57 GMT+02:00 Jason Cooper <jason@xxxxxxxxxxxxxx>:
> On Tue, Oct 14, 2014 at 11:11:28PM +0200, Rickard Strandqvist wrote:
>> 2014-10-14 17:36 GMT+02:00 Jason Cooper <jason@xxxxxxxxxxxxxx>:
>> > On Mon, Oct 13, 2014 at 11:20:35PM +0200, Rickard Strandqvist wrote:
>> >> 2014-10-12 21:22 GMT+02:00 Jason Cooper <jason@xxxxxxxxxxxxxx>:
>> >> > Rickard,
>> >> >
>> >> > On Sun, Oct 12, 2014 at 12:49:31PM +0200, Rickard Strandqvist wrote:
>> >> >> Changed from using strncat to strlcat to simplify the code
>> >> >
>> >> > I'd like to see a little more explicit discussion here.  As Guenter got
>> >> > caught up in the mis-understanding, I doubt he'd be the only one.  I
>> >> > think it's worth spelling out that the old code prevents overflowing the
>> >> > buffer 'buf' of size PAGE_SIZE.  And that strlcat() does that internally
>> >> > allowing this code to be more readable.
>> >> >
>> >> > It should also be mentioned that the final strlen(buf) is safe because
>> >> > every operation on buf will insert a NULL terminator within the
>> >> > buffers limit.
>> >> >
>> >> > thx,
>> >> >
>> >> > Jason.
>> >> >
>> >> >> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@xxxxxxxxxxxxxxxxxx>
>> >> >> ---
>> >
>> > [1]
>> >
>> >> >>  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
>> >>
>> >>
>> >> Hi
>> >>
>> >> Do not know if I understand this right, you want to explain strlcat
>> >> function better then ..?
>> >
>> > I want to see that the submitter of the patch has thought this through
>> > and isn't just blindly doing s/strn/strl/g and some cleanup.
>> >
>> > Please keep in mind that the kernel community is *huge* and no one
>> > person can see everything going on.  The type of fixes and cleanup
>> > you're doing crosses many sub-systems.  As a result, you haven't popped
>> > up on anyones radar as a regular contributor within a sub-system yet.
>> >
>> > iow, I didn't have the thought in my head "Rickard, yeah, he's the guy
>> > doing the cppcheck and strn/l cleanup properly" because none of your
>> > patches have crossed my inbox until now.
>> >
>> >> But while I think this is something you have to learn, rather than
>> >> typing it in git comment.
>> >
>> > Wether it's appropriate for the git comment or not is debatable, I'll
>> > agree.  The point I'm trying to make is that reviewers aren't
>> > super-human.  All I saw at first is a patch from someone I don't know
>> > changing buffer handling in crypto/rng code.  I had no indication in the
>> > email as to how carefully this had been done.  I'll call that out every
>> > time. :)
>> >
>> > A short explanation, here [1], would have let first-time reviewers of
>> > your patches know that you had taken the time to grok the code and
>> > wasn't blindly fulfilling a eudyptula challenge or similar.
>>
>>
>> Hi Jason!
>>
>> Thanks for your response.
>> Yes, I've done patches all over in Kernel. And the response is
>> different everywhere, and this was the first time I got this response
>> :-)
>>
>> But sure! You mean I should put this in git comment, or just that it
>> should have been included as a cover letter?
>
> If it's a series, the cover letter is fine.  For single patches, after
> the '---' works.  For more complicated patches, it's best to put it in
> the commit message.
>
>> How about adding this line.
>> "By using strlcat it is much easier to ensure that you do not write
>> past the maximum string length, which could simplify this code a bit."
>
> That defines strlcat, which as you said previously, is obvious.  I was
> more looking for:
>
> "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."
>


Hi Jason!

Sure, that looks good!
A new submit with that text is coming...

Kind regards
Rickard Strandqvist
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/