EDAC/mc_sysfs: refactor deprecated strncpy

Commit Message

Justin Stitt Sept. 13, 2023, 1:26 a.m. UTC

`strncpy` is deprecated for use on NUL-terminated destination strings [1].

We should prefer more robust and less ambiguous string interfaces.

A suitable replacement is `strscpy_pad` [2] due to the fact that it guarantees
NUL-termination on the destination buffer whilst maintaining the
NUL-padding behavior that `strncpy` provides. This may not be strictly
necessary but as I couldn't understand what this code does I wanted to
ensure that the functionality is the same.

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: [e-mail]
Signed-off-by: Justin Stitt <[e-mail]>
---
Note: build-tested only.
---
 drivers/edac/edac_mc_sysfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230913-strncpy-drivers-edac-edac_mc_sysfs-c-e619b00124a3

Best regards,
--
Justin Stitt <[e-mail]>

Comments

Justin Stitt Sept. 13, 2023, 1:30 a.m. UTC | #1

On Tue, Sep 12, 2023 at 6:26 PM Justin Stitt <[e-mail]> wrote:
>
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> We should prefer more robust and less ambiguous string interfaces.
>
> A suitable replacement is `strscpy_pad` [2] due to the fact that it guarantees
> NUL-termination on the destination buffer whilst maintaining the
> NUL-padding behavior that `strncpy` provides. This may not be strictly
> necessary but as I couldn't understand what this code does I wanted to
> ensure that the functionality is the same.
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: [e-mail]
> Signed-off-by: Justin Stitt <[e-mail]>
> ---
> Note: build-tested only.
> ---
>  drivers/edac/edac_mc_sysfs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 15f63452a9be..b303309a63cf 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device *dev,
>         if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label))
>                 return -EINVAL;
>
> -       strncpy(rank->dimm->label, data, copy_count);
> -       rank->dimm->label[copy_count] = '\0';
> +       strscpy_pad(rank->dimm->label, data, copy_count);
>
>         return count;
>  }
>
> ---
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> change-id: 20230913-strncpy-drivers-edac-edac_mc_sysfs-c-e619b00124a3
>
> Best regards,
> --
> Justin Stitt <[e-mail]>
>

I typo'd my grep and initially missed refactoring another instance of
strncpy in this same file. v2 [1] resolves this.

[1]: https://lore.kernel.org/r/[e-mail]

Tony Luck Sept. 13, 2023, 3:13 p.m. UTC | #2

> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> We should prefer more robust and less ambiguous string interfaces.
>
> A suitable replacement is `strscpy_pad` [2] due to the fact that it guarantees
> NUL-termination on the destination buffer whilst maintaining the
> NUL-padding behavior that `strncpy` provides. This may not be strictly
> necessary but as I couldn't understand what this code does I wanted to
> ensure that the functionality is the same.
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: [e-mail]
> Signed-off-by: Justin Stitt <[e-mail]>
> ---
> Note: build-tested only.
> ---
>  drivers/edac/edac_mc_sysfs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 15f63452a9be..b303309a63cf 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device *dev,
>         if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label))
>                 return -EINVAL;
>
> -       strncpy(rank->dimm->label, data, copy_count);
> -       rank->dimm->label[copy_count] = '\0';
> +       strscpy_pad(rank->dimm->label, data, copy_count);

That doc page says the problem with strncpy() is that it doesn't guarantee to
NUL terminate the target string. But this code is aware of that limitation and
zaps a '\0' at the end to be sure.

So this code doesn't suffer from the potential problems.

If it is going to be fixed, then some further analysis of the original code
would be wise. Just replacing with strscpy_pad() means the code probably
still suffers from the "needless performance penalty" also mentioned in
the deprecation document.

-Tony

Justin Stitt Sept. 13, 2023, 5:17 p.m. UTC | #3

On Wed, Sep 13, 2023 at 8:13 AM Luck, Tony <[e-mail]> wrote:
>
> > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> >
> > We should prefer more robust and less ambiguous string interfaces.
> >
> > A suitable replacement is `strscpy_pad` [2] due to the fact that it guarantees
> > NUL-termination on the destination buffer whilst maintaining the
> > NUL-padding behavior that `strncpy` provides. This may not be strictly
> > necessary but as I couldn't understand what this code does I wanted to
> > ensure that the functionality is the same.
> >
> > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: [e-mail]
> > Signed-off-by: Justin Stitt <[e-mail]>
> > ---
> > Note: build-tested only.
> > ---
> >  drivers/edac/edac_mc_sysfs.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> > index 15f63452a9be..b303309a63cf 100644
> > --- a/drivers/edac/edac_mc_sysfs.c
> > +++ b/drivers/edac/edac_mc_sysfs.c
> > @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device *dev,
> >         if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label))
> >                 return -EINVAL;
> >
> > -       strncpy(rank->dimm->label, data, copy_count);
> > -       rank->dimm->label[copy_count] = '\0';
> > +       strscpy_pad(rank->dimm->label, data, copy_count);
>
> That doc page says the problem with strncpy() is that it doesn't guarantee to
> NUL terminate the target string. But this code is aware of that limitation and
> zaps a '\0' at the end to be sure.
>
> So this code doesn't suffer from the potential problems.

Right, the original code did not have an existing bug due to the
reason you mentioned. However, I'm pretty keen on eliminating uses of
this interface treewide as there is always a more robust and less
ambiguous option.


>
> If it is going to be fixed, then some further analysis of the original code
> would be wise. Just replacing with strscpy_pad() means the code probably
> still suffers from the "needless performance penalty" also mentioned in
> the deprecation document.
Got it, sending a v2 that prefers `strscpy` to `strscpy_pad` resolving
the performance issue.

>
> -Tony
>

Thanks for the timely review!
Justin

Patch

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 15f63452a9be..b303309a63cf 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -229,8 +229,7 @@  static ssize_t channel_dimm_label_store(struct device *dev,
 	if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label))
 		return -EINVAL;
 
-	strncpy(rank->dimm->label, data, copy_count);
-	rank->dimm->label[copy_count] = '\0';
+	strscpy_pad(rank->dimm->label, data, copy_count);
 
 	return count;
 }