Messages in this thread | | | From | Miguel Ojeda <> | Date | Mon, 12 Feb 2018 21:42:23 +0100 | Subject | Re: [PATCH V2] auxdisplay: use correct string length |
| |
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
| |