Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
On Tue, Mar 13, 2018 at 4:41 PM, Martin Seborwrote: > 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! Please let us know whenever it is in the gcc trunk. > > > No problem. The patch has been committed into the trunk of GCC 8. > Let me know if there's something else. It may be too late to make > any more changes like this before GCC 8.1 is released but further > refinements to suppress false positives can be made in GCC 8.2. > Thanks a lot, I see it in gcc's 2bc9729cac37dee999f4fb48a166cffc198293aa ("PR tree-optimization/84725 - enable attribute nonstring for all narrow character types"). I will compile it and see how it goes. Cheers, Miguel
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
On Tue, Mar 13, 2018 at 4:41 PM, Martin Sebor wrote: > 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! Please let us know whenever it is in the gcc trunk. > > > No problem. The patch has been committed into the trunk of GCC 8. > Let me know if there's something else. It may be too late to make > any more changes like this before GCC 8.1 is released but further > refinements to suppress false positives can be made in GCC 8.2. > Thanks a lot, I see it in gcc's 2bc9729cac37dee999f4fb48a166cffc198293aa ("PR tree-optimization/84725 - enable attribute nonstring for all narrow character types"). I will compile it and see how it goes. Cheers, Miguel
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
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! Please let us know whenever it is in the gcc trunk. No problem. The patch has been committed into the trunk of GCC 8. Let me know if there's something else. It may be too late to make any more changes like this before GCC 8.1 is released but further refinements to suppress false positives can be made in GCC 8.2. Martin
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
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! Please let us know whenever it is in the gcc trunk. No problem. The patch has been committed into the trunk of GCC 8. Let me know if there's something else. It may be too late to make any more changes like this before GCC 8.1 is released but further refinements to suppress false positives can be made in GCC 8.2. Martin
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
On Mon, Mar 5, 2018 at 8:05 PM, Martin Seborwrote: > On 03/02/2018 10:36 AM, Miguel Ojeda wrote: >> >> On Thu, Mar 1, 2018 at 10:57 AM, Arnd Bergmann wrote: >>> >>> On Mon, Feb 19, 2018 at 1:01 AM, Miguel Ojeda >>> wrote: On Mon, Feb 19, 2018 at 12:20 AM, David Rientjes 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 >> Cc: Ingo Molnar >> Cc: Josh Poimboeuf >> Cc: Kees Cook >> Cc: Andrew Morton >> Cc: Geert Uytterhoeven >> Cc: Will Deacon >> Cc: Greg Kroah-Hartman >> Cc: David Rientjes > > > 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 >>> */ >>> - __u8s_first_error_func[32]; /* function where the error >>> happened */ >>> + chars_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 >>> */ >>> - __u8s_last_error_func[32]; /* function where the error >>> happened */ >>> + chars_last_error_func[32] __nonstring; /* function >>> where the error happened */ >>> >>> doesn't feel right. Maybe
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
On Mon, Mar 5, 2018 at 8:05 PM, Martin Sebor wrote: > On 03/02/2018 10:36 AM, Miguel Ojeda wrote: >> >> On Thu, Mar 1, 2018 at 10:57 AM, Arnd Bergmann wrote: >>> >>> On Mon, Feb 19, 2018 at 1:01 AM, Miguel Ojeda >>> wrote: On Mon, Feb 19, 2018 at 12:20 AM, David Rientjes 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 >> Cc: Ingo Molnar >> Cc: Josh Poimboeuf >> Cc: Kees Cook >> Cc: Andrew Morton >> Cc: Geert Uytterhoeven >> Cc: Will Deacon >> Cc: Greg Kroah-Hartman >> Cc: David Rientjes > > > 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 >>> */ >>> - __u8s_first_error_func[32]; /* function where the error >>> happened */ >>> + chars_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 >>> */ >>> - __u8s_last_error_func[32]; /* function where the error >>> happened */ >>> + chars_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
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
On 03/02/2018 10:36 AM, Miguel Ojeda wrote: On Thu, Mar 1, 2018 at 10:57 AM, Arnd Bergmannwrote: On Mon, Feb 19, 2018 at 1:01 AM, Miguel Ojeda wrote: On Mon, Feb 19, 2018 at 12:20 AM, David Rientjes 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 Cc: Ingo Molnar Cc: Josh Poimboeuf Cc: Kees Cook Cc: Andrew Morton Cc: Geert Uytterhoeven Cc: Will Deacon Cc: Greg Kroah-Hartman Cc: David Rientjes 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 */ - __u8s_first_error_func[32]; /* function where the error happened */ + chars_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 */ - __u8s_last_error_func[32]; /* function where the error happened */ + chars_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
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
On 03/02/2018 10:36 AM, Miguel Ojeda wrote: On Thu, Mar 1, 2018 at 10:57 AM, Arnd Bergmann wrote: On Mon, Feb 19, 2018 at 1:01 AM, Miguel Ojeda wrote: On Mon, Feb 19, 2018 at 12:20 AM, David Rientjes 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 Cc: Ingo Molnar Cc: Josh Poimboeuf Cc: Kees Cook Cc: Andrew Morton Cc: Geert Uytterhoeven Cc: Will Deacon Cc: Greg Kroah-Hartman Cc: David Rientjes 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 */ - __u8s_first_error_func[32]; /* function where the error happened */ + chars_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 */ - __u8s_last_error_func[32]; /* function where the error happened */ + chars_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
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
On Thu, Mar 1, 2018 at 10:57 AM, Arnd Bergmannwrote: > On Mon, Feb 19, 2018 at 1:01 AM, Miguel Ojeda > wrote: >> On Mon, Feb 19, 2018 at 12:20 AM, David Rientjes 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 Cc: Ingo Molnar Cc: Josh Poimboeuf Cc: Kees Cook Cc: Andrew Morton Cc: Geert Uytterhoeven Cc: Will Deacon Cc: Greg Kroah-Hartman Cc: David Rientjes >>> >>> 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 */ > - __u8s_first_error_func[32]; /* function where the error happened > */ > + chars_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 */ > - __u8s_last_error_func[32]; /* function where the error happened > */ > + chars_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
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
On Thu, Mar 1, 2018 at 10:57 AM, Arnd Bergmann wrote: > On Mon, Feb 19, 2018 at 1:01 AM, Miguel Ojeda > wrote: >> On Mon, Feb 19, 2018 at 12:20 AM, David Rientjes 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 Cc: Ingo Molnar Cc: Josh Poimboeuf Cc: Kees Cook Cc: Andrew Morton Cc: Geert Uytterhoeven Cc: Will Deacon Cc: Greg Kroah-Hartman Cc: David Rientjes >>> >>> 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 */ > - __u8s_first_error_func[32]; /* function where the error happened > */ > + chars_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 */ > - __u8s_last_error_func[32]; /* function where the error happened > */ > + chars_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
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
On Mon, Feb 19, 2018 at 1:01 AM, Miguel Ojedawrote: > On Mon, Feb 19, 2018 at 12:20 AM, David Rientjes 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 >>> Cc: Ingo Molnar >>> Cc: Josh Poimboeuf >>> Cc: Kees Cook >>> Cc: Andrew Morton >>> Cc: Geert Uytterhoeven >>> Cc: Will Deacon >>> Cc: Greg Kroah-Hartman >>> Cc: David Rientjes >> >> 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 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. 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. 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 */ - __u8s_first_error_func[32]; /* function where the error happened */ + chars_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 */ - __u8s_last_error_func[32]; /* function where the error happened */ + chars_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. > [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. 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. Arnd
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
On Mon, Feb 19, 2018 at 1:01 AM, Miguel Ojeda wrote: > On Mon, Feb 19, 2018 at 12:20 AM, David Rientjes 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 >>> Cc: Ingo Molnar >>> Cc: Josh Poimboeuf >>> Cc: Kees Cook >>> Cc: Andrew Morton >>> Cc: Geert Uytterhoeven >>> Cc: Will Deacon >>> Cc: Greg Kroah-Hartman >>> Cc: David Rientjes >> >> 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 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. 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. 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 */ - __u8s_first_error_func[32]; /* function where the error happened */ + chars_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 */ - __u8s_last_error_func[32]; /* function where the error happened */ + chars_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. > [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. 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. Arnd
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
On Mon, Feb 19, 2018 at 12:20 AM, David Rientjeswrote: > 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 >> Cc: Ingo Molnar >> Cc: Josh Poimboeuf >> Cc: Kees Cook >> Cc: Andrew Morton >> Cc: Geert Uytterhoeven >> Cc: Will Deacon >> Cc: Greg Kroah-Hartman >> Cc: David Rientjes > > 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). [By the way, CC'ing Xiongfeng, Willy and Arnd, since they were involved in the example report; sorry guys!]. Cheers, Miguel
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
On Mon, Feb 19, 2018 at 12:20 AM, David Rientjes 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 >> Cc: Ingo Molnar >> Cc: Josh Poimboeuf >> Cc: Kees Cook >> Cc: Andrew Morton >> Cc: Geert Uytterhoeven >> Cc: Will Deacon >> Cc: Greg Kroah-Hartman >> Cc: David Rientjes > > 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). [By the way, CC'ing Xiongfeng, Willy and Arnd, since they were involved in the example report; sorry guys!]. Cheers, Miguel
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
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> Cc: Ingo Molnar > Cc: Josh Poimboeuf > Cc: Kees Cook > Cc: Andrew Morton > Cc: Geert Uytterhoeven > Cc: Will Deacon > Cc: Greg Kroah-Hartman > Cc: David Rientjes I would have expected to have seen __nonstring used somewhere as part of this patch.
Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)
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 > Cc: Ingo Molnar > Cc: Josh Poimboeuf > Cc: Kees Cook > Cc: Andrew Morton > Cc: Geert Uytterhoeven > Cc: Will Deacon > Cc: Greg Kroah-Hartman > Cc: David Rientjes I would have expected to have seen __nonstring used somewhere as part of this patch.
[PATCH] Support the nonstring variable attribute (gcc >= 8)
>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 OjedaCc: Ingo Molnar Cc: Josh Poimboeuf Cc: Kees Cook Cc: Andrew Morton Cc: Geert Uytterhoeven Cc: Will Deacon Cc: Greg Kroah-Hartman Cc: David Rientjes --- Another option is using -Wno-stringop-truncation, but it remains to be seen how useful the new warning will be. We can try to keep it for the moment until the real bugs and false positives are dealt with and see if it is worth it. At least in the reported case at drivers/auxdisplay, using __nonstring is enough and it can actually replace a comment that was there about the non-stringness of the char arrays that gcc complained about. See https://godbolt.org/g/dydPah to play with the warning in this case. include/linux/compiler-gcc.h | 14 ++ include/linux/compiler_types.h | 4 2 files changed, 18 insertions(+) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index 73bc63e0a1c4..6a9784c0c7f3 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -317,6 +317,20 @@ #define __designated_init __attribute__((designated_init)) #endif +#if GCC_VERSION >= 8 +/* + * 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 + */ +#define __nonstring __attribute__((nonstring)) +#endif + #endif /* gcc version >= 4 specific checks */ #if !defined(__noclone) diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 6b79a9bba9a7..654dd3114052 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -271,4 +271,8 @@ struct ftrace_likely_data { # define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long)) #endif +#ifndef __nonstring +# define __nonstring +#endif + #endif /* __LINUX_COMPILER_TYPES_H */ -- 2.14.1
[PATCH] Support the nonstring variable attribute (gcc >= 8)
>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 Cc: Ingo Molnar Cc: Josh Poimboeuf Cc: Kees Cook Cc: Andrew Morton Cc: Geert Uytterhoeven Cc: Will Deacon Cc: Greg Kroah-Hartman Cc: David Rientjes --- Another option is using -Wno-stringop-truncation, but it remains to be seen how useful the new warning will be. We can try to keep it for the moment until the real bugs and false positives are dealt with and see if it is worth it. At least in the reported case at drivers/auxdisplay, using __nonstring is enough and it can actually replace a comment that was there about the non-stringness of the char arrays that gcc complained about. See https://godbolt.org/g/dydPah to play with the warning in this case. include/linux/compiler-gcc.h | 14 ++ include/linux/compiler_types.h | 4 2 files changed, 18 insertions(+) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index 73bc63e0a1c4..6a9784c0c7f3 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -317,6 +317,20 @@ #define __designated_init __attribute__((designated_init)) #endif +#if GCC_VERSION >= 8 +/* + * 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 + */ +#define __nonstring __attribute__((nonstring)) +#endif + #endif /* gcc version >= 4 specific checks */ #if !defined(__noclone) diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 6b79a9bba9a7..654dd3114052 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -271,4 +271,8 @@ struct ftrace_likely_data { # define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long)) #endif +#ifndef __nonstring +# define __nonstring +#endif + #endif /* __LINUX_COMPILER_TYPES_H */ -- 2.14.1