Re: [PATCH V2] auxdisplay: use correct string length

[Miguel Ojeda]

On Mon, Feb 12, 2018 at 6:11 PM, Willy Tarreau <w@xxxxxx> 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