It is a good rule to avoid submitting code without users.
Currently the stpcpy() is unusable due to missed declaration.
Any attempts to use it will bring something like:
error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]
Move declaration to the header and guard it as other string functions.
Fixes: 1e1b6d63d634 ("lib/string.c: implement stpcpy")
Reported-by: kernel test robot <[email protected]>
Cc: Nick Desaulniers <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
v3: new patch to fix reported issue
include/linux/string.h | 3 +++
lib/string.c | 3 ++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/string.h b/include/linux/string.h
index b6572aeca2f5..b1aeb3475396 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -31,6 +31,9 @@ size_t strlcpy(char *, const char *, size_t);
#ifndef __HAVE_ARCH_STRSCPY
ssize_t strscpy(char *, const char *, size_t);
#endif
+#ifndef __HAVE_ARCH_STPCPY
+char *stpcpy(char *__restrict__ dest, const char *__restrict__ src);
+#endif
/* Wraps calls to strscpy()/memset(), no arch specific code required */
ssize_t strscpy_pad(char *dest, const char *src, size_t count);
diff --git a/lib/string.c b/lib/string.c
index 485777c9da83..4ecb8ec1fdd1 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -233,6 +233,7 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
EXPORT_SYMBOL(strscpy);
#endif
+#ifndef __HAVE_ARCH_STPCPY
/**
* stpcpy - copy a string from src to dest returning a pointer to the new end
* of dest, including src's %NUL-terminator. May overrun dest.
@@ -248,7 +249,6 @@ EXPORT_SYMBOL(strscpy);
* not recommended for usage. Instead, its definition is provided in case
* the compiler lowers other libcalls to stpcpy.
*/
-char *stpcpy(char *__restrict__ dest, const char *__restrict__ src);
char *stpcpy(char *__restrict__ dest, const char *__restrict__ src)
{
while ((*dest++ = *src++) != '\0')
@@ -256,6 +256,7 @@ char *stpcpy(char *__restrict__ dest, const char *__restrict__ src)
return --dest;
}
EXPORT_SYMBOL(stpcpy);
+#endif
#ifndef __HAVE_ARCH_STRCAT
/**
--
2.34.1
The literals "big-endian" and "little-endian" may be potentially
occurred in other places. Dropping space allows linker to
"compress" them by using only a single copy.
Rasmus suggested, while at it, replacing strcpy() + strlen() by
p = stpcpy(), which is done here as well.
Signed-off-by: Andy Shevchenko <[email protected]>
Acked-by: Sakari Ailus <[email protected]>
---
v3: no changes
lib/vsprintf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 4e8f3e9acb99..e2a1d89f1a5c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1781,8 +1781,8 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
*p++ = isascii(c) && isprint(c) ? c : '.';
}
- strcpy(p, orig & BIT(31) ? " big-endian" : " little-endian");
- p += strlen(p);
+ *p++ = ' ';
+ p = stpcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
*p++ = ' ';
*p++ = '(';
--
2.34.1
On Wed, Jan 26, 2022 at 04:19:15PM +0200, Andy Shevchenko wrote:
> It is a good rule to avoid submitting code without users.
While I agree with the sentiment in the general case, I don't think that
it applies in this case and this comment should be dropped. The message
of the commit this fixes and the comment right above the declaration
both make it pretty obvious why this interface was added with no in-tree
users and why the declaration was placed right above the definition.
> Currently the stpcpy() is unusable due to missed declaration.
> Any attempts to use it will bring something like:
>
> error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]
>
> Move declaration to the header and guard it as other string functions.
>
> Fixes: 1e1b6d63d634 ("lib/string.c: implement stpcpy")
> Reported-by: kernel test robot <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
Regardless, the commit itself seems fine from a technical standpoint. I
won't comment on whether or not this interface should be opened up.
Reviewed-by: Nathan Chancellor <[email protected]>
> ---
> v3: new patch to fix reported issue
> include/linux/string.h | 3 +++
> lib/string.c | 3 ++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b6572aeca2f5..b1aeb3475396 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -31,6 +31,9 @@ size_t strlcpy(char *, const char *, size_t);
> #ifndef __HAVE_ARCH_STRSCPY
> ssize_t strscpy(char *, const char *, size_t);
> #endif
> +#ifndef __HAVE_ARCH_STPCPY
> +char *stpcpy(char *__restrict__ dest, const char *__restrict__ src);
> +#endif
>
> /* Wraps calls to strscpy()/memset(), no arch specific code required */
> ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> diff --git a/lib/string.c b/lib/string.c
> index 485777c9da83..4ecb8ec1fdd1 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -233,6 +233,7 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
> EXPORT_SYMBOL(strscpy);
> #endif
>
> +#ifndef __HAVE_ARCH_STPCPY
> /**
> * stpcpy - copy a string from src to dest returning a pointer to the new end
> * of dest, including src's %NUL-terminator. May overrun dest.
> @@ -248,7 +249,6 @@ EXPORT_SYMBOL(strscpy);
> * not recommended for usage. Instead, its definition is provided in case
> * the compiler lowers other libcalls to stpcpy.
> */
> -char *stpcpy(char *__restrict__ dest, const char *__restrict__ src);
> char *stpcpy(char *__restrict__ dest, const char *__restrict__ src)
> {
> while ((*dest++ = *src++) != '\0')
> @@ -256,6 +256,7 @@ char *stpcpy(char *__restrict__ dest, const char *__restrict__ src)
> return --dest;
> }
> EXPORT_SYMBOL(stpcpy);
> +#endif
>
> #ifndef __HAVE_ARCH_STRCAT
> /**
> --
> 2.34.1
>
>
On Wed, Jan 26, 2022 at 6:19 AM Andy Shevchenko
<[email protected]> wrote:
>
> It is a good rule to avoid submitting code without users.
> Currently the stpcpy() is unusable due to missed declaration.
> Any attempts to use it will bring something like:
>
> error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]
>
> Move declaration to the header and guard it as other string functions.
>
> Fixes: 1e1b6d63d634 ("lib/string.c: implement stpcpy")
> Reported-by: kernel test robot <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
Recall the discussion from Kees:
https://lore.kernel.org/lkml/CAK7LNAQXo5-5W6hvNMEVPBPf3tRWaf-pQdSR-0OHyi4RCGhjsQ@mail.gmail.com/
and
https://lore.kernel.org/lkml/202008150921.B70721A359@keescook/
I'm guessing that the 0day bot report was on the third patch in this
series, which looks to use stpcpy:
https://lore.kernel.org/lkml/[email protected]/
for patch 3, I'd s/"compress"/de-duplicate/ or s/"compress"/merge/.
Kees, I'm curious if we can do something like require the string
length to be known at compile time otherwise BUILD_BUG_ON?
> ---
> v3: new patch to fix reported issue
> include/linux/string.h | 3 +++
> lib/string.c | 3 ++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b6572aeca2f5..b1aeb3475396 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -31,6 +31,9 @@ size_t strlcpy(char *, const char *, size_t);
> #ifndef __HAVE_ARCH_STRSCPY
> ssize_t strscpy(char *, const char *, size_t);
> #endif
> +#ifndef __HAVE_ARCH_STPCPY
> +char *stpcpy(char *__restrict__ dest, const char *__restrict__ src);
> +#endif
>
> /* Wraps calls to strscpy()/memset(), no arch specific code required */
> ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> diff --git a/lib/string.c b/lib/string.c
> index 485777c9da83..4ecb8ec1fdd1 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -233,6 +233,7 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
> EXPORT_SYMBOL(strscpy);
> #endif
>
> +#ifndef __HAVE_ARCH_STPCPY
> /**
> * stpcpy - copy a string from src to dest returning a pointer to the new end
> * of dest, including src's %NUL-terminator. May overrun dest.
> @@ -248,7 +249,6 @@ EXPORT_SYMBOL(strscpy);
> * not recommended for usage. Instead, its definition is provided in case
> * the compiler lowers other libcalls to stpcpy.
> */
> -char *stpcpy(char *__restrict__ dest, const char *__restrict__ src);
> char *stpcpy(char *__restrict__ dest, const char *__restrict__ src)
> {
> while ((*dest++ = *src++) != '\0')
> @@ -256,6 +256,7 @@ char *stpcpy(char *__restrict__ dest, const char *__restrict__ src)
> return --dest;
> }
> EXPORT_SYMBOL(stpcpy);
> +#endif
>
> #ifndef __HAVE_ARCH_STRCAT
> /**
> --
> 2.34.1
>
--
Thanks,
~Nick Desaulniers
On Wed, Jan 26, 2022 at 09:38:02AM -0700, Nathan Chancellor wrote:
> On Wed, Jan 26, 2022 at 04:19:15PM +0200, Andy Shevchenko wrote:
> > It is a good rule to avoid submitting code without users.
>
> While I agree with the sentiment in the general case, I don't think that
> it applies in this case and this comment should be dropped. The message
> of the commit this fixes and the comment right above the declaration
> both make it pretty obvious why this interface was added with no in-tree
> users and why the declaration was placed right above the definition.
Thanks for accenting on this. Yes, I see now the reasoning and I don't
know which way is better. As a consumer of this API it shows a room for
micro-optimizations (I dunno if GCC and/or Clang able to replace the two
by stpcpy(), as done in the patch, at compile time).
That said, depending on the others' opinions let see how to proceed.
> > Currently the stpcpy() is unusable due to missed declaration.
> > Any attempts to use it will bring something like:
> >
> > error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]
> >
> > Move declaration to the header and guard it as other string functions.
> >
> > Fixes: 1e1b6d63d634 ("lib/string.c: implement stpcpy")
> > Reported-by: kernel test robot <[email protected]>
> > Cc: Nick Desaulniers <[email protected]>
> > Cc: Nathan Chancellor <[email protected]>
> > Signed-off-by: Andy Shevchenko <[email protected]>
>
> Regardless, the commit itself seems fine from a technical standpoint. I
> won't comment on whether or not this interface should be opened up.
>
> Reviewed-by: Nathan Chancellor <[email protected]>
Thanks!
--
With Best Regards,
Andy Shevchenko
On Wed, Jan 26, 2022 at 09:49:38AM -0800, Nick Desaulniers wrote:
> On Wed, Jan 26, 2022 at 6:19 AM Andy Shevchenko
> <[email protected]> wrote:
> >
> > It is a good rule to avoid submitting code without users.
> > Currently the stpcpy() is unusable due to missed declaration.
> > Any attempts to use it will bring something like:
> >
> > error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]
> >
> > Move declaration to the header and guard it as other string functions.
...
> Recall the discussion from Kees:
> https://lore.kernel.org/lkml/CAK7LNAQXo5-5W6hvNMEVPBPf3tRWaf-pQdSR-0OHyi4RCGhjsQ@mail.gmail.com/
> and
> https://lore.kernel.org/lkml/202008150921.B70721A359@keescook/
> I'm guessing that the 0day bot report was on the third patch in this
> series, which looks to use stpcpy:
> https://lore.kernel.org/lkml/[email protected]/
>
> for patch 3,
Yes. It was Rasmus' suggestion.
> I'd s/"compress"/de-duplicate/ or s/"compress"/merge/.
I'll fix it locally.
> Kees, I'm curious if we can do something like require the string
> length to be known at compile time otherwise BUILD_BUG_ON?
Perhaps static_assert() is better.
--
With Best Regards,
Andy Shevchenko
On Wed, Jan 26, 2022 at 09:49:38AM -0800, Nick Desaulniers wrote:
> On Wed, Jan 26, 2022 at 6:19 AM Andy Shevchenko
> <[email protected]> wrote:
> >
> > It is a good rule to avoid submitting code without users.
> > Currently the stpcpy() is unusable due to missed declaration.
> > Any attempts to use it will bring something like:
> >
> > error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]
> >
> > Move declaration to the header and guard it as other string functions.
...
> Recall the discussion from Kees:
> https://lore.kernel.org/lkml/CAK7LNAQXo5-5W6hvNMEVPBPf3tRWaf-pQdSR-0OHyi4RCGhjsQ@mail.gmail.com/
> and
> https://lore.kernel.org/lkml/202008150921.B70721A359@keescook/
For the record :-)
https://lore.kernel.org/lkml/CAHp75VfniSw3AFTyyDk2OoAChGx7S6wF7sZKpJXNHmk97BoRXA@mail.gmail.com/
--
With Best Regards,
Andy Shevchenko
On Wed, Jan 26, 2022 at 08:12:22PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 26, 2022 at 09:49:38AM -0800, Nick Desaulniers wrote:
> > On Wed, Jan 26, 2022 at 6:19 AM Andy Shevchenko
> > <[email protected]> wrote:
> > >
> > > It is a good rule to avoid submitting code without users.
> > > Currently the stpcpy() is unusable due to missed declaration.
> > > Any attempts to use it will bring something like:
> > >
> > > error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]
> > >
> > > Move declaration to the header and guard it as other string functions.
>
> ...
>
> > Recall the discussion from Kees:
> > https://lore.kernel.org/lkml/CAK7LNAQXo5-5W6hvNMEVPBPf3tRWaf-pQdSR-0OHyi4RCGhjsQ@mail.gmail.com/
> > and
> > https://lore.kernel.org/lkml/202008150921.B70721A359@keescook/
>
> For the record :-)
> https://lore.kernel.org/lkml/CAHp75VfniSw3AFTyyDk2OoAChGx7S6wF7sZKpJXNHmk97BoRXA@mail.gmail.com/
> [...
> strcpy() is not a bad API for the cases when you know what you are
> doing. A problem that most of the developers do not know what they are
> doing.
> No need to split everything to bad and good by its name or semantics,
> each API has its own pros and cons and programmers must use their
> brains.
> ...]
Developers should not need to remember to avoid foot-guns; the toolchain
should be doing all of that. The trouble is that C (and its standard
libs) are filled with foot-guns.
I do not want to add another foot-gun API to the kernel; we've been
working very hard to _remove_ them. :) If the kernel's stpcpy() _only_
worked on all known-size strings, etc, so that memory safety could be
determined at compile-time, then I'd have no objection.
What's not clear to me is if such macro versions would be workable for
the reason stpcpy() was added in the first place, which was the compiler
transforming other calls stuff into library calls it thinks are defined.
Totally untested:
#define stpcpy(dst, src) ({ \
size_t _stp__dst = __builtin_object_size(dst, 1); \
size_t _stp__src = __builtin_object_size(src, 1); \
\
BUILD_BUG_ON(_stp__dst == -1 || _stp__src == -1); \
BUILD_BUG_ON(_stp__src > _stp__dst); \
\
__builtin_stpcpy(dst, src); \
})
(Is there even a __builtin_stpcpy()?)
--
Kees Cook
On Wed, Jan 26, 2022 at 04:19:17PM +0200, Andy Shevchenko wrote:
> The literals "big-endian" and "little-endian" may be potentially
> occurred in other places. Dropping space allows linker to
> "compress" them by using only a single copy.
>
> Rasmus suggested, while at it, replacing strcpy() + strlen() by
> p = stpcpy(), which is done here as well.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Acked-by: Sakari Ailus <[email protected]>
> ---
> v3: no changes
> lib/vsprintf.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 4e8f3e9acb99..e2a1d89f1a5c 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1781,8 +1781,8 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> *p++ = isascii(c) && isprint(c) ? c : '.';
> }
>
> - strcpy(p, orig & BIT(31) ? " big-endian" : " little-endian");
> - p += strlen(p);
> + *p++ = ' ';
> + p = stpcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
>
> *p++ = ' ';
> *p++ = '(';
No need for any of the manual construction nor stpcpy():
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7ad44f2c8f5..aef8bd2789a9 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1788,14 +1788,9 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
*p++ = isascii(c) && isprint(c) ? c : '.';
}
- strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
- p += strlen(p);
-
- *p++ = ' ';
- *p++ = '(';
- p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, sizeof(u32));
- *p++ = ')';
- *p = '\0';
+ snprintf(p, sizeof(output) - sizeof(*fourcc), " %s (0x%x)",
+ *fourcc & BIT(31) ? "big-endian" : " little-endian",
+ *fourcc);
return string(buf, end, output, spec);
}
(untested)
--
Kees Cook
Hi Kees,
On Wed, Jan 26, 2022 at 01:22:32PM -0800, Kees Cook wrote:
> On Wed, Jan 26, 2022 at 04:19:17PM +0200, Andy Shevchenko wrote:
> > The literals "big-endian" and "little-endian" may be potentially
> > occurred in other places. Dropping space allows linker to
> > "compress" them by using only a single copy.
> >
> > Rasmus suggested, while at it, replacing strcpy() + strlen() by
> > p = stpcpy(), which is done here as well.
> >
> > Signed-off-by: Andy Shevchenko <[email protected]>
> > Acked-by: Sakari Ailus <[email protected]>
> > ---
> > v3: no changes
> > lib/vsprintf.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 4e8f3e9acb99..e2a1d89f1a5c 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1781,8 +1781,8 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > *p++ = isascii(c) && isprint(c) ? c : '.';
> > }
> >
> > - strcpy(p, orig & BIT(31) ? " big-endian" : " little-endian");
> > - p += strlen(p);
> > + *p++ = ' ';
> > + p = stpcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
> >
> > *p++ = ' ';
> > *p++ = '(';
>
> No need for any of the manual construction nor stpcpy():
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index d7ad44f2c8f5..aef8bd2789a9 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1788,14 +1788,9 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> *p++ = isascii(c) && isprint(c) ? c : '.';
> }
>
> - strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
> - p += strlen(p);
> -
> - *p++ = ' ';
> - *p++ = '(';
> - p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, sizeof(u32));
> - *p++ = ')';
> - *p = '\0';
> + snprintf(p, sizeof(output) - sizeof(*fourcc), " %s (0x%x)",
> + *fourcc & BIT(31) ? "big-endian" : " little-endian",
> + *fourcc);
>
> return string(buf, end, output, spec);
> }
>
>
> (untested)
This could work but results in two nested calls to vsnprintf(). I'd
definitely avoid that, due to stack usage and then albeit it's easy to see
the recursion will end with two iterations, that will very fast cease to be
the case if you keep applying the pattern.
--
Regards,
Sakari Ailus
On Wed, Jan 26, 2022 at 01:22:32PM -0800, Kees Cook wrote:
> On Wed, Jan 26, 2022 at 04:19:17PM +0200, Andy Shevchenko wrote:
...
> > *p++ = ' ';
> > *p++ = '(';
>
> No need for any of the manual construction nor stpcpy():
No, we are better to avoid recursive printf() calls.
Entire vsprintf.c is written keeping that in mind.
--
With Best Regards,
Andy Shevchenko