[PATCH V2] auxdisplay: use correct string length

[PATCH V2] auxdisplay: use correct string length

[Xiongfeng Wang]

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);
 	list_add(&key->list, &logical_inputs);
 	return key;
 }
-- 
1.8.3.1

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

[Miguel Ojeda]

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
>

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

[Willy Tarreau]

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

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

[Miguel Ojeda]

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

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

[Xiongfeng Wang]

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
>>
> 
> .
> 

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

[Willy Tarreau]

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

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

[Arnd Bergmann]

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