| Message ID | 20201031174028.1080476-1-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] drivers: net: sky2: Fix -Wstringop-truncation with W=1 | expand |
On Sat, 31 Oct 2020 18:40:28 +0100 Andrew Lunn wrote: > In function ‘strncpy’, > inlined from ‘sky2_name’ at drivers/net/ethernet/marvell/sky2.c:4903:3, > inlined from ‘sky2_probe’ at drivers/net/ethernet/marvell/sky2.c:5049:2: > ./include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 16 equals destination size [-Wstringop-truncation] > > None of the device names are 16 characters long, so it was never an > issue, but reduce the length of the buffer size by one to avoid the > warning. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/net/ethernet/marvell/sky2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c > index 25981a7a43b5..35b0ec5afe13 100644 > --- a/drivers/net/ethernet/marvell/sky2.c > +++ b/drivers/net/ethernet/marvell/sky2.c > @@ -4900,7 +4900,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) > }; > > if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) > - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); > + strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz - 1); Hm. This irks the eye a little. AFAIK the idiomatic code would be: strncpy(buf, name..., sz - 1); buf[sz - 1] = '\0'; Perhaps it's easier to convert to strscpy()/strscpy_pad()? > else > snprintf(buf, sz, "(chip %#x)", chipid); > return buf;
From: Jakub Kicinski > Sent: 03 November 2020 00:01 > > On Sat, 31 Oct 2020 18:40:28 +0100 Andrew Lunn wrote: > > In function ‘strncpy’, > > inlined from ‘sky2_name’ at drivers/net/ethernet/marvell/sky2.c:4903:3, > > inlined from ‘sky2_probe’ at drivers/net/ethernet/marvell/sky2.c:5049:2: > > ./include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 16 equals destination > size [-Wstringop-truncation] > > > > None of the device names are 16 characters long, so it was never an > > issue, but reduce the length of the buffer size by one to avoid the > > warning. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > --- > > drivers/net/ethernet/marvell/sky2.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c > > index 25981a7a43b5..35b0ec5afe13 100644 > > --- a/drivers/net/ethernet/marvell/sky2.c > > +++ b/drivers/net/ethernet/marvell/sky2.c > > @@ -4900,7 +4900,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) > > }; > > > > if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) > > - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); > > + strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz - 1); > > Hm. This irks the eye a little. AFAIK the idiomatic code would be: > > strncpy(buf, name..., sz - 1); > buf[sz - 1] = '\0'; > > Perhaps it's easier to convert to strscpy()/strscpy_pad()? > > > else > > snprintf(buf, sz, "(chip %#x)", chipid); > > return buf; Is the pad needed? It isn't present in the 'else' branch. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, 3 Nov 2020 10:19:55 +0000 David Laight <David.Laight@ACULAB.COM> wrote: > From: Jakub Kicinski > > Sent: 03 November 2020 00:01 > > > > On Sat, 31 Oct 2020 18:40:28 +0100 Andrew Lunn wrote: > > > In function ‘strncpy’, > > > inlined from ‘sky2_name’ at drivers/net/ethernet/marvell/sky2.c:4903:3, > > > inlined from ‘sky2_probe’ at drivers/net/ethernet/marvell/sky2.c:5049:2: > > > ./include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 16 equals destination > > size [-Wstringop-truncation] > > > > > > None of the device names are 16 characters long, so it was never an > > > issue, but reduce the length of the buffer size by one to avoid the > > > warning. > > > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > > --- > > > drivers/net/ethernet/marvell/sky2.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c > > > index 25981a7a43b5..35b0ec5afe13 100644 > > > --- a/drivers/net/ethernet/marvell/sky2.c > > > +++ b/drivers/net/ethernet/marvell/sky2.c > > > @@ -4900,7 +4900,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) > > > }; > > > > > > if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) > > > - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); > > > + strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz - 1); > > > > Hm. This irks the eye a little. AFAIK the idiomatic code would be: > > > > strncpy(buf, name..., sz - 1); > > buf[sz - 1] = '\0'; > > > > Perhaps it's easier to convert to strscpy()/strscpy_pad()? > > > > > else > > > snprintf(buf, sz, "(chip %#x)", chipid); > > > return buf; > > Is the pad needed? > It isn't present in the 'else' branch. Since this is non-critical code and is only ther to print something useful on boot, why not just use snprintf on both sides of statement? diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c index 25981a7a43b5..96edad30006e 100644 --- a/drivers/net/ethernet/marvell/sky2.c +++ b/drivers/net/ethernet/marvell/sky2.c @@ -4900,7 +4900,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) }; if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); + snprintf(buf, sz, name[chipid - CHIP_ID_YUKON_XL]); else snprintf(buf, sz, "(chip %#x)", chipid); return buf
On Tue, 3 Nov 2020 08:25:01 -0800 Stephen Hemminger <stephen@networkplumber.org> wrote: > On Tue, 3 Nov 2020 10:19:55 +0000 > David Laight <David.Laight@ACULAB.COM> wrote: > > > From: Jakub Kicinski > > > Sent: 03 November 2020 00:01 > > > > > > On Sat, 31 Oct 2020 18:40:28 +0100 Andrew Lunn wrote: > > > > In function ‘strncpy’, > > > > inlined from ‘sky2_name’ at drivers/net/ethernet/marvell/sky2.c:4903:3, > > > > inlined from ‘sky2_probe’ at drivers/net/ethernet/marvell/sky2.c:5049:2: > > > > ./include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 16 equals destination > > > size [-Wstringop-truncation] > > > > > > > > None of the device names are 16 characters long, so it was never an > > > > issue, but reduce the length of the buffer size by one to avoid the > > > > warning. > > > > > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > > > --- > > > > drivers/net/ethernet/marvell/sky2.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c > > > > index 25981a7a43b5..35b0ec5afe13 100644 > > > > --- a/drivers/net/ethernet/marvell/sky2.c > > > > +++ b/drivers/net/ethernet/marvell/sky2.c > > > > @@ -4900,7 +4900,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) > > > > }; > > > > > > > > if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) > > > > - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); > > > > + strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz - 1); > > > > > > Hm. This irks the eye a little. AFAIK the idiomatic code would be: > > > > > > strncpy(buf, name..., sz - 1); > > > buf[sz - 1] = '\0'; > > > > > > Perhaps it's easier to convert to strscpy()/strscpy_pad()? > > > > > > > else > > > > snprintf(buf, sz, "(chip %#x)", chipid); > > > > return buf; > > > > Is the pad needed? > > It isn't present in the 'else' branch. > > Since this is non-critical code and is only ther to print something useful > on boot, why not just use snprintf on both sides of statement? Like this is what I meant... diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c index 25981a7a43b5..ebe1406c6e64 100644 --- a/drivers/net/ethernet/marvell/sky2.c +++ b/drivers/net/ethernet/marvell/sky2.c @@ -4900,7 +4900,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) }; if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); + snprintf(buf, sz, "%s", name[chipid - CHIP_ID_YUKON_XL]); else snprintf(buf, sz, "(chip %#x)", chipid); return buf;
diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c index 25981a7a43b5..35b0ec5afe13 100644 --- a/drivers/net/ethernet/marvell/sky2.c +++ b/drivers/net/ethernet/marvell/sky2.c @@ -4900,7 +4900,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) }; if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); + strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz - 1); else snprintf(buf, sz, "(chip %#x)", chipid); return buf;
In function ‘strncpy’, inlined from ‘sky2_name’ at drivers/net/ethernet/marvell/sky2.c:4903:3, inlined from ‘sky2_probe’ at drivers/net/ethernet/marvell/sky2.c:5049:2: ./include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 16 equals destination size [-Wstringop-truncation] None of the device names are 16 characters long, so it was never an issue, but reduce the length of the buffer size by one to avoid the warning. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/ethernet/marvell/sky2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)