fix the compile warnings of 'strncpy' output truncated before
terminating nul copying N bytes from a string of the same length
Signed-off-by: Luo bin <redacted>
Reported-by: kernel test robot <redacted>
---
V0~V1:
- use the strlen()+1 pattern consistently
drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: David Laight <hidden> Date: 2020-08-07 09:32:13
From: Luo bin
quoted hunk
Sent: 07 August 2020 03:09
fix the compile warnings of 'strncpy' output truncated before
terminating nul copying N bytes from a string of the same length
Signed-off-by: Luo bin <redacted>
Reported-by: kernel test robot <redacted>
---
V0~V1:
- use the strlen()+1 pattern consistently
drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg,level=event->event.chip.err_level;if(level<FAULT_LEVEL_MAX)-strncpy(level_str,fault_level[level],strlen(fault_level[level]));+strncpy(level_str,fault_level[level],strlen(fault_level[level])+1);
Have you even considered what that code is actually doing?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Sent: 07 August 2020 03:09
fix the compile warnings of 'strncpy' output truncated before
terminating nul copying N bytes from a string of the same length
Signed-off-by: Luo bin <redacted>
Reported-by: kernel test robot <redacted>
---
V0~V1:
- use the strlen()+1 pattern consistently
drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg,level=event->event.chip.err_level;if(level<FAULT_LEVEL_MAX)-strncpy(level_str,fault_level[level],strlen(fault_level[level]));+strncpy(level_str,fault_level[level],strlen(fault_level[level])+1);
Have you even considered what that code is actually doing?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
.
I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you explain more to me?
@@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg,level=event->event.chip.err_level;if(level<FAULT_LEVEL_MAX)-strncpy(level_str,fault_level[level],strlen(fault_level[level]));+strncpy(level_str,fault_level[level],strlen(fault_level[level])+1);
Have you even considered what that code is actually doing?
...
I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you explain more to me?
David is trying to express the same thing I was trying to explain to
you, you should use sizeof(level_str) as the third argument because
the code is trying to make sure that the destination buffer is not
overrun.
If you use the strlen() of the source buffer, the strncpy() can still
overflow the destination buffer.
Now do you understand?
@@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg,level=event->event.chip.err_level;if(level<FAULT_LEVEL_MAX)-strncpy(level_str,fault_level[level],strlen(fault_level[level]));+strncpy(level_str,fault_level[level],strlen(fault_level[level])+1);
Have you even considered what that code is actually doing?
...
quoted
I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you explain more to me?
David is trying to express the same thing I was trying to explain to
you, you should use sizeof(level_str) as the third argument because
the code is trying to make sure that the destination buffer is not
overrun.
If you use the strlen() of the source buffer, the strncpy() can still
overflow the destination buffer.
Now do you understand?
Agh, please never use strncpy() on NUL-terminated strings[1]. (You can
see this ultimately gets passed down into devlink_fmsg_string_put()
which expects NUL-terminated strings and does not require trailing
NUL-padding (which if it did, should still never use strncpy(), but
rather strscpy_pad()).
But, as David Laight hints, none of this is needed. The entire buffer
can be avoided (just point into the existing array of strings -- which
should also be const). Add I see that one of the array sizes is wrong.
Both use FAULT_TYPE_MAX, but one should be FAULT_LEVEL_MAX. And since
"Unknown" can just be added to the array, do that and clamp the value
since it's only used for finding the strings in the array.
I would suggest this (totally untested):
From: David Laight <hidden> Date: 2020-08-08 12:50:34
From: luobin (L)
Sent: 08 August 2020 04:37
On 2020/8/7 17:32, David Laight wrote:
quoted
From: Luo bin
quoted
Sent: 07 August 2020 03:09
fix the compile warnings of 'strncpy' output truncated before
terminating nul copying N bytes from a string of the same length
Signed-off-by: Luo bin <redacted>
Reported-by: kernel test robot <redacted>
---
V0~V1:
- use the strlen()+1 pattern consistently
drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg,level=event->event.chip.err_level;if(level<FAULT_LEVEL_MAX)-strncpy(level_str,fault_level[level],strlen(fault_level[level]));+strncpy(level_str,fault_level[level],strlen(fault_level[level])+1);
Have you even considered what that code is actually doing?
David
I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you
explain more to me?
If you can't see it you probably shouldn't be submitting patches....
Consider what happens when the string is long.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
@@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg,level=event->event.chip.err_level;if(level<FAULT_LEVEL_MAX)-strncpy(level_str,fault_level[level],strlen(fault_level[level]));+strncpy(level_str,fault_level[level],strlen(fault_level[level])+1);
Have you even considered what that code is actually doing?
...
quoted
I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you explain more to me?
David is trying to express the same thing I was trying to explain to
you, you should use sizeof(level_str) as the third argument because
the code is trying to make sure that the destination buffer is not
overrun.
If you use the strlen() of the source buffer, the strncpy() can still
overflow the destination buffer.
Now do you understand?
.
Thanks for your explanation. I explained that why I didn't use sizeof(level_str) as the third argument in my previous reply e-mail to you.
Because using sizeof(level_str) as the third argument will still cause the following compile warning:
In function ‘strncpy’,
inlined from ‘chip_fault_show’ at drivers/net/ethernet/huawei/hinic/hinic_devlink.c:345:3:
./include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 17 equals destination size [-Wstringop-truncation]
297 | #define __underlying_strncpy __builtin_strncpy
Now I know that using strncpy() on NUL-terminated strings is deprecated as Kees Cook points out and actually there is no need to use it
in my code.
@@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg,level=event->event.chip.err_level;if(level<FAULT_LEVEL_MAX)-strncpy(level_str,fault_level[level],strlen(fault_level[level]));+strncpy(level_str,fault_level[level],strlen(fault_level[level])+1);
Have you even considered what that code is actually doing?
...
quoted
I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you explain more to me?
David is trying to express the same thing I was trying to explain to
you, you should use sizeof(level_str) as the third argument because
the code is trying to make sure that the destination buffer is not
overrun.
If you use the strlen() of the source buffer, the strncpy() can still
overflow the destination buffer.
Now do you understand?
Agh, please never use strncpy() on NUL-terminated strings[1]. (You can
see this ultimately gets passed down into devlink_fmsg_string_put()
which expects NUL-terminated strings and does not require trailing
NUL-padding (which if it did, should still never use strncpy(), but
rather strscpy_pad()).
But, as David Laight hints, none of this is needed. The entire buffer
can be avoided (just point into the existing array of strings -- which
should also be const). Add I see that one of the array sizes is wrong.
Both use FAULT_TYPE_MAX, but one should be FAULT_LEVEL_MAX. And since
"Unknown" can just be added to the array, do that and clamp the value
since it's only used for finding the strings in the array.
I would suggest this (totally untested):
Thanks for your explanation and review. I haven't realized using strncpy() on NUL-terminated strings is deprecated
and just trying to avoid the compile warnings. The website you provide helps me a lot. Thank you very much!
Sent: 08 August 2020 04:37
On 2020/8/7 17:32, David Laight wrote:
quoted
From: Luo bin
quoted
Sent: 07 August 2020 03:09
fix the compile warnings of 'strncpy' output truncated before
terminating nul copying N bytes from a string of the same length
Signed-off-by: Luo bin <redacted>
Reported-by: kernel test robot <redacted>
---
V0~V1:
- use the strlen()+1 pattern consistently
drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg,level=event->event.chip.err_level;if(level<FAULT_LEVEL_MAX)-strncpy(level_str,fault_level[level],strlen(fault_level[level]));+strncpy(level_str,fault_level[level],strlen(fault_level[level])+1);
Have you even considered what that code is actually doing?
David
I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you
explain more to me?
If you can't see it you probably shouldn't be submitting patches....
Consider what happens when the string is long.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Thanks for your explanation and review. The fault_level[level] is a fixed and NUL-terminated character
string in that code and its length is smaller than the dest buffer size so I think using
strlen(fault_level[level]) + 1 will not overflow the destination buffer. But using strncpy() on
NUL-terminated strings is dangerous indeed and there is totally no need to use it in that code
as Kees points out.
From: David Laight <hidden> Date: 2020-08-10 08:15:55
Thanks for your explanation and review. I haven't realized using strncpy() on NUL-terminated strings
is deprecated
and just trying to avoid the compile warnings. The website you provide helps me a lot. Thank you very
much!
Never try to remove compile-time warnings without understanding
what the code it doing.
The basic problem is that strncpy() almost [1] never does what you want.
It really expects it's input string to be '\0' terminated but
doesn't guarantee the output will be, and also (typically) wastes
cpu cycles zero filling the output buffer.
Someone then defined strscpy() as an alternative, it guarantees
to '\0' the output and doesn't zero fill - which can be an issue.
However strscpy() has it's own problems, the return value is
defined to be the length of the input string - which absolutely
requires it be '\0' terminated. With 'unknown' input this can
page fault!
[1] This fragment looked wrong, but was right!
strncpy(dest, src, sizeof src);
Naive conversion to remove the strncpy() broke it.
In fact 'dest' was 1 byte longer than 'src' and already
zero filled, 'src' might not have been '\0' terminated.
It is about the only time strncpy() is what you want!
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)