lkml.org 
[lkml]   [2018]   [Mar]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] Support the nonstring variable attribute (gcc >= 8)
From
Date
On 03/02/2018 10:36 AM, Miguel Ojeda wrote:
> On Thu, Mar 1, 2018 at 10:57 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Mon, Feb 19, 2018 at 1:01 AM, Miguel Ojeda
>> <miguel.ojeda.sandonis@gmail.com> wrote:
>>> On Mon, Feb 19, 2018 at 12:20 AM, David Rientjes <rientjes@google.com> wrote:
>>>> On Sat, 17 Feb 2018, Miguel Ojeda wrote:
>>>>
>>>>> From the GCC manual:
>>>>>
>>>>> The nonstring variable attribute specifies that an object or member
>>>>> declaration with type array of char or pointer to char is intended to
>>>>> store character arrays that do not necessarily contain a terminating NUL
>>>>> character. This is useful in detecting uses of such arrays or pointers
>>>>> with functions that expect NUL-terminated strings, and to avoid warnings
>>>>> when such an array or pointer is used as an argument to a bounded string
>>>>> manipulation function such as strncpy.
>>>>>
>>>>> https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html
>>>>>
>>>>> Some reports are already coming to the LKML regarding these
>>>>> warnings. When they are false positives, we can use __nonstring to let
>>>>> gcc know a NUL character is not required; like in this case:
>>>>>
>>>>> https://lkml.org/lkml/2018/1/16/135
>>>>>
>>>>> Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
>>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>>> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
>>>>> Cc: Kees Cook <keescook@chromium.org>
>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>> Cc: David Rientjes <rientjes@google.com>
>>>>
>>>> I would have expected to have seen __nonstring used somewhere as part of
>>>> this patch.
>>>
>>> Do you mean to expand the commit message with an actual code example
>>> instead of the links to the docs and the discussion about the report?
>>> Otherwise, if you mean in the actual commit, I think in that case it
>>> should be a patch series, not a single commit.
>>>
>>> In any case, the key point here is to agree on the short-term policy:
>>> i.e. whether we want to disable the upcoming warning or try to take
>>> advantage of it (which not *necessarily* implies using __nonstring,
>>> there are other workarounds; though where applicable, __nonstring is
>>> probably the right thing to use).
>>
>> What David was asking for is to have a couple of users of the
>> __nonstring attribute in places for which it is the right solution.
>>
>
> I understood :-) My question was regarding where he was asking to see it.
>
>> I would suggest making it a patch series, with patch 1/x introducing
>> the attribute (i.e. your patch), and followed by additional patches
>> that add the attribute to individual header files or drivers for which
>> it is the right solution.
>
> Yep, that is what I suggested too.
>
>>
>> When I looked at the warning, I found that we have around 120 file
>> for which we warn. The majority of them are actually questionable
>> uses of strncpy() that probably should have been strscpy(), but
>> most of those do not actually cause undefined behavior.
>
> Then it looks like enabling the warning by default is useful and not
> too noisy (at least for just char).
>
>>
>> A smaller number like the example from ext4 are nonstrings
>> (i.e. character arrays without nul-termination) that would benefit
>> from the nonstring attribute. About half of those are actually
>> arrays of u8/__u8/uint8_t/__uint8_t for which the currently
>> implemented nonstring attribute is invalid, and it seems odd
>> to convert those to 'char', e.g.
>>
>> struct ext4_super_block {
>> __le32 s_first_error_time; /* first time an error happened */
>> __le32 s_first_error_ino; /* inode involved in first error */
>> __le64 s_first_error_block; /* block involved of first error */
>> - __u8 s_first_error_func[32]; /* function where the error happened */
>> + char s_first_error_func[32] __nonstring; /* function
>> where the error happened */
>> __le32 s_first_error_line; /* line number where error happened */
>> __le32 s_last_error_time; /* most recent time of an error */
>> __le32 s_last_error_ino; /* inode involved in last error */
>> __le32 s_last_error_line; /* line number where error happened */
>> __le64 s_last_error_block; /* block involved of last error */
>> - __u8 s_last_error_func[32]; /* function where the error happened */
>> + char s_last_error_func[32] __nonstring; /* function
>> where the error happened */
>>
>> doesn't feel right. Maybe we can extend gcc to also accept
>> the attribute on arrays of other 8-bit types.
>
> Hum... On one hand, the warning is meant to protect against misuses of
> the typical string handling functions, and those take pointers to
> char. Therefore, one could argue that using signed or unsigned char
> already marks an array/pointer as "not a string" (for the purposes of
> the attribute).
>
> On the other hand, people *will* call string handling functions with
> signed and unsigned char, and for those cases, it is useful to have
> the warning nevertheless and being able to annotate those arrays with
> nonstring, which is also good documentation-wise. On top of that, C
> specifies char as equivalent to either signed or unsigned char (even
> if it is a distinct type), so one could argue it should work for the
> three types anyway.
>
> Given that 1) this is a warning that can disabled just fine and that
> 2) we already have real life cases using nonstring, non-char arrays
> calling typical string handling functions, I would favor supporting
> the warning and the attribute for all the three types.
>
>>
>>> [By the way, CC'ing Xiongfeng, Willy and Arnd, since they were
>>> involved in the example report; sorry guys!].
>>
>> Martin Sebor also asked me about this, he's the one that worked on
>> the gcc code that introduced the warning. Sorry for not replying earlier.
>>
>
> Maybe you can pass this to him? (maybe open a bug in gcc's bugzilla?)

I've opened bug 84725 to extend attribute nonstring to the other
two character types:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84725

Thanks
Martin

>
>> For a complete list of affected files, see https://pastebin.com/eWFQf58i
>> this is what I come up with by doing randconfig builds, but I have not
>> tried to submit additional patches here, since I'm sure that a lot of
>> those are wrong -- they need a much closer inspection to decide which
>> ones are actual bugs vs harmless warnings, and which ones should
>> use strscpy()/strlcpy() vs a nonstring annotation or a rewrite of that
>> function.
>
> Indeed -- nice work anyway finding those. If we agree on getting the
> nonstring attribute, maybe you can send that patch as an RFC to ping
> the respective maintainers?
>
> Thanks,
> Miguel
>

\
 
 \ /
  Last update: 2018-03-05 20:05    [W:0.063 / U:0.728 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site