| Message ID | 1516095490-83827-1-git-send-email-wangxiongfeng2@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] auxdisplay: use correct string length | expand |
On Tue, Jan 16, 2018 at 10:38 AM, Xiongfeng Wang <wangxiongfeng2@huawei.com> wrote: > From: Xiongfeng Wang <xiongfeng.wang@linaro.org> > > gcc-8 reports > > drivers/auxdisplay/panel.c: In function 'panel_attach': > ./include/linux/string.h:245:9: warning: '__builtin_strncpy' specified > bound 12 equals destination size [-Wstringop-truncation] > > We need one less byte or call strlcpy() to make it a nul-terminated > string. > > Signed-off-by: Xiongfeng Wang <xiongfeng.wang@linaro.org> > --- > drivers/auxdisplay/panel.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c > index ea7869c..d288900 100644 > --- a/drivers/auxdisplay/panel.c > +++ b/drivers/auxdisplay/panel.c > @@ -1506,10 +1506,10 @@ static struct logical_input *panel_bind_key(const char *name, const char *press, > key->rise_time = 1; > key->fall_time = 1; > > - strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str)); > - strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str)); > + strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str) - 1); > + strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str) - 1); > strncpy(key->u.kbd.release_str, release, > - sizeof(key->u.kbd.release_str)); > + sizeof(key->u.kbd.release_str) - 1); Are you sure about this patch? `kbd` says "strings can be non null-terminated". Willy, maybe those should just be memcpy()s? (unless the remaining bytes, if any, must be 0). Thanks, Miguel > list_add(&key->list, &logical_inputs); > return key; > } > -- > 1.8.3.1 >
On Mon, Feb 12, 2018 at 01:53:57PM +0100, Miguel Ojeda wrote: > > diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c > > index ea7869c..d288900 100644 > > --- a/drivers/auxdisplay/panel.c > > +++ b/drivers/auxdisplay/panel.c > > @@ -1506,10 +1506,10 @@ static struct logical_input *panel_bind_key(const char *name, const char *press, > > key->rise_time = 1; > > key->fall_time = 1; > > > > - strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str)); > > - strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str)); > > + strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str) - 1); > > + strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str) - 1); > > strncpy(key->u.kbd.release_str, release, > > - sizeof(key->u.kbd.release_str)); > > + sizeof(key->u.kbd.release_str) - 1); > > Are you sure about this patch? `kbd` says "strings can be non null-terminated". > > Willy, maybe those should just be memcpy()s? (unless the remaining > bytes, if any, must be 0). For me this seems to be the result of yet another very stupid gcc warning trying to dissuade us from using well defined fonctions... it's unimaginable how gcc warnings have become stupid and irrelevant since its developers stopped using C to write it :-( If you want to work around this wrong warning, probably that increasing the destination storage size by one and adding -1 to strncpy() would shut it up but that really becomes quite annoying to have to modify code and storage just to shut down a dumbass compiler trying to be smart. Willy
On Mon, Feb 12, 2018 at 6:11 PM, Willy Tarreau <w@1wt.eu> wrote: > On Mon, Feb 12, 2018 at 01:53:57PM +0100, Miguel Ojeda wrote: >> > diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c >> > index ea7869c..d288900 100644 >> > --- a/drivers/auxdisplay/panel.c >> > +++ b/drivers/auxdisplay/panel.c >> > @@ -1506,10 +1506,10 @@ static struct logical_input *panel_bind_key(const char *name, const char *press, >> > key->rise_time = 1; >> > key->fall_time = 1; >> > >> > - strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str)); >> > - strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str)); >> > + strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str) - 1); >> > + strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str) - 1); >> > strncpy(key->u.kbd.release_str, release, >> > - sizeof(key->u.kbd.release_str)); >> > + sizeof(key->u.kbd.release_str) - 1); >> >> Are you sure about this patch? `kbd` says "strings can be non null-terminated". >> >> Willy, maybe those should just be memcpy()s? (unless the remaining >> bytes, if any, must be 0). > > For me this seems to be the result of yet another very stupid gcc warning > trying to dissuade us from using well defined fonctions... it's unimaginable > how gcc warnings have become stupid and irrelevant since its developers > stopped using C to write it :-( > > If you want to work around this wrong warning, probably that increasing the > destination storage size by one and adding -1 to strncpy() would shut it up It does indeed. > but that really becomes quite annoying to have to modify code and storage > just to shut down a dumbass compiler trying to be smart. > I have looked a bit deeper at this new warning. It comes with -Wall, and to trigger it the compiler needs to have some optimization passes enabled (-O2 works). See https://godbolt.org/g/dydPah to play with it in action. Assuming gcc 8 comes out with this warning implemented as of today (likely), we have several options then: a) -Wno-stringop-truncation for gcc >= 8. Note: the warning checks for several kinds of potential issues, some might be useful. b) Use the new __attribute__((nonstring)) for gcc >= 8. For instance, the kbd strings are anyhow marked with the /* strings can be non null-terminated */ comment, so we might just as well replace the comment with the attribute. c) For this case: +1 the array length, -1 the sizeof; as Willy suggested; or similar workarounds. d) For this case: try to make gcc see that the source is a small enough array. Unlikely, due to the keypad_profile indirection and the loop in keypad_init. Since anyway we will be hit by this warning in N places in the kernel now or in the future, let's ask Linus et. al. about what should be the policy to follow :) Miguel > Willy
On 2018/2/12 20:53, Miguel Ojeda wrote: > On Tue, Jan 16, 2018 at 10:38 AM, Xiongfeng Wang > <wangxiongfeng2@huawei.com> wrote: >> From: Xiongfeng Wang <xiongfeng.wang@linaro.org> >> >> gcc-8 reports >> >> drivers/auxdisplay/panel.c: In function 'panel_attach': >> ./include/linux/string.h:245:9: warning: '__builtin_strncpy' specified >> bound 12 equals destination size [-Wstringop-truncation] >> >> We need one less byte or call strlcpy() to make it a nul-terminated >> string. >> >> Signed-off-by: Xiongfeng Wang <xiongfeng.wang@linaro.org> >> --- >> drivers/auxdisplay/panel.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c >> index ea7869c..d288900 100644 >> --- a/drivers/auxdisplay/panel.c >> +++ b/drivers/auxdisplay/panel.c >> @@ -1506,10 +1506,10 @@ static struct logical_input *panel_bind_key(const char *name, const char *press, >> key->rise_time = 1; >> key->fall_time = 1; >> >> - strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str)); >> - strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str)); >> + strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str) - 1); >> + strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str) - 1); >> strncpy(key->u.kbd.release_str, release, >> - sizeof(key->u.kbd.release_str)); >> + sizeof(key->u.kbd.release_str) - 1); > > Are you sure about this patch? `kbd` says "strings can be non null-terminated". > > Willy, maybe those should just be memcpy()s? (unless the remaining > bytes, if any, must be 0). Sorry, my apologies. I think I made a mistake. I meant to use strlcpy(), but this also decrease the destination storage size by one. I think, if the strings can be non null-terminated, we can just use memcpy(). This may suppress the gcc warning. Thanks, Xiongfeng > > Thanks, > Miguel > >> list_add(&key->list, &logical_inputs); >> return key; >> } >> -- >> 1.8.3.1 >> > > . >
On Tue, Feb 13, 2018 at 09:19:21AM +0800, Xiongfeng Wang wrote: > >> diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c > >> index ea7869c..d288900 100644 > >> --- a/drivers/auxdisplay/panel.c > >> +++ b/drivers/auxdisplay/panel.c > >> @@ -1506,10 +1506,10 @@ static struct logical_input *panel_bind_key(const char *name, const char *press, > >> key->rise_time = 1; > >> key->fall_time = 1; > >> > >> - strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str)); > >> - strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str)); > >> + strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str) - 1); > >> + strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str) - 1); > >> strncpy(key->u.kbd.release_str, release, > >> - sizeof(key->u.kbd.release_str)); > >> + sizeof(key->u.kbd.release_str) - 1); > > > > Are you sure about this patch? `kbd` says "strings can be non null-terminated". > > > > Willy, maybe those should just be memcpy()s? (unless the remaining > > bytes, if any, must be 0). > Sorry, my apologies. I think I made a mistake. I meant to use strlcpy(), but this > also decrease the destination storage size by one. > I think, if the strings can be non null-terminated, we can just use memcpy(). Well, memcpy() needs as much data in as out. strncpy() does exactly what was apparently needed there : take at most X chars from a given string, and write exactly X on the output, possibly padding with zeroes. We sure can stop using strncpy() and reimplement it, but that becomes ridiculous. One day gcc will tell us that an "if" statement misses an "else" which is probably an error and we'll have to put "else dummy();" everywhere in the code to calm it. > This may suppress the gcc warning. In fact there are two big problems with gcc warnings : - writing -Wno-something-unknown triggers an error on versions where "something-unknown" isn't known, making it difficult to permanently disable warnings (though in the kernel we handle this pretty fine) - warnings and diagnostics are conflated. Some warnings should only be provided when the developer explicitly asks for suspicious stuff (-Wsecurity, -Wsuspicious, etc) that would *not* be part of -Wall since -Wall is usually used to report real warnings. My preferred one today is the one by which gcc reads your *comments* to figure whether you forgot a break in a case statement... Regards, Willy
On Tue, Feb 13, 2018 at 8:23 AM, Willy Tarreau <w@1wt.eu> wrote: > On Tue, Feb 13, 2018 at 09:19:21AM +0800, Xiongfeng Wang wrote: >> This may suppress the gcc warning. > > In fact there are two big problems with gcc warnings : > - writing -Wno-something-unknown triggers an error on versions where > "something-unknown" isn't known, making it difficult to permanently > disable warnings (though in the kernel we handle this pretty fine) We have the $(call cc-disable-warning) helper for this. > - warnings and diagnostics are conflated. Some warnings should only be > provided when the developer explicitly asks for suspicious stuff > (-Wsecurity, -Wsuspicious, etc) that would *not* be part of -Wall > since -Wall is usually used to report real warnings. And this is what the W=1 and W=2 make options are for. I originally asked Xiongfeng to look at some of the new gcc-8 warnings, but must have miscommunicated at some point. I think we do want to enable the new -Wstringop-overflow warnings by default and fix all instances, including the false-positive ones. This is a useful warning because it tells you when you actually write past a buffer, e.g. using strcpy() with a source string that is longer than the destination. For -Wstringop-truncation, we can decide whether it should be W=1 or W=2, but I'd still make it possible for users and maintainers to easily enable the warning when test building new code. Arnd
diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c index ea7869c..d288900 100644 --- a/drivers/auxdisplay/panel.c +++ b/drivers/auxdisplay/panel.c @@ -1506,10 +1506,10 @@ static struct logical_input *panel_bind_key(const char *name, const char *press, key->rise_time = 1; key->fall_time = 1; - strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str)); - strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str)); + strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str) - 1); + strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str) - 1); strncpy(key->u.kbd.release_str, release, - sizeof(key->u.kbd.release_str)); + sizeof(key->u.kbd.release_str) - 1); list_add(&key->list, &logical_inputs); return key; }