| Message ID | 20200815002417.1512973-1-ndesaulniers@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | lib/string.c: implement stpcpy | expand |
On Fri, 2020-08-14 at 17:24 -0700, Nick Desaulniers wrote: > LLVM implemented a recent "libcall optimization" that lowers calls to > `sprintf(dest, "%s", str)` where the return value is used to > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved > in parsing format strings. [] > diff --git 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 > +extern char *stpcpy(char *__restrict, const char *__restrict__); Why use two different forms for __restrict and __restrict__? Any real reason to use __restrict__ at all? It's used nowhere else in the kernel. $ git grep -w -P '__restrict_{0,2}' scripts/genksyms/keywords.c: // According to rth, c99 defines "_Bool", __restrict", __restrict__", "restrict". KAO scripts/genksyms/keywords.c: { "__restrict__", RESTRICT_KEYW },
Hi Nick, On Fri, Aug 14, 2020 at 5:24 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > LLVM implemented a recent "libcall optimization" that lowers calls to > `sprintf(dest, "%s", str)` where the return value is used to > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved > in parsing format strings. > > `stpcpy` is just like `strcpy` except: > 1. it returns the pointer to the new tail of `dest`. This allows you to > chain multiple calls to `stpcpy` in one statement. > 2. it requires the parameters not to overlap. Calling `sprintf` with > overlapping arguments was clarified in ISO C99 and POSIX.1-2001 to be > undefined behavior. > > `stpcpy` was first standardized in POSIX.1-2008. > > Implement this so that we don't observe linkage failures due to missing > symbol definitions for `stpcpy`. > > Similar to last year's fire drill with: > commit 5f074f3e192f ("lib/string.c: implement a basic bcmp") > > This optimization was introduced into clang-12. > > Cc: stable@vger.kernel.org > Link: https://bugs.llvm.org/show_bug.cgi?id=47162 > Link: https://github.com/ClangBuiltLinux/linux/issues/1126 > Link: https://man7.org/linux/man-pages/man3/stpcpy.3.html > Link: https://pubs.opengroup.org/onlinepubs/9699919799/functions/stpcpy.html > Link: https://reviews.llvm.org/D85963 > Reported-by: Sami Tolvanen <samitolvanen@google.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > --- > include/linux/string.h | 3 +++ > lib/string.c | 23 +++++++++++++++++++++++ > 2 files changed, 26 insertions(+) Thank you for the patch! I can confirm that this fixes the build for me with ToT Clang. Tested-by: Sami Tolvanen <samitolvanen@google.com> Sami
On Fri, Aug 14, 2020 at 05:24:15PM -0700, Nick Desaulniers wrote: > +#ifndef __HAVE_ARCH_STPCPY > +/** > + * stpcpy - copy a string from src to dest returning a pointer to the new end > + * of dest, including src's NULL terminator. May overrun dest. > + * @dest: pointer to end of string being copied into. Must be large enough > + * to receive copy. > + * @src: pointer to the beginning of string being copied from. Must not overlap > + * dest. > + * > + * stpcpy differs from strcpy in two key ways: > + * 1. inputs must not overlap. > + * 2. return value is the new NULL terminated character. (for strcpy, the > + * return value is a pointer to src. > + */ > +#undef stpcpy > +char *stpcpy(char *__restrict__ dest, const char *__restrict__ src) > +{ > + while ((*dest++ = *src++) != '\0') > + /* nothing */; > + return dest; > +} > +#endif > + Won't this return a pointer that's one _past_ the terminating NUL? I think you need the increments to be inside the loop body, rather than as part of the condition. Nit: NUL is more correct than NULL to refer to the string terminator.
On Fri, Aug 14, 2020 at 09:33:10PM -0400, Arvind Sankar wrote: > On Fri, Aug 14, 2020 at 05:24:15PM -0700, Nick Desaulniers wrote: > > +#ifndef __HAVE_ARCH_STPCPY > > +/** > > + * stpcpy - copy a string from src to dest returning a pointer to the new end > > + * of dest, including src's NULL terminator. May overrun dest. > > + * @dest: pointer to end of string being copied into. Must be large enough > > + * to receive copy. > > + * @src: pointer to the beginning of string being copied from. Must not overlap > > + * dest. > > + * > > + * stpcpy differs from strcpy in two key ways: > > + * 1. inputs must not overlap. > > + * 2. return value is the new NULL terminated character. (for strcpy, the > > + * return value is a pointer to src. > > + */ > > +#undef stpcpy > > +char *stpcpy(char *__restrict__ dest, const char *__restrict__ src) > > +{ > > + while ((*dest++ = *src++) != '\0') > > + /* nothing */; > > + return dest; > > +} > > +#endif > > + > > Won't this return a pointer that's one _past_ the terminating NUL? I > think you need the increments to be inside the loop body, rather than as > part of the condition. > > Nit: NUL is more correct than NULL to refer to the string terminator. Also, strcpy (at least the one in the C standard) is undefined if the strings overlap, so that's not really a difference.
On Fri, Aug 14, 2020 at 5:52 PM Joe Perches <joe@perches.com> wrote: > > On Fri, 2020-08-14 at 17:24 -0700, Nick Desaulniers wrote: > > LLVM implemented a recent "libcall optimization" that lowers calls to > > `sprintf(dest, "%s", str)` where the return value is used to > > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved > > in parsing format strings. > [] > > diff --git 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 > > +extern char *stpcpy(char *__restrict, const char *__restrict__); > > Why use two different forms for __restrict and __restrict__? > Any real reason to use __restrict__ at all? Bah, sorry, I recently enabled some setting in my ~/.vimrc to help me find my cursor better: " highlight cursor set cursorline set cursorcolumn Turns out this makes it pretty difficult to see underscores, or the lack thereof. Will fix up. > > It's used nowhere else in the kernel. > > $ git grep -w -P '__restrict_{0,2}' > scripts/genksyms/keywords.c: // According to rth, c99 defines "_Bool", __restrict", __restrict__", "restrict". KAO > scripts/genksyms/keywords.c: { "__restrict__", RESTRICT_KEYW }, > > -- Thanks, ~Nick Desaulniers
On Fri, Aug 14, 2020 at 6:33 PM Arvind Sankar <nivedita@alum.mit.edu> wrote: > > On Fri, Aug 14, 2020 at 05:24:15PM -0700, Nick Desaulniers wrote: > > +#ifndef __HAVE_ARCH_STPCPY > > +/** > > + * stpcpy - copy a string from src to dest returning a pointer to the new end > > + * of dest, including src's NULL terminator. May overrun dest. > > + * @dest: pointer to end of string being copied into. Must be large enough > > + * to receive copy. > > + * @src: pointer to the beginning of string being copied from. Must not overlap > > + * dest. > > + * > > + * stpcpy differs from strcpy in two key ways: > > + * 1. inputs must not overlap. > > + * 2. return value is the new NULL terminated character. (for strcpy, the > > + * return value is a pointer to src. > > + */ > > +#undef stpcpy > > +char *stpcpy(char *__restrict__ dest, const char *__restrict__ src) > > +{ > > + while ((*dest++ = *src++) != '\0') > > + /* nothing */; > > + return dest; > > +} > > +#endif > > + > > Won't this return a pointer that's one _past_ the terminating NUL? I > think you need the increments to be inside the loop body, rather than as > part of the condition. Yep, looks like I had a bug in my test program that masked this. Thanks for triple checking. > > Nit: NUL is more correct than NULL to refer to the string terminator. TIL. -- Thanks, ~Nick Desaulniers
On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote: > LLVM implemented a recent "libcall optimization" that lowers calls to > `sprintf(dest, "%s", str)` where the return value is used to > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved > in parsing format strings. Calling `sprintf` with overlapping arguments > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior. > > `stpcpy` is just like `strcpy` except it returns the pointer to the new > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in > one statement. O_O What? No; this is a _terrible_ API: there is no bounds checking, there are no buffer sizes. Anything using the example sprintf() pattern is _already_ wrong and must be removed from the kernel. (Yes, I realize that the kernel is *filled* with this bad assumption that "I'll never write more than PAGE_SIZE bytes to this buffer", but that's both theoretically wrong ("640k is enough for anybody") and has been known to be wrong in practice too (e.g. when suddenly your writing routine is reachable by splice(2) and you may not have a PAGE_SIZE buffer). But we cannot _add_ another dangerous string API. We're already in a terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This needs to be addressed up by removing the unbounded sprintf() uses. (And to do so without introducing bugs related to using snprintf() when scnprintf() is expected[4].) -Kees [1] https://github.com/KSPP/linux/issues/88 [2] https://github.com/KSPP/linux/issues/89 [3] https://github.com/KSPP/linux/issues/90 [4] https://lore.kernel.org/lkml/20200810092100.GA42813@2f5448a72a42/ -- Kees Cook
Yeah, sprintf calls should be replaced with something safer. > Dňa 15. 8. 2020 o 18:34 užívateľ Kees Cook <keescook@chromium.org> napísal: > > On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote: >> LLVM implemented a recent "libcall optimization" that lowers calls to >> `sprintf(dest, "%s", str)` where the return value is used to >> `stpcpy(dest, str) - dest`. This generally avoids the machinery involved >> in parsing format strings. Calling `sprintf` with overlapping arguments >> was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior. >> >> `stpcpy` is just like `strcpy` except it returns the pointer to the new >> tail of `dest`. This allows you to chain multiple calls to `stpcpy` in >> one statement. > > O_O What? > > No; this is a _terrible_ API: there is no bounds checking, there are no > buffer sizes. Anything using the example sprintf() pattern is _already_ > wrong and must be removed from the kernel. (Yes, I realize that the > kernel is *filled* with this bad assumption that "I'll never write more > than PAGE_SIZE bytes to this buffer", but that's both theoretically > wrong ("640k is enough for anybody") and has been known to be wrong in > practice too (e.g. when suddenly your writing routine is reachable by > splice(2) and you may not have a PAGE_SIZE buffer). > > But we cannot _add_ another dangerous string API. We're already in a > terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This > needs to be addressed up by removing the unbounded sprintf() uses. (And > to do so without introducing bugs related to using snprintf() when > scnprintf() is expected[4].) > > -Kees > > [1] https://github.com/KSPP/linux/issues/88 > [2] https://github.com/KSPP/linux/issues/89 > [3] https://github.com/KSPP/linux/issues/90 > [4] https://lore.kernel.org/lkml/20200810092100.GA42813@2f5448a72a42/ > > -- > Kees Cook
On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote: > > On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote: > > LLVM implemented a recent "libcall optimization" that lowers calls to > > `sprintf(dest, "%s", str)` where the return value is used to > > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved > > in parsing format strings. Calling `sprintf` with overlapping arguments > > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior. > > > > `stpcpy` is just like `strcpy` except it returns the pointer to the new > > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in > > one statement. > > O_O What? > > No; this is a _terrible_ API: there is no bounds checking, there are no > buffer sizes. Anything using the example sprintf() pattern is _already_ > wrong and must be removed from the kernel. (Yes, I realize that the > kernel is *filled* with this bad assumption that "I'll never write more > than PAGE_SIZE bytes to this buffer", but that's both theoretically > wrong ("640k is enough for anybody") and has been known to be wrong in > practice too (e.g. when suddenly your writing routine is reachable by > splice(2) and you may not have a PAGE_SIZE buffer). > > But we cannot _add_ another dangerous string API. We're already in a > terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This > needs to be addressed up by removing the unbounded sprintf() uses. (And > to do so without introducing bugs related to using snprintf() when > scnprintf() is expected[4].) Well, everything (-next, mainline, stable) is broken right now (with ToT Clang) without providing this symbol. I'm not going to go clean the entire kernel's use of sprintf to get our CI back to being green. Thoughts on not exposing the declaration in the header, and changing the comment to be to the effect of: "Exists only for optimizing compilers to replace calls to sprintf with; generally unsafe as bounds checks aren't performed, do not use." It's still a worthwhile optimization to have, even if the use that generated it didn't do any bounds checking. > > -Kees > > [1] https://github.com/KSPP/linux/issues/88 > [2] https://github.com/KSPP/linux/issues/89 > [3] https://github.com/KSPP/linux/issues/90 > [4] https://lore.kernel.org/lkml/20200810092100.GA42813@2f5448a72a42/ > > -- > Kees Cook -- Thanks, ~Nick Desaulniers
On Sat, Aug 15, 2020 at 2:24 PM Joe Perches <joe@perches.com> wrote: > > On Sat, 2020-08-15 at 13:47 -0700, Nick Desaulniers wrote: > > On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote: > > > On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote: > > > > LLVM implemented a recent "libcall optimization" that lowers calls to > > > > `sprintf(dest, "%s", str)` where the return value is used to > > > > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved > > > > in parsing format strings. Calling `sprintf` with overlapping arguments > > > > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior. > > > > > > > > `stpcpy` is just like `strcpy` except it returns the pointer to the new > > > > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in > > > > one statement. > > > > > > O_O What? > > > > > > No; this is a _terrible_ API: there is no bounds checking, there are no > > > buffer sizes. Anything using the example sprintf() pattern is _already_ > > > wrong and must be removed from the kernel. (Yes, I realize that the > > > kernel is *filled* with this bad assumption that "I'll never write more > > > than PAGE_SIZE bytes to this buffer", but that's both theoretically > > > wrong ("640k is enough for anybody") and has been known to be wrong in > > > practice too (e.g. when suddenly your writing routine is reachable by > > > splice(2) and you may not have a PAGE_SIZE buffer). > > > > > > But we cannot _add_ another dangerous string API. We're already in a > > > terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This > > > needs to be addressed up by removing the unbounded sprintf() uses. (And > > > to do so without introducing bugs related to using snprintf() when > > > scnprintf() is expected[4].) > > > > Well, everything (-next, mainline, stable) is broken right now (with > > ToT Clang) without providing this symbol. I'm not going to go clean > > the entire kernel's use of sprintf to get our CI back to being green. > > Maybe this should get place in compiler-clang.h so it isn't > generic and public. https://bugs.llvm.org/show_bug.cgi?id=47162#c7 and https://bugs.llvm.org/show_bug.cgi?id=47144 Seem to imply that Clang is not the only compiler that can lower a sequence of libcalls to stpcpy. Do we want to wait until we have a fire drill w/ GCC to move such an implementation from include/linux/compiler-clang.h back in to lib/string.c? > > Something like: > > --- > include/linux/compiler-clang.h | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h > index cee0c728d39a..6279f1904e39 100644 > --- a/include/linux/compiler-clang.h > +++ b/include/linux/compiler-clang.h > @@ -61,3 +61,30 @@ > #if __has_feature(shadow_call_stack) > # define __noscs __attribute__((__no_sanitize__("shadow-call-stack"))) > #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 NULL terminator. May overrun dest. > + * @dest: pointer to buffer being copied into. > + * Must be large enough to receive copy. > + * @src: pointer to the beginning of string being copied from. > + * Must not overlap dest. > + * > + * This function exists _only_ to support clang's possible conversion of > + * sprintf calls to stpcpy. > + * > + * stpcpy differs from strcpy in two key ways: > + * 1. inputs must not overlap. > + * 2. return value is dest's NUL termination character after copy. > + * (for strcpy, the return value is a pointer to src) > + */ > + > +static inline char *stpcpy(char __restrict *dest, const char __restrict *src) > +{ > + while ((*dest++ = *src++) != '\0') { > + ; /* nothing */ > + } > + return --dest; > +} > +#endif > >
On Sat, 2020-08-15 at 14:28 -0700, Nick Desaulniers wrote: > On Sat, Aug 15, 2020 at 2:24 PM Joe Perches <joe@perches.com> wrote: > > On Sat, 2020-08-15 at 13:47 -0700, Nick Desaulniers wrote: > > > On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote: > > > > On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote: > > > > > LLVM implemented a recent "libcall optimization" that lowers calls to > > > > > `sprintf(dest, "%s", str)` where the return value is used to > > > > > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved > > > > > in parsing format strings. Calling `sprintf` with overlapping arguments > > > > > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior. > > > > > > > > > > `stpcpy` is just like `strcpy` except it returns the pointer to the new > > > > > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in > > > > > one statement. > > > > > > > > O_O What? > > > > > > > > No; this is a _terrible_ API: there is no bounds checking, there are no > > > > buffer sizes. Anything using the example sprintf() pattern is _already_ > > > > wrong and must be removed from the kernel. (Yes, I realize that the > > > > kernel is *filled* with this bad assumption that "I'll never write more > > > > than PAGE_SIZE bytes to this buffer", but that's both theoretically > > > > wrong ("640k is enough for anybody") and has been known to be wrong in > > > > practice too (e.g. when suddenly your writing routine is reachable by > > > > splice(2) and you may not have a PAGE_SIZE buffer). > > > > > > > > But we cannot _add_ another dangerous string API. We're already in a > > > > terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This > > > > needs to be addressed up by removing the unbounded sprintf() uses. (And > > > > to do so without introducing bugs related to using snprintf() when > > > > scnprintf() is expected[4].) > > > > > > Well, everything (-next, mainline, stable) is broken right now (with > > > ToT Clang) without providing this symbol. I'm not going to go clean > > > the entire kernel's use of sprintf to get our CI back to being green. > > > > Maybe this should get place in compiler-clang.h so it isn't > > generic and public. > > https://bugs.llvm.org/show_bug.cgi?id=47162#c7 and > https://bugs.llvm.org/show_bug.cgi?id=47144 > Seem to imply that Clang is not the only compiler that can lower a > sequence of libcalls to stpcpy. Do we want to wait until we have a > fire drill w/ GCC to move such an implementation from > include/linux/compiler-clang.h back in to lib/string.c? My guess is yes, wait until gcc, if ever, needs it.
On Sat, Aug 15, 2020 at 2:31 PM Joe Perches <joe@perches.com> wrote: > > On Sat, 2020-08-15 at 14:28 -0700, Nick Desaulniers wrote: > > On Sat, Aug 15, 2020 at 2:24 PM Joe Perches <joe@perches.com> wrote: > > > On Sat, 2020-08-15 at 13:47 -0700, Nick Desaulniers wrote: > > > > On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote: > > > > > On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote: > > > > > > LLVM implemented a recent "libcall optimization" that lowers calls to > > > > > > `sprintf(dest, "%s", str)` where the return value is used to > > > > > > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved > > > > > > in parsing format strings. Calling `sprintf` with overlapping arguments > > > > > > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior. > > > > > > > > > > > > `stpcpy` is just like `strcpy` except it returns the pointer to the new > > > > > > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in > > > > > > one statement. > > > > > > > > > > O_O What? > > > > > > > > > > No; this is a _terrible_ API: there is no bounds checking, there are no > > > > > buffer sizes. Anything using the example sprintf() pattern is _already_ > > > > > wrong and must be removed from the kernel. (Yes, I realize that the > > > > > kernel is *filled* with this bad assumption that "I'll never write more > > > > > than PAGE_SIZE bytes to this buffer", but that's both theoretically > > > > > wrong ("640k is enough for anybody") and has been known to be wrong in > > > > > practice too (e.g. when suddenly your writing routine is reachable by > > > > > splice(2) and you may not have a PAGE_SIZE buffer). > > > > > > > > > > But we cannot _add_ another dangerous string API. We're already in a > > > > > terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This > > > > > needs to be addressed up by removing the unbounded sprintf() uses. (And > > > > > to do so without introducing bugs related to using snprintf() when > > > > > scnprintf() is expected[4].) > > > > > > > > Well, everything (-next, mainline, stable) is broken right now (with > > > > ToT Clang) without providing this symbol. I'm not going to go clean > > > > the entire kernel's use of sprintf to get our CI back to being green. > > > > > > Maybe this should get place in compiler-clang.h so it isn't > > > generic and public. > > > > https://bugs.llvm.org/show_bug.cgi?id=47162#c7 and > > https://bugs.llvm.org/show_bug.cgi?id=47144 > > Seem to imply that Clang is not the only compiler that can lower a > > sequence of libcalls to stpcpy. Do we want to wait until we have a > > fire drill w/ GCC to move such an implementation from > > include/linux/compiler-clang.h back in to lib/string.c? > > My guess is yes, wait until gcc, if ever, needs it. The suggestion to use static inline doesn't even make sense. The compiler is lowering calls to other library routines; `stpcpy` isn't being explicitly called. Even if it was, not sure we want it being inlined. No symbol definition will be emitted; problem not solved. And I refuse to add any more code using `extern inline`. Putting the definition in lib/string.c is the most straightforward and avoids revisiting this issue in the future for other toolchains. I'll limit access by removing the declaration, and adding a comment to avoid its use. But if you're going to use a gnu target triple without using -ffreestanding because you *want* libcall optimizations, then you have to provide symbols for all possible library routines! -- Thanks, ~Nick Desaulniers
From: Nick Desaulniers > Sent: 15 August 2020 01:24 > > LLVM implemented a recent "libcall optimization" that lowers calls to > `sprintf(dest, "%s", str)` where the return value is used to > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved > in parsing format strings. > > `stpcpy` is just like `strcpy` except: > 1. it returns the pointer to the new tail of `dest`. This allows you to > chain multiple calls to `stpcpy` in one statement. > 2. it requires the parameters not to overlap. Calling `sprintf` with > overlapping arguments was clarified in ISO C99 and POSIX.1-2001 to be > undefined behavior. > > `stpcpy` was first standardized in POSIX.1-2008. > > Implement this so that we don't observe linkage failures due to missing > symbol definitions for `stpcpy`. > .. ... > diff --git a/include/linux/string.h b/include/linux/string.h > index b1f3894a0a3e..e570b9b10f50 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 > +extern char *stpcpy(char *__restrict, const char *__restrict__); > +#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 6012c385fb31..81bc4d62c256 100644 > --- a/lib/string.c > +++ b/lib/string.c > @@ -272,6 +272,29 @@ ssize_t strscpy_pad(char *dest, const char *src, size_t count) > } > EXPORT_SYMBOL(strscpy_pad); > > +#ifndef __HAVE_ARCH_STPCPY ... > +#undef stpcpy > +char *stpcpy(char *__restrict__ dest, const char *__restrict__ src) > +{ > + while ((*dest++ = *src++) != '\0') > + /* nothing */; > + return dest; > +} > +#endif > + Hmmmm.... Maybe the compiler should just inline the above? OTOH there are faster copies on 64bit systems (for moderate length strings). It would also be nicer if the compiler actually used/required a symbol in the 'reserved for the implementation' namespace. Then the linker should be able to do a fixup to a differently name symbol - if that is required. But compiler writers enjoy making embedded coders life hell. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 2020-08-15, 'Nick Desaulniers' via Clang Built Linux wrote: >On Sat, Aug 15, 2020 at 2:31 PM Joe Perches <joe@perches.com> wrote: >> >> On Sat, 2020-08-15 at 14:28 -0700, Nick Desaulniers wrote: >> > On Sat, Aug 15, 2020 at 2:24 PM Joe Perches <joe@perches.com> wrote: >> > > On Sat, 2020-08-15 at 13:47 -0700, Nick Desaulniers wrote: >> > > > On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote: >> > > > > On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote: >> > > > > > LLVM implemented a recent "libcall optimization" that lowers calls to >> > > > > > `sprintf(dest, "%s", str)` where the return value is used to >> > > > > > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved >> > > > > > in parsing format strings. Calling `sprintf` with overlapping arguments >> > > > > > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior. >> > > > > > >> > > > > > `stpcpy` is just like `strcpy` except it returns the pointer to the new >> > > > > > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in >> > > > > > one statement. >> > > > > >> > > > > O_O What? >> > > > > >> > > > > No; this is a _terrible_ API: there is no bounds checking, there are no >> > > > > buffer sizes. Anything using the example sprintf() pattern is _already_ >> > > > > wrong and must be removed from the kernel. (Yes, I realize that the >> > > > > kernel is *filled* with this bad assumption that "I'll never write more >> > > > > than PAGE_SIZE bytes to this buffer", but that's both theoretically >> > > > > wrong ("640k is enough for anybody") and has been known to be wrong in >> > > > > practice too (e.g. when suddenly your writing routine is reachable by >> > > > > splice(2) and you may not have a PAGE_SIZE buffer). >> > > > > >> > > > > But we cannot _add_ another dangerous string API. We're already in a >> > > > > terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This >> > > > > needs to be addressed up by removing the unbounded sprintf() uses. (And >> > > > > to do so without introducing bugs related to using snprintf() when >> > > > > scnprintf() is expected[4].) >> > > > >> > > > Well, everything (-next, mainline, stable) is broken right now (with >> > > > ToT Clang) without providing this symbol. I'm not going to go clean >> > > > the entire kernel's use of sprintf to get our CI back to being green. >> > > >> > > Maybe this should get place in compiler-clang.h so it isn't >> > > generic and public. >> > >> > https://bugs.llvm.org/show_bug.cgi?id=47162#c7 and >> > https://bugs.llvm.org/show_bug.cgi?id=47144 >> > Seem to imply that Clang is not the only compiler that can lower a >> > sequence of libcalls to stpcpy. Do we want to wait until we have a >> > fire drill w/ GCC to move such an implementation from >> > include/linux/compiler-clang.h back in to lib/string.c? >> >> My guess is yes, wait until gcc, if ever, needs it. > >The suggestion to use static inline doesn't even make sense. The >compiler is lowering calls to other library routines; `stpcpy` isn't >being explicitly called. Even if it was, not sure we want it being >inlined. No symbol definition will be emitted; problem not solved. >And I refuse to add any more code using `extern inline`. Putting the >definition in lib/string.c is the most straightforward and avoids >revisiting this issue in the future for other toolchains. I'll limit >access by removing the declaration, and adding a comment to avoid its >use. But if you're going to use a gnu target triple without using >-ffreestanding because you *want* libcall optimizations, then you have >to provide symbols for all possible library routines! Adding a definition without a declaration for stpcpy looks good. Clang LTO will work. (If the kernel does not want to provide these routines, is http://git.kernel.org/linus/6edfba1b33c701108717f4e036320fc39abe1912 probably wrong? (why remove -ffreestanding from the main Makefile) )
On Sun, Aug 16, 2020 at 2:19 AM 'Fangrui Song' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > > On 2020-08-15, 'Nick Desaulniers' via Clang Built Linux wrote: > >On Sat, Aug 15, 2020 at 2:31 PM Joe Perches <joe@perches.com> wrote: > >> > >> On Sat, 2020-08-15 at 14:28 -0700, Nick Desaulniers wrote: > >> > On Sat, Aug 15, 2020 at 2:24 PM Joe Perches <joe@perches.com> wrote: > >> > > On Sat, 2020-08-15 at 13:47 -0700, Nick Desaulniers wrote: > >> > > > On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote: > >> > > > > On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote: > >> > > > > > LLVM implemented a recent "libcall optimization" that lowers calls to > >> > > > > > `sprintf(dest, "%s", str)` where the return value is used to > >> > > > > > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved > >> > > > > > in parsing format strings. Calling `sprintf` with overlapping arguments > >> > > > > > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior. > >> > > > > > > >> > > > > > `stpcpy` is just like `strcpy` except it returns the pointer to the new > >> > > > > > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in > >> > > > > > one statement. > >> > > > > > >> > > > > O_O What? > >> > > > > > >> > > > > No; this is a _terrible_ API: there is no bounds checking, there are no > >> > > > > buffer sizes. Anything using the example sprintf() pattern is _already_ > >> > > > > wrong and must be removed from the kernel. (Yes, I realize that the > >> > > > > kernel is *filled* with this bad assumption that "I'll never write more > >> > > > > than PAGE_SIZE bytes to this buffer", but that's both theoretically > >> > > > > wrong ("640k is enough for anybody") and has been known to be wrong in > >> > > > > practice too (e.g. when suddenly your writing routine is reachable by > >> > > > > splice(2) and you may not have a PAGE_SIZE buffer). > >> > > > > > >> > > > > But we cannot _add_ another dangerous string API. We're already in a > >> > > > > terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This > >> > > > > needs to be addressed up by removing the unbounded sprintf() uses. (And > >> > > > > to do so without introducing bugs related to using snprintf() when > >> > > > > scnprintf() is expected[4].) > >> > > > > >> > > > Well, everything (-next, mainline, stable) is broken right now (with > >> > > > ToT Clang) without providing this symbol. I'm not going to go clean > >> > > > the entire kernel's use of sprintf to get our CI back to being green. > >> > > > >> > > Maybe this should get place in compiler-clang.h so it isn't > >> > > generic and public. > >> > > >> > https://bugs.llvm.org/show_bug.cgi?id=47162#c7 and > >> > https://bugs.llvm.org/show_bug.cgi?id=47144 > >> > Seem to imply that Clang is not the only compiler that can lower a > >> > sequence of libcalls to stpcpy. Do we want to wait until we have a > >> > fire drill w/ GCC to move such an implementation from > >> > include/linux/compiler-clang.h back in to lib/string.c? > >> > >> My guess is yes, wait until gcc, if ever, needs it. > > > >The suggestion to use static inline doesn't even make sense. The > >compiler is lowering calls to other library routines; `stpcpy` isn't > >being explicitly called. Even if it was, not sure we want it being > >inlined. No symbol definition will be emitted; problem not solved. > >And I refuse to add any more code using `extern inline`. Putting the > >definition in lib/string.c is the most straightforward and avoids > >revisiting this issue in the future for other toolchains. I'll limit > >access by removing the declaration, and adding a comment to avoid its > >use. But if you're going to use a gnu target triple without using > >-ffreestanding because you *want* libcall optimizations, then you have > >to provide symbols for all possible library routines! > > Adding a definition without a declaration for stpcpy looks good. > Clang LTO will work. > > (If the kernel does not want to provide these routines, > is http://git.kernel.org/linus/6edfba1b33c701108717f4e036320fc39abe1912 > probably wrong? (why remove -ffreestanding from the main Makefile) ) > We had some many issues in arch/x86 where *FLAGS were removed or used differently and had to re-add them :-(. So if -ffreestanding is used in arch/x86 and was! used in top-level Makefile - it makes sense to re-add it back? ( I cannot speak for archs other than x86. ) - Sedat -
On Sun, Aug 16, 2020 at 07:22:35AM +0200, Sedat Dilek wrote: > On Sun, Aug 16, 2020 at 2:19 AM 'Fangrui Song' via Clang Built Linux > <clang-built-linux@googlegroups.com> wrote: > > > > Adding a definition without a declaration for stpcpy looks good. > > Clang LTO will work. > > > > (If the kernel does not want to provide these routines, > > is http://git.kernel.org/linus/6edfba1b33c701108717f4e036320fc39abe1912 > > probably wrong? (why remove -ffreestanding from the main Makefile) ) > > > > We had some many issues in arch/x86 where *FLAGS were removed or used > differently and had to re-add them :-(. > > So if -ffreestanding is used in arch/x86 and was! used in top-level > Makefile - it makes sense to re-add it back? > ( I cannot speak for archs other than x86. ) > > - Sedat - -ffreestanding disables _all_ builtins and libcall optimizations, which is probably not desirable. If we added it back, we'd need to also go back to #define various string functions to the __builtin versions. Though I don't understand the original issue, with -ffreestanding, sprintf shouldn't have been turned into strcpy in the first place. 32-bit still has -ffreestanding -- I wonder if that's actually necessary any more? Why does -fno-builtin-stpcpy not work with clang LTO? Isn't that a compiler bug? Thanks.
On Sun, Aug 16, 2020 at 8:02 AM Arvind Sankar <nivedita@alum.mit.edu> wrote: > > On Sun, Aug 16, 2020 at 07:22:35AM +0200, Sedat Dilek wrote: > > On Sun, Aug 16, 2020 at 2:19 AM 'Fangrui Song' via Clang Built Linux > > <clang-built-linux@googlegroups.com> wrote: > > > > > > Adding a definition without a declaration for stpcpy looks good. > > > Clang LTO will work. > > > > > > (If the kernel does not want to provide these routines, > > > is http://git.kernel.org/linus/6edfba1b33c701108717f4e036320fc39abe1912 > > > probably wrong? (why remove -ffreestanding from the main Makefile) ) > > > > > > > We had some many issues in arch/x86 where *FLAGS were removed or used > > differently and had to re-add them :-(. > > > > So if -ffreestanding is used in arch/x86 and was! used in top-level > > Makefile - it makes sense to re-add it back? > > ( I cannot speak for archs other than x86. ) > > > > - Sedat - > > -ffreestanding disables _all_ builtins and libcall optimizations, which > is probably not desirable. If we added it back, we'd need to also go > back to #define various string functions to the __builtin versions. > > Though I don't understand the original issue, with -ffreestanding, > sprintf shouldn't have been turned into strcpy in the first place. > > 32-bit still has -ffreestanding -- I wonder if that's actually necessary > any more? > > Why does -fno-builtin-stpcpy not work with clang LTO? Isn't that a > compiler bug? I just confirmed that adding -fno-builtin-stpcpy to KBUILD_CFLAGS does work with LTO as well. Sami
On Mon, Aug 17, 2020 at 10:14 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > On Sun, Aug 16, 2020 at 8:02 AM Arvind Sankar <nivedita@alum.mit.edu> wrote: > > > > On Sun, Aug 16, 2020 at 07:22:35AM +0200, Sedat Dilek wrote: > > > On Sun, Aug 16, 2020 at 2:19 AM 'Fangrui Song' via Clang Built Linux > > > <clang-built-linux@googlegroups.com> wrote: > > > > > > > > Adding a definition without a declaration for stpcpy looks good. > > > > Clang LTO will work. > > > > > > > > (If the kernel does not want to provide these routines, > > > > is http://git.kernel.org/linus/6edfba1b33c701108717f4e036320fc39abe1912 > > > > probably wrong? (why remove -ffreestanding from the main Makefile) ) > > > > > > > > > > We had some many issues in arch/x86 where *FLAGS were removed or used > > > differently and had to re-add them :-(. > > > > > > So if -ffreestanding is used in arch/x86 and was! used in top-level > > > Makefile - it makes sense to re-add it back? > > > ( I cannot speak for archs other than x86. ) > > > > > > - Sedat - > > > > -ffreestanding disables _all_ builtins and libcall optimizations, which > > is probably not desirable. If we added it back, we'd need to also go I agree. > > back to #define various string functions to the __builtin versions. > > > > Though I don't understand the original issue, with -ffreestanding, > > sprintf shouldn't have been turned into strcpy in the first place. Huh? The original issue for this thread is because `-ffreestanding` *isn't* being used for most targets (oh boy, actually mixed usage by ARCH. Looks like MIPS, m68k, superH, xtensa, and 32b x86 use it?); and I'm not suggesting it be used. > > 32-bit still has -ffreestanding -- I wonder if that's actually necessary > > any more? Fair question. Someone will have to go chase git history, since 0a6ef376d4ba covers it up. If anyone has any tricks to do so quickly; I'd love to know. I generally checkout the commit prior, then use vim fugitive to get git blame. > > Why does -fno-builtin-stpcpy not work with clang LTO? Isn't that a > > compiler bug? Yes; Sami found a recent patch that looks to me like it may have recently solved that bug. https://reviews.llvm.org/D71193 which landed Dec 12 2019. The bug report was based on https://github.com/ClangBuiltLinux/linux/issues/416#issuecomment-472231304 (Issue reported March 8 2019). And I do recall being able to reproduce the bug when I sent commit 5f074f3e192f ("lib/string.c: implement a basic bcmp") Now that that is fixed as reported by Sami below, I don't mind sending a revert for 5f074f3e192f that adds -fno-builtin-bcmp, because the current implementation of bcmp isn't useful. That said, this libcall optimization/transformation (sprintf->stpcpy) does look useful to me. Kees, do you have thoughts on me providing the implementation without exposing it in a header vs using -fno-builtin-stpcpy? (I would need to add the missing EXPORT_SYMBOL, as pointed out by 0day bot and on the github thread). I don't care either way; I'd just like your input before sending a V+1. Maybe better to just not implement it and never implement it? > > I just confirmed that adding -fno-builtin-stpcpy to KBUILD_CFLAGS does > work with LTO as well. > > Sami -- Thanks, ~Nick Desaulniers
On Mon, Aug 17, 2020 at 11:36:49AM -0700, Nick Desaulniers wrote: > That said, this libcall optimization/transformation (sprintf->stpcpy) > does look useful to me. Kees, do you have thoughts on me providing > the implementation without exposing it in a header vs using > -fno-builtin-stpcpy? (I would need to add the missing EXPORT_SYMBOL, > as pointed out by 0day bot and on the github thread). I don't care > either way; I'd just like your input before sending a V+1. Maybe > better to just not implement it and never implement it? I think I would ultimately prefer -fno-builtin-stpcpy, but for now, sure, an implementation without a header (and a biiig comment above it detailing why and a reference to the bug) would be fine by me. -- Kees Cook
On Mon, Aug 17, 2020 at 10:14:43AM -0700, Sami Tolvanen wrote: > I just confirmed that adding -fno-builtin-stpcpy to KBUILD_CFLAGS does > work with LTO as well. Oh, I read this out of order; sorry! Yes, if -fno-builtin-stpcpy works, let's use that instead. Doesn't that solve it too? -- Kees Cook
On Mon, Aug 17, 2020 at 11:36:49AM -0700, Nick Desaulniers wrote: > > > Though I don't understand the original issue, with -ffreestanding, > > > sprintf shouldn't have been turned into strcpy in the first place. > > Huh? The original issue for this thread is because `-ffreestanding` > *isn't* being used for most targets (oh boy, actually mixed usage by > ARCH. Looks like MIPS, m68k, superH, xtensa, and 32b x86 use it?); and > I'm not suggesting it be used. > Sorry, I meant the issue mentioned in the commit that removed -ffreestanding, not the stpcpy one you're solving now. It says that sprintf got converted into strcpy, which caused failures because back then, strcpy was #define'd to __builtin_strcpy, and the default implementation was actually of a function called __builtin_strcpy o_O, not strcpy. Anyway, that's water under the bridge now. 6edfba1b33c7 ("x86_64: Don't define string functions to builtin") gcc should handle this anyways, and it causes problems when sprintf is turned into strcpy by gcc behind our backs and the C fallback version of strcpy is actually defining __builtin_strcpy
On Mon, Aug 17, 2020 at 1:13 PM Arvind Sankar <nivedita@alum.mit.edu> wrote: > > On Mon, Aug 17, 2020 at 11:36:49AM -0700, Nick Desaulniers wrote: > > > > Though I don't understand the original issue, with -ffreestanding, > > > > sprintf shouldn't have been turned into strcpy in the first place. > > > > Huh? The original issue for this thread is because `-ffreestanding` > > *isn't* being used for most targets (oh boy, actually mixed usage by > > ARCH. Looks like MIPS, m68k, superH, xtensa, and 32b x86 use it?); and > > I'm not suggesting it be used. > > > > Sorry, I meant the issue mentioned in the commit that removed > -ffreestanding, not the stpcpy one you're solving now. It says that > sprintf got converted into strcpy, which caused failures because back > then, strcpy was #define'd to __builtin_strcpy, and the default > implementation was actually of a function called __builtin_strcpy o_O, > not strcpy. > > Anyway, that's water under the bridge now. > > 6edfba1b33c7 ("x86_64: Don't define string functions to builtin") > gcc should handle this anyways, and it causes problems when > sprintf is turned into strcpy by gcc behind our backs and > the C fallback version of strcpy is actually defining __builtin_strcpy For fun, I tried removing `-ffreestanding` from arch/x86/Makefile; both gcc and clang can compile+boot the i386 defconfig just fine. Why don't I send a patch removing it with your suggested by in a series of fixes for stpcpy and bcmp? -- Thanks, ~Nick Desaulniers
On Tue, 2020-08-18 at 08:24 +0000, David Laight wrote: > From: Nick Desaulniers > > Sent: 17 August 2020 19:37 > .. > > That said, this libcall optimization/transformation (sprintf->stpcpy) > > does look useful to me. > > I'd rather get a cow if I ask for a cow... > > Maybe checkpatch (etc) could report places where snprintf() > could be replaced by a simpler function. You mean sprintf no? Reminds me of seq_printf vs seq_puts... https://lkml.org/lkml/2013/3/16/79
diff --git a/include/linux/string.h b/include/linux/string.h index b1f3894a0a3e..e570b9b10f50 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 +extern char *stpcpy(char *__restrict, const char *__restrict__); +#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 6012c385fb31..81bc4d62c256 100644 --- a/lib/string.c +++ b/lib/string.c @@ -272,6 +272,29 @@ ssize_t strscpy_pad(char *dest, const char *src, size_t count) } EXPORT_SYMBOL(strscpy_pad); +#ifndef __HAVE_ARCH_STPCPY +/** + * stpcpy - copy a string from src to dest returning a pointer to the new end + * of dest, including src's NULL terminator. May overrun dest. + * @dest: pointer to end of string being copied into. Must be large enough + * to receive copy. + * @src: pointer to the beginning of string being copied from. Must not overlap + * dest. + * + * stpcpy differs from strcpy in two key ways: + * 1. inputs must not overlap. + * 2. return value is the new NULL terminated character. (for strcpy, the + * return value is a pointer to src. + */ +#undef stpcpy +char *stpcpy(char *__restrict__ dest, const char *__restrict__ src) +{ + while ((*dest++ = *src++) != '\0') + /* nothing */; + return dest; +} +#endif + #ifndef __HAVE_ARCH_STRCAT /** * strcat - Append one %NUL-terminated string to another
LLVM implemented a recent "libcall optimization" that lowers calls to `sprintf(dest, "%s", str)` where the return value is used to `stpcpy(dest, str) - dest`. This generally avoids the machinery involved in parsing format strings. `stpcpy` is just like `strcpy` except: 1. it returns the pointer to the new tail of `dest`. This allows you to chain multiple calls to `stpcpy` in one statement. 2. it requires the parameters not to overlap. Calling `sprintf` with overlapping arguments was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior. `stpcpy` was first standardized in POSIX.1-2008. Implement this so that we don't observe linkage failures due to missing symbol definitions for `stpcpy`. Similar to last year's fire drill with: commit 5f074f3e192f ("lib/string.c: implement a basic bcmp") This optimization was introduced into clang-12. Cc: stable@vger.kernel.org Link: https://bugs.llvm.org/show_bug.cgi?id=47162 Link: https://github.com/ClangBuiltLinux/linux/issues/1126 Link: https://man7.org/linux/man-pages/man3/stpcpy.3.html Link: https://pubs.opengroup.org/onlinepubs/9699919799/functions/stpcpy.html Link: https://reviews.llvm.org/D85963 Reported-by: Sami Tolvanen <samitolvanen@google.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- include/linux/string.h | 3 +++ lib/string.c | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+) -- 2.28.0.220.ged08abb693-goog