| Message ID | d1524130f91d7cfd61bc736623409693d2895f57.camel@perches.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().) | expand |
On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote: > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote: > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote: > > > Added warnings in checkpatch.pl script to : > > > > > > 1. Deprecate strcpy() in favor of strscpy(). > > > 2. Deprecate strlcpy() in favor of strscpy(). > > > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad(). > > > > > > Updated strncpy() section in Documentation/process/deprecated.rst > > > to cover strscpy_pad() case. > > [] > > I sent a patch series for some strscpy/strlcpy misuses. > > How about adding a macro helper to avoid the misuses like: > --- > include/linux/string.h | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/include/linux/string.h b/include/linux/string.h > index 4deb11f7976b..ef01bd6f19df 100644 > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t); > /* Wraps calls to strscpy()/memset(), no arch specific code required */ > ssize_t strscpy_pad(char *dest, const char *src, size_t count); > > +#define stracpy(to, from) \ > +({ \ > + size_t size = ARRAY_SIZE(to); \ > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \ > + \ > + strscpy(to, from, size); \ > +}) > + > +#define stracpy_pad(to, from) \ > +({ \ > + size_t size = ARRAY_SIZE(to); \ > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \ > + \ > + strscpy_pad(to, from, size); \ > +}) > + > #ifndef __HAVE_ARCH_STRCAT > extern char * strcat(char *, const char *); > #endif This seems like a reasonable addition, yes. I think Coccinelle might actually be able to find all the existing strscpy(dst, src, sizeof(dst)) cases to jump-start this conversion. Devil's advocate: this adds yet more string handling functions... will this cause even more confusion?
On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote: > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote: > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote: > > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote: > > > > Added warnings in checkpatch.pl script to : > > > > > > > > 1. Deprecate strcpy() in favor of strscpy(). > > > > 2. Deprecate strlcpy() in favor of strscpy(). > > > > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad(). > > > > > > > > Updated strncpy() section in Documentation/process/deprecated.rst > > > > to cover strscpy_pad() case. > > > > [] > > > > I sent a patch series for some strscpy/strlcpy misuses. > > > > How about adding a macro helper to avoid the misuses like: > > --- > > include/linux/string.h | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/include/linux/string.h b/include/linux/string.h > > index 4deb11f7976b..ef01bd6f19df 100644 > > --- a/include/linux/string.h > > +++ b/include/linux/string.h > > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t); > > /* Wraps calls to strscpy()/memset(), no arch specific code required */ > > ssize_t strscpy_pad(char *dest, const char *src, size_t count); > > > > +#define stracpy(to, from) \ > > +({ \ > > + size_t size = ARRAY_SIZE(to); \ > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \ > > + \ > > + strscpy(to, from, size); \ > > +}) > > + > > +#define stracpy_pad(to, from) \ > > +({ \ > > + size_t size = ARRAY_SIZE(to); \ > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \ > > + \ > > + strscpy_pad(to, from, size); \ > > +}) > > + > > #ifndef __HAVE_ARCH_STRCAT > > extern char * strcat(char *, const char *); > > #endif > > This seems like a reasonable addition, yes. I think Coccinelle might > actually be able to find all the existing strscpy(dst, src, sizeof(dst)) > cases to jump-start this conversion. I did that. It works. It's a lot of conversions. $ cat str.cpy.cocci @@ expression e1; expression e2; @@ - strscpy(e1, e2, sizeof(e1)) + stracpy(e1, e2) @@ expression e1; expression e2; @@ - strlcpy(e1, e2, sizeof(e1)) + stracpy(e1, e2) > Devil's advocate: this adds yet more string handling functions... will > this cause even more confusion? Documentation is good. Actual in-kernel use and examples better.
On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote: > On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote: > > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote: > > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote: > > > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote: > > > > > Added warnings in checkpatch.pl script to : > > > > > > > > > > 1. Deprecate strcpy() in favor of strscpy(). > > > > > 2. Deprecate strlcpy() in favor of strscpy(). > > > > > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad(). > > > > > > > > > > Updated strncpy() section in Documentation/process/deprecated.rst > > > > > to cover strscpy_pad() case. > > > > > > [] > > > > > > I sent a patch series for some strscpy/strlcpy misuses. > > > > > > How about adding a macro helper to avoid the misuses like: > > > --- > > > include/linux/string.h | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/include/linux/string.h b/include/linux/string.h > > > index 4deb11f7976b..ef01bd6f19df 100644 > > > --- a/include/linux/string.h > > > +++ b/include/linux/string.h > > > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t); > > > /* Wraps calls to strscpy()/memset(), no arch specific code required */ > > > ssize_t strscpy_pad(char *dest, const char *src, size_t count); > > > > > > +#define stracpy(to, from) \ > > > +({ \ > > > + size_t size = ARRAY_SIZE(to); \ > > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \ > > > + \ > > > + strscpy(to, from, size); \ > > > +}) > > > + > > > +#define stracpy_pad(to, from) \ > > > +({ \ > > > + size_t size = ARRAY_SIZE(to); \ > > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \ > > > + \ > > > + strscpy_pad(to, from, size); \ > > > +}) > > > + > > > #ifndef __HAVE_ARCH_STRCAT > > > extern char * strcat(char *, const char *); > > > #endif > > > > This seems like a reasonable addition, yes. I think Coccinelle might > > actually be able to find all the existing strscpy(dst, src, sizeof(dst)) > > cases to jump-start this conversion. > > I did that. It works. It's a lot of conversions. > > $ cat str.cpy.cocci > @@ > expression e1; > expression e2; > @@ > > - strscpy(e1, e2, sizeof(e1)) > + stracpy(e1, e2) > > @@ > expression e1; > expression e2; > @@ > > - strlcpy(e1, e2, sizeof(e1)) > + stracpy(e1, e2) > > > Devil's advocate: this adds yet more string handling functions... will > > this cause even more confusion? > > Documentation is good. > Actual in-kernel use and examples better. btw: I just ran this again and it produces: $ spatch --in-place -sp-file str.cpy.cocci . $ git checkout tools/ $ git diff --shortstat 958 files changed, 2179 insertions(+), 2655 deletions(-)
On Mon, Jul 22, 2019 at 10:58:15AM -0700, Joe Perches wrote: > On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote: > > On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote: > > > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote: > > > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote: > > > > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote: > > > > > > Added warnings in checkpatch.pl script to : > > > > > > > > > > > > 1. Deprecate strcpy() in favor of strscpy(). > > > > > > 2. Deprecate strlcpy() in favor of strscpy(). > > > > > > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad(). > > > > > > > > > > > > Updated strncpy() section in Documentation/process/deprecated.rst > > > > > > to cover strscpy_pad() case. > > > > > > > > [] > > > > > > > > I sent a patch series for some strscpy/strlcpy misuses. > > > > > > > > How about adding a macro helper to avoid the misuses like: > > > > --- > > > > include/linux/string.h | 16 ++++++++++++++++ > > > > 1 file changed, 16 insertions(+) > > > > > > > > diff --git a/include/linux/string.h b/include/linux/string.h > > > > index 4deb11f7976b..ef01bd6f19df 100644 > > > > --- a/include/linux/string.h > > > > +++ b/include/linux/string.h > > > > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t); > > > > /* Wraps calls to strscpy()/memset(), no arch specific code required */ > > > > ssize_t strscpy_pad(char *dest, const char *src, size_t count); > > > > > > > > +#define stracpy(to, from) \ > > > > +({ \ > > > > + size_t size = ARRAY_SIZE(to); \ > > > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \ > > > > + \ > > > > + strscpy(to, from, size); \ > > > > +}) > > > > + > > > > +#define stracpy_pad(to, from) \ > > > > +({ \ > > > > + size_t size = ARRAY_SIZE(to); \ > > > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \ > > > > + \ > > > > + strscpy_pad(to, from, size); \ > > > > +}) > > > > + > > > > #ifndef __HAVE_ARCH_STRCAT > > > > extern char * strcat(char *, const char *); > > > > #endif > > > > > > This seems like a reasonable addition, yes. I think Coccinelle might > > > actually be able to find all the existing strscpy(dst, src, sizeof(dst)) > > > cases to jump-start this conversion. > > > > I did that. It works. It's a lot of conversions. > > > > $ cat str.cpy.cocci > > @@ > > expression e1; > > expression e2; > > @@ > > > > - strscpy(e1, e2, sizeof(e1)) > > + stracpy(e1, e2) > > > > @@ > > expression e1; > > expression e2; > > @@ > > > > - strlcpy(e1, e2, sizeof(e1)) > > + stracpy(e1, e2) > > > > > Devil's advocate: this adds yet more string handling functions... will > > > this cause even more confusion? > > > > Documentation is good. > > Actual in-kernel use and examples better. > > btw: I just ran this again and it produces: > > $ spatch --in-place -sp-file str.cpy.cocci . > $ git checkout tools/ > $ git diff --shortstat > 958 files changed, 2179 insertions(+), 2655 deletions(-) Cool. Well, assuming no one hates this, let's do it. :) Can you send a more complete patch with docs, etc? Maybe Linus will take it for late in the next merge window, perhaps?
On Mon, Jul 22, 2019 at 10:58:15AM -0700, Joe Perches wrote: > On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote: > > On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote: > > > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote: > > > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote: > > > > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote: > > > > > > Added warnings in checkpatch.pl script to : > > > > > > > > > > > > 1. Deprecate strcpy() in favor of strscpy(). > > > > > > 2. Deprecate strlcpy() in favor of strscpy(). > > > > > > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad(). > > > > > > > > > > > > Updated strncpy() section in Documentation/process/deprecated.rst > > > > > > to cover strscpy_pad() case. > > > > > > > > [] > > > > > > > > I sent a patch series for some strscpy/strlcpy misuses. > > > > > > > > How about adding a macro helper to avoid the misuses like: > > > > --- > > > > include/linux/string.h | 16 ++++++++++++++++ > > > > 1 file changed, 16 insertions(+) > > > > > > > > diff --git a/include/linux/string.h b/include/linux/string.h > > > > index 4deb11f7976b..ef01bd6f19df 100644 > > > > --- a/include/linux/string.h > > > > +++ b/include/linux/string.h > > > > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t); > > > > /* Wraps calls to strscpy()/memset(), no arch specific code required */ > > > > ssize_t strscpy_pad(char *dest, const char *src, size_t count); > > > > > > > > +#define stracpy(to, from) \ > > > > +({ \ > > > > + size_t size = ARRAY_SIZE(to); \ > > > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \ > > > > + \ > > > > + strscpy(to, from, size); \ > > > > +}) Where does the 'a' in 'stracpy' come from? Googling around finds other people using a function called stracpy, but it takes different arguments. http://stracpy.blogspot.com/ takes a size argument, as does https://docs.polserver.com/doxygen/html/d5/dce/stracpy_8cpp_source.html The one in the 'Links' webbrowser (can't find a link to its source) seems like a strdup clone.
On Mon, 2019-07-22 at 11:27 -0700, Matthew Wilcox wrote: > On Mon, Jul 22, 2019 at 10:58:15AM -0700, Joe Perches wrote: > > On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote: > > > On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote: > > > > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote: > > > > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote: [] > > > > > +#define stracpy(to, from) \ > > > > > +({ \ > > > > > + size_t size = ARRAY_SIZE(to); \ > > > > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \ > > > > > + \ > > > > > + strscpy(to, from, size); \ > > > > > +}) > > Where does the 'a' in 'stracpy' come from? No place in particular. I used it because dst has to be an 'a'rray rather than a pointer.
diff --git a/include/linux/string.h b/include/linux/string.h index 4deb11f7976b..ef01bd6f19df 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t); /* Wraps calls to strscpy()/memset(), no arch specific code required */ ssize_t strscpy_pad(char *dest, const char *src, size_t count); +#define stracpy(to, from) \ +({ \ + size_t size = ARRAY_SIZE(to); \ + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \ + \ + strscpy(to, from, size); \ +}) + +#define stracpy_pad(to, from) \ +({ \ + size_t size = ARRAY_SIZE(to); \ + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \ + \ + strscpy_pad(to, from, size); \ +}) + #ifndef __HAVE_ARCH_STRCAT extern char * strcat(char *, const char *); #endif