linux-kernel - Re: [PATCH v2] lib/string.c: implement stpcpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 14 Aug 2020 22:58:16 -0400
From:   Arvind Sankar <nivedita@...m.mit.edu>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Dávid Bolvanský <david.bolvansky@...il.com>,
        Eli Friedman <efriedma@...cinc.com>, stable@...r.kernel.org,
        Arvind Sankar <nivedita@...m.mit.edu>,
        Joe Perches <joe@...ches.com>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "Joel Fernandes (Google)" <joel@...lfernandes.org>,
        Daniel Axtens <dja@...ens.net>,
        Kees Cook <keescook@...omium.org>,
        Ingo Molnar <mingo@...nel.org>,
        Yury Norov <yury.norov@...il.com>,
        Alexandru Ardelean <alexandru.ardelean@...log.com>,
        linux-kernel@...r.kernel.org, clang-built-linux@...glegroups.com
Subject: Re: [PATCH v2] lib/string.c: implement stpcpy

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.
> 
> `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@...r.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
> Suggested-by: Arvind Sankar <nivedita@...m.mit.edu>
> Suggested-by: Joe Perches <joe@...ches.com>
> Reported-by: Sami Tolvanen <samitolvanen@...gle.com>
> Tested-by: Sami Tolvanen <samitolvanen@...gle.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@...gle.com>
> ---
> Changes V2:
> * Added Sami's Tested by; though the patch changed implementation, the
>   missing symbol at link time was the problem Sami was observing.
> * Fix __restrict -> __restrict__ typo as per Joe.
> * Drop note about restrict from commit message as per Arvind.
> * Fix NULL -> NUL as per Arvind; NUL is ASCII '\0'. TIL
> * Fix off by one error as per Arvind; I had another off by one error in
>   my test program that was masking this.
> 
>  include/linux/string.h |  3 +++
>  lib/string.c           | 23 +++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b1f3894a0a3e..7686dbca8582 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..68ddbffbbd58 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 NUL 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.

Looks like you missed my second email: strcpy also does not allow inputs
to overlap. Couple typos below.

> + * 2. return value is the new NULL terminated character. (for strcpy, the
				 ^^ NUL terminator.
> + *    return value is a pointer to src.
				      ^^ dest.)
> + */
> +#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
> -- 
> 2.28.0.220.ged08abb693-goog
> 

The kernel-doc comments in string.c currently have a mix of %NUL and
NUL, but the former seems to be more common. %NUL-terminator appears to
be the preferred wording.