* [PATCH] ext4: use strlcpy() instead of strncpy()
@ 2018-01-09 8:58 Xiongfeng Wang
2018-01-09 10:06 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Xiongfeng Wang @ 2018-01-09 8:58 UTC (permalink / raw)
To: adilger.kernel; +Cc: linux-ext4, wangxiongfeng2
From: Xiongfeng Wang <xiongfeng.wang@linaro.org>
gcc-8 reports
fs/ext4/super.c: In function '__save_error_info.isra.6':
./include/linux/string.h:245:9: warning: '__builtin_strncpy' specified
bound 32 equals destination size [-Wstringop-truncation]
We need to use strlcpy() to make sure the dest string is nul-terminated.
Signed-off-by: Xiongfeng Wang <xiongfeng.wang@linaro.org>
---
fs/ext4/super.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 7c46693..051b988 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -323,11 +323,11 @@ static void __save_error_info(struct super_block *sb, const char *func,
return;
es->s_state |= cpu_to_le16(EXT4_ERROR_FS);
es->s_last_error_time = cpu_to_le32(get_seconds());
- strncpy(es->s_last_error_func, func, sizeof(es->s_last_error_func));
+ strlcpy(es->s_last_error_func, func, sizeof(es->s_last_error_func));
es->s_last_error_line = cpu_to_le32(line);
if (!es->s_first_error_time) {
es->s_first_error_time = es->s_last_error_time;
- strncpy(es->s_first_error_func, func,
+ strlcpy(es->s_first_error_func, func,
sizeof(es->s_first_error_func));
es->s_first_error_line = cpu_to_le32(line);
es->s_first_error_ino = es->s_last_error_ino;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: use strlcpy() instead of strncpy()
2018-01-09 8:58 [PATCH] ext4: use strlcpy() instead of strncpy() Xiongfeng Wang
@ 2018-01-09 10:06 ` Jan Kara
2018-01-09 11:14 ` Xiongfeng Wang
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2018-01-09 10:06 UTC (permalink / raw)
To: Xiongfeng Wang; +Cc: adilger.kernel, linux-ext4
On Tue 09-01-18 16:58:39, Xiongfeng Wang wrote:
> From: Xiongfeng Wang <xiongfeng.wang@linaro.org>
>
> gcc-8 reports
>
> fs/ext4/super.c: In function '__save_error_info.isra.6':
> ./include/linux/string.h:245:9: warning: '__builtin_strncpy' specified
> bound 32 equals destination size [-Wstringop-truncation]
>
> We need to use strlcpy() to make sure the dest string is nul-terminated.
>
> Signed-off-by: Xiongfeng Wang <xiongfeng.wang@linaro.org>
Please have a look how s_last_error_func is used. If you do that, you'll
notice that we do take care to print only sizeof(s_last_error_func)
characters from the string or NUL-terminate it (in e2fsprogs). So your
patch just wastes one character for useless NUL-termination...
Honza
> ---
> fs/ext4/super.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 7c46693..051b988 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -323,11 +323,11 @@ static void __save_error_info(struct super_block *sb, const char *func,
> return;
> es->s_state |= cpu_to_le16(EXT4_ERROR_FS);
> es->s_last_error_time = cpu_to_le32(get_seconds());
> - strncpy(es->s_last_error_func, func, sizeof(es->s_last_error_func));
> + strlcpy(es->s_last_error_func, func, sizeof(es->s_last_error_func));
> es->s_last_error_line = cpu_to_le32(line);
> if (!es->s_first_error_time) {
> es->s_first_error_time = es->s_last_error_time;
> - strncpy(es->s_first_error_func, func,
> + strlcpy(es->s_first_error_func, func,
> sizeof(es->s_first_error_func));
> es->s_first_error_line = cpu_to_le32(line);
> es->s_first_error_ino = es->s_last_error_ino;
> --
> 1.8.3.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: use strlcpy() instead of strncpy()
2018-01-09 10:06 ` Jan Kara
@ 2018-01-09 11:14 ` Xiongfeng Wang
2018-01-09 20:20 ` Theodore Ts'o
0 siblings, 1 reply; 6+ messages in thread
From: Xiongfeng Wang @ 2018-01-09 11:14 UTC (permalink / raw)
To: Jan Kara; +Cc: adilger.kernel, linux-ext4, arnd
On 2018/1/9 18:06, Jan Kara wrote:
> On Tue 09-01-18 16:58:39, Xiongfeng Wang wrote:
>> From: Xiongfeng Wang <xiongfeng.wang@linaro.org>
>>
>> gcc-8 reports
>>
>> fs/ext4/super.c: In function '__save_error_info.isra.6':
>> ./include/linux/string.h:245:9: warning: '__builtin_strncpy' specified
>> bound 32 equals destination size [-Wstringop-truncation]
>>
>> We need to use strlcpy() to make sure the dest string is nul-terminated.
>>
>> Signed-off-by: Xiongfeng Wang <xiongfeng.wang@linaro.org>
>
> Please have a look how s_last_error_func is used. If you do that, you'll
> notice that we do take care to print only sizeof(s_last_error_func)
> characters from the string or NUL-terminate it (in e2fsprogs). So your
> patch just wastes one character for useless NUL-termination...
>
> Honza
Sorry, I didn't notice how s_last_error_func is used before.
We do waste one character if we use strlcpy() instead of strncpy().
We can't use memcpy() either. But I can't figure out a better way to avoid
this warning.
Thanks,
Xiongfeng
>
>> ---
>> fs/ext4/super.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 7c46693..051b988 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -323,11 +323,11 @@ static void __save_error_info(struct super_block *sb, const char *func,
>> return;
>> es->s_state |= cpu_to_le16(EXT4_ERROR_FS);
>> es->s_last_error_time = cpu_to_le32(get_seconds());
>> - strncpy(es->s_last_error_func, func, sizeof(es->s_last_error_func));
>> + strlcpy(es->s_last_error_func, func, sizeof(es->s_last_error_func));
>> es->s_last_error_line = cpu_to_le32(line);
>> if (!es->s_first_error_time) {
>> es->s_first_error_time = es->s_last_error_time;
>> - strncpy(es->s_first_error_func, func,
>> + strlcpy(es->s_first_error_func, func,
>> sizeof(es->s_first_error_func));
>> es->s_first_error_line = cpu_to_le32(line);
>> es->s_first_error_ino = es->s_last_error_ino;
>> --
>> 1.8.3.1
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: use strlcpy() instead of strncpy()
2018-01-09 11:14 ` Xiongfeng Wang
@ 2018-01-09 20:20 ` Theodore Ts'o
2018-01-10 21:30 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Theodore Ts'o @ 2018-01-09 20:20 UTC (permalink / raw)
To: Xiongfeng Wang; +Cc: Jan Kara, adilger.kernel, linux-ext4, arnd
On Tue, Jan 09, 2018 at 07:14:36PM +0800, Xiongfeng Wang wrote:
>
> Sorry, I didn't notice how s_last_error_func is used before.
> We do waste one character if we use strlcpy() instead of strncpy().
> We can't use memcpy() either. But I can't figure out a better way to avoid
> this warning.
Compain to the GCC developers? Create scripts that filter out crap?
What's important is code correctness, not eliminating warnings,
especially if the warnings are bogus. Sacrificing code performance or
correctness just for the sake of silencing warnings is just silly....
About all I'm willing to do here is to take a patch which adds a
comment saying, "ignore bogus GCC warning on this line".
- Ted
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: use strlcpy() instead of strncpy()
2018-01-09 20:20 ` Theodore Ts'o
@ 2018-01-10 21:30 ` Arnd Bergmann
2018-01-10 23:01 ` Theodore Ts'o
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2018-01-10 21:30 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Xiongfeng Wang, Jan Kara, Andreas Dilger, linux-ext4
On Tue, Jan 9, 2018 at 9:20 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Tue, Jan 09, 2018 at 07:14:36PM +0800, Xiongfeng Wang wrote:
>>
>> Sorry, I didn't notice how s_last_error_func is used before.
>> We do waste one character if we use strlcpy() instead of strncpy().
>> We can't use memcpy() either. But I can't figure out a better way to avoid
>> this warning.
>
> Compain to the GCC developers? Create scripts that filter out crap?
This warning option appears to be particularly good in finding code that
is actually dangerous in case of an overflow, and generally using
strscpy() improves either correctness and readability over strncpy(),
I think it's worth leaving enabled globally.
I have an experimental patch series to let you wrap the _Pragma("GCC
diagnostic ingnored \"-Wstringop-truncation\"") directive in some
nicer syntax. Would that work for you here?
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: use strlcpy() instead of strncpy()
2018-01-10 21:30 ` Arnd Bergmann
@ 2018-01-10 23:01 ` Theodore Ts'o
0 siblings, 0 replies; 6+ messages in thread
From: Theodore Ts'o @ 2018-01-10 23:01 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Xiongfeng Wang, Jan Kara, Andreas Dilger, linux-ext4
On Wed, Jan 10, 2018 at 10:30:15PM +0100, Arnd Bergmann wrote:
> This warning option appears to be particularly good in finding code that
> is actually dangerous in case of an overflow, and generally using
> strscpy() improves either correctness and readability over strncpy(),
> I think it's worth leaving enabled globally.
>
> I have an experimental patch series to let you wrap the _Pragma("GCC
> diagnostic ingnored \"-Wstringop-truncation\"") directive in some
> nicer syntax. Would that work for you here?
Having a Pragma which silences errors is certainly better than
nothing, but what might be nice is to have a way of tagging a variable
or structure member function as "not guaranteed to be null
terminated". That would suppress warnings where strncpy is used to
set the character array, and add warnings if how the character array
is accessed without taking due care that it might not be
null-terminated.
Essentially all of the on-disk strings in ext4 are not NUL terminated
(or not necessarily NUL terminated). We either use an explicit length
(directory entries) or we use a fixed length character array (for the
things like the volume label, last error function, etc.)
Cheers,
- Ted
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-10 23:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-09 8:58 [PATCH] ext4: use strlcpy() instead of strncpy() Xiongfeng Wang
2018-01-09 10:06 ` Jan Kara
2018-01-09 11:14 ` Xiongfeng Wang
2018-01-09 20:20 ` Theodore Ts'o
2018-01-10 21:30 ` Arnd Bergmann
2018-01-10 23:01 ` Theodore Ts'o
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).