Messages in this thread | | | Subject | Re: [PATCH] Support the nonstring variable attribute (gcc >= 8) | From | Martin Sebor <> | Date | Mon, 5 Mar 2018 12:05:22 -0700 |
| |
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 >
| |