* [PATCH v2] char: hw_random: core.c: Changed from using strncat to strlcat @ 2014-10-12 10:49 Rickard Strandqvist 2014-10-12 19:22 ` Jason Cooper 0 siblings, 1 reply; 7+ messages in thread From: Rickard Strandqvist @ 2014-10-12 10:49 UTC (permalink / raw) To: Matt Mackall, Herbert Xu Cc: Rickard Strandqvist, Torsten Duwe, Theodore Ts'o, Jason Cooper, Amit Shah, Stephen Boyd, Paul Gortmaker, Kees Cook, Dan Carpenter, linux-kernel Changed from using strncat to strlcat to simplify the code 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] char: hw_random: core.c: Changed from using strncat to strlcat 2014-10-12 10:49 [PATCH v2] char: hw_random: core.c: Changed from using strncat to strlcat Rickard Strandqvist @ 2014-10-12 19:22 ` Jason Cooper 2014-10-13 21:20 ` Rickard Strandqvist 0 siblings, 1 reply; 7+ messages in thread From: Jason Cooper @ 2014-10-12 19:22 UTC (permalink / raw) To: Rickard Strandqvist Cc: Matt Mackall, Herbert Xu, Torsten Duwe, Theodore Ts'o, Amit Shah, Stephen Boyd, Paul Gortmaker, Kees Cook, Dan Carpenter, linux-kernel 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@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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] char: hw_random: core.c: Changed from using strncat to strlcat 2014-10-12 19:22 ` Jason Cooper @ 2014-10-13 21:20 ` Rickard Strandqvist 2014-10-14 15:36 ` Jason Cooper 0 siblings, 1 reply; 7+ messages in thread From: Rickard Strandqvist @ 2014-10-13 21:20 UTC (permalink / raw) To: Jason Cooper Cc: Matt Mackall, Herbert Xu, Torsten Duwe, Theodore Ts'o, Amit Shah, Stephen Boyd, Paul Gortmaker, Kees Cook, Dan Carpenter, linux-kernel 2014-10-12 21:22 GMT+02:00 Jason Cooper <jason@lakedaemon.net>: > 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@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 Hi Do not know if I understand this right, you want to explain strlcat function better then ..? 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 But while I think this is something you have to learn, rather than typing it in git comment. But sure, it's just what i think :-) Come with a suggestion what you want I add it... Kind regards Rickard Strandqvist ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] char: hw_random: core.c: Changed from using strncat to strlcat 2014-10-13 21:20 ` Rickard Strandqvist @ 2014-10-14 15:36 ` Jason Cooper 2014-10-14 21:11 ` Rickard Strandqvist 0 siblings, 1 reply; 7+ messages in thread From: Jason Cooper @ 2014-10-14 15:36 UTC (permalink / raw) To: Rickard Strandqvist Cc: Matt Mackall, Herbert Xu, Torsten Duwe, Theodore Ts'o, Amit Shah, Stephen Boyd, Paul Gortmaker, Kees Cook, Dan Carpenter, linux-kernel On Mon, Oct 13, 2014 at 11:20:35PM +0200, Rickard Strandqvist wrote: > 2014-10-12 21:22 GMT+02:00 Jason Cooper <jason@lakedaemon.net>: > > 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@spectrumdigital.se> > >> --- [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. thx, Jason. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] char: hw_random: core.c: Changed from using strncat to strlcat 2014-10-14 15:36 ` Jason Cooper @ 2014-10-14 21:11 ` Rickard Strandqvist 2014-10-15 14:57 ` Jason Cooper 0 siblings, 1 reply; 7+ messages in thread From: Rickard Strandqvist @ 2014-10-14 21:11 UTC (permalink / raw) To: Jason Cooper Cc: Matt Mackall, Herbert Xu, Torsten Duwe, Theodore Ts'o, Amit Shah, Stephen Boyd, Paul Gortmaker, Kees Cook, Dan Carpenter, linux-kernel 2014-10-14 17:36 GMT+02:00 Jason Cooper <jason@lakedaemon.net>: > On Mon, Oct 13, 2014 at 11:20:35PM +0200, Rickard Strandqvist wrote: >> 2014-10-12 21:22 GMT+02:00 Jason Cooper <jason@lakedaemon.net>: >> > 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@spectrumdigital.se> >> >> --- > > [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? 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." Kind regards Rickard Strandqvist ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] char: hw_random: core.c: Changed from using strncat to strlcat 2014-10-14 21:11 ` Rickard Strandqvist @ 2014-10-15 14:57 ` Jason Cooper 2014-10-16 16:56 ` Rickard Strandqvist 0 siblings, 1 reply; 7+ messages in thread From: Jason Cooper @ 2014-10-15 14:57 UTC (permalink / raw) To: Rickard Strandqvist Cc: Matt Mackall, Herbert Xu, Torsten Duwe, Theodore Ts'o, Amit Shah, Stephen Boyd, Paul Gortmaker, Kees Cook, Dan Carpenter, linux-kernel On Tue, Oct 14, 2014 at 11:11:28PM +0200, Rickard Strandqvist wrote: > 2014-10-14 17:36 GMT+02:00 Jason Cooper <jason@lakedaemon.net>: > > On Mon, Oct 13, 2014 at 11:20:35PM +0200, Rickard Strandqvist wrote: > >> 2014-10-12 21:22 GMT+02:00 Jason Cooper <jason@lakedaemon.net>: > >> > 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@spectrumdigital.se> > >> >> --- > > > > [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." thx, Jason. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] char: hw_random: core.c: Changed from using strncat to strlcat 2014-10-15 14:57 ` Jason Cooper @ 2014-10-16 16:56 ` Rickard Strandqvist 0 siblings, 0 replies; 7+ messages in thread From: Rickard Strandqvist @ 2014-10-16 16:56 UTC (permalink / raw) To: Jason Cooper Cc: Matt Mackall, Herbert Xu, Torsten Duwe, Theodore Ts'o, Amit Shah, Stephen Boyd, Paul Gortmaker, Kees Cook, Dan Carpenter, linux-kernel 2014-10-15 16:57 GMT+02:00 Jason Cooper <jason@lakedaemon.net>: > On Tue, Oct 14, 2014 at 11:11:28PM +0200, Rickard Strandqvist wrote: >> 2014-10-14 17:36 GMT+02:00 Jason Cooper <jason@lakedaemon.net>: >> > On Mon, Oct 13, 2014 at 11:20:35PM +0200, Rickard Strandqvist wrote: >> >> 2014-10-12 21:22 GMT+02:00 Jason Cooper <jason@lakedaemon.net>: >> >> > 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@spectrumdigital.se> >> >> >> --- >> > >> > [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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-10-16 16:56 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-10-12 10:49 [PATCH v2] char: hw_random: core.c: Changed from using strncat to strlcat Rickard Strandqvist 2014-10-12 19:22 ` Jason Cooper 2014-10-13 21:20 ` Rickard Strandqvist 2014-10-14 15:36 ` Jason Cooper 2014-10-14 21:11 ` Rickard Strandqvist 2014-10-15 14:57 ` Jason Cooper 2014-10-16 16:56 ` Rickard Strandqvist
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®