| Message ID | 20240328140512.4148825-3-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address remaining stringop-truncation warnings | expand |
On 3/28/24 07:04, Arnd Bergmann wrote: > diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c > index ba7237e83863..58726c15ebac 100644 > --- a/drivers/scsi/scsi_devinfo.c > +++ b/drivers/scsi/scsi_devinfo.c > @@ -290,18 +290,28 @@ static struct scsi_dev_info_list_table *scsi_devinfo_lookup_by_key(int key) > static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length, > char *from, int compatible) > { > - size_t from_length; > + int ret; > > - from_length = strlen(from); > - /* This zero-pads the destination */ > - strncpy(to, from, to_length); > - if (from_length < to_length && !compatible) { > - /* > - * space pad the string if it is short. > - */ > - memset(&to[from_length], ' ', to_length - from_length); > + if (compatible) { > + /* This zero-pads and nul-terminates the destination */ > + ret = strscpy_pad(to, from, to_length); > + } else { > + /* no nul-termination but space-padding for short strings */ > + size_t from_length = strlen(from); > + ret = from_length; > + > + if (from_length > to_length) { > + from_length = to_length; > + ret = -E2BIG; > + } > + > + memcpy(to, from, from_length); > + > + if (from_length < to_length) > + memset(&to[from_length], ' ', to_length - from_length); > } > - if (from_length > to_length) > + > + if (ret < 0) > printk(KERN_WARNING "%s: %s string '%s' is too long\n", > __func__, name, from); > } Please eliminate the variable 'ret'. I think that will improve readability of the new code. Thanks, Bart.
Hi, On Thu, Mar 28, 2024 at 03:04:46PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > scsi_strcpy_devinfo() appears to work as intended but its semantics are > so confusing that gcc warns about it when -Wstringop-truncation is enabled: > > In function 'scsi_strcpy_devinfo', > inlined from 'scsi_dev_info_list_add_keyed' at drivers/scsi/scsi_devinfo.c:370:2: > drivers/scsi/scsi_devinfo.c:297:9: error: 'strncpy' specified bound 16 equals destination size [-Werror=stringop-truncation] > 297 | strncpy(to, from, to_length); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Reorganize the function to completely separate the nul-terminated from > the space-padded/non-terminated case. The former is just strscpy_pad(), > while the latter does not have a standard function. > I did the same in a patch sent earlier (few weeks ago): https://lore.kernel.org/all/20240305-strncpy-drivers-scsi-mpi3mr-mpi3mr_fw-c-v3-5-5b78a13ff984@google.com/ Maybe reviewers can chime in on which version is preferred and go from there. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/scsi/scsi_devinfo.c | 30 ++++++++++++++++++++---------- > 1 file changed, 20 insertions(+), 10 deletions(-) > > diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c > index ba7237e83863..58726c15ebac 100644 > --- a/drivers/scsi/scsi_devinfo.c > +++ b/drivers/scsi/scsi_devinfo.c > @@ -290,18 +290,28 @@ static struct scsi_dev_info_list_table *scsi_devinfo_lookup_by_key(int key) > static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length, > char *from, int compatible) > { > - size_t from_length; > + int ret; > > - from_length = strlen(from); > - /* This zero-pads the destination */ > - strncpy(to, from, to_length); > - if (from_length < to_length && !compatible) { > - /* > - * space pad the string if it is short. > - */ > - memset(&to[from_length], ' ', to_length - from_length); > + if (compatible) { > + /* This zero-pads and nul-terminates the destination */ > + ret = strscpy_pad(to, from, to_length); > + } else { > + /* no nul-termination but space-padding for short strings */ > + size_t from_length = strlen(from); > + ret = from_length; > + > + if (from_length > to_length) { > + from_length = to_length; > + ret = -E2BIG; > + } > + > + memcpy(to, from, from_length); > + > + if (from_length < to_length) > + memset(&to[from_length], ' ', to_length - from_length); > } > - if (from_length > to_length) > + > + if (ret < 0) > printk(KERN_WARNING "%s: %s string '%s' is too long\n", > __func__, name, from); > } > -- > 2.39.2 > Thanks Justin
On Fri, Mar 29, 2024, at 00:14, Justin Stitt wrote: > > On Thu, Mar 28, 2024 at 03:04:46PM +0100, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> scsi_strcpy_devinfo() appears to work as intended but its semantics are >> so confusing that gcc warns about it when -Wstringop-truncation is enabled: >> >> In function 'scsi_strcpy_devinfo', >> inlined from 'scsi_dev_info_list_add_keyed' at drivers/scsi/scsi_devinfo.c:370:2: >> drivers/scsi/scsi_devinfo.c:297:9: error: 'strncpy' specified bound 16 equals destination size [-Werror=stringop-truncation] >> 297 | strncpy(to, from, to_length); >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> Reorganize the function to completely separate the nul-terminated from >> the space-padded/non-terminated case. The former is just strscpy_pad(), >> while the latter does not have a standard function. >> > > I did the same in a patch sent earlier (few weeks ago): > > https://lore.kernel.org/all/20240305-strncpy-drivers-scsi-mpi3mr-mpi3mr_fw-c-v3-5-5b78a13ff984@google.com/ > > Maybe reviewers can chime in on which version is preferred and go from > there. I'm in favor of your version, it looks nicer and addresses the comment that Bart had on mine. Arnd
diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c index ba7237e83863..58726c15ebac 100644 --- a/drivers/scsi/scsi_devinfo.c +++ b/drivers/scsi/scsi_devinfo.c @@ -290,18 +290,28 @@ static struct scsi_dev_info_list_table *scsi_devinfo_lookup_by_key(int key) static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length, char *from, int compatible) { - size_t from_length; + int ret; - from_length = strlen(from); - /* This zero-pads the destination */ - strncpy(to, from, to_length); - if (from_length < to_length && !compatible) { - /* - * space pad the string if it is short. - */ - memset(&to[from_length], ' ', to_length - from_length); + if (compatible) { + /* This zero-pads and nul-terminates the destination */ + ret = strscpy_pad(to, from, to_length); + } else { + /* no nul-termination but space-padding for short strings */ + size_t from_length = strlen(from); + ret = from_length; + + if (from_length > to_length) { + from_length = to_length; + ret = -E2BIG; + } + + memcpy(to, from, from_length); + + if (from_length < to_length) + memset(&to[from_length], ' ', to_length - from_length); } - if (from_length > to_length) + + if (ret < 0) printk(KERN_WARNING "%s: %s string '%s' is too long\n", __func__, name, from); }