lkml.org 
[lkml]   [2015]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patches in this message
    /
    Date
    From
    SubjectRe: [PATCH] string: Improve the generic strlcpy() implementation

    * Linus Torvalds <torvalds@linux-foundation.org> wrote:

    > On Mon, Oct 5, 2015 at 12:27 PM, Ingo Molnar <mingo@kernel.org> wrote:
    > >
    > > Interesting. I noticed that strscpy() says this in its comments:
    > >
    > > * In addition, the implementation is robust to the string changing out
    > > * from underneath it, unlike the current strlcpy() implementation.
    > >
    > > The strscpy() interface is very nice, but shouldn't we also fix this strlcpy()
    > > unrobustness/race it refers to, in light of the 2000+ existing strlcpy() call
    > > sites?
    >
    > Well, I'm not sure the race really matters. [...]

    Yeah, so if we do the word-by-word optimization for strlcpy() then the race is
    fixed 'automatically', for free.

    But you are right:

    > [...] I personally think strlcpy() is a horrible interface, and the thing is,
    > the return value of strlcpy (which is what can race) is kind of useless because
    > it's not actually the size of the resulting string *anyway* (because of the
    > overflow issue).
    >
    > So I'm not sure it's worth even fixing.

    > Also, if you do this, then you're better off using the (hopefully optimized)
    > "strlen()" for the tail part of the strlcpy destination for the overflow case
    > that didn't get copied.

    Indeed, this on top of my patch should do that:

    lib/string.c | 5 +----
    1 file changed, 1 insertion(+), 4 deletions(-)

    diff --git a/lib/string.c b/lib/string.c
    index e0cfca299606..dfd24b557e84 100644
    --- a/lib/string.c
    +++ b/lib/string.c
    @@ -212,10 +212,7 @@ size_t strlcpy(char *dest, const char *src, size_t dest_size)
    if (dest_size)
    dest[dest_size-1] = '\0';

    - while (src[src_len])
    - src_len++;
    -
    - return src_len;
    + return strlen(src) + src_len;
    }
    EXPORT_SYMBOL(strlcpy);
    #endif
    (untested)

    > In other words, I think your patch is overly fragile and complex.

    Well, it's mostly a copy of strscpy() with obvious conversion of the return
    convention, but you are right to point out:

    > Instead, you might choose to implement strlcpy() in terms of
    > "strscpy()" and "strlen()".
    >
    > Something like
    >
    > int strlcpy(dst, src, len)
    > {
    > // do the actual copy
    > int n = strscpy(dst, src, len);
    >
    > // handle the insane and broken strlcpy overflow return value
    > if (n < 0)
    > return len + strlen(src+len);
    >
    > return n;
    > }
    >
    > but I didn't actually verify that the above is correct for all the corner case.

    Hm, so I considered doing that initially, but managed to convince myself that it's
    not equivalent: but on a second thought your variant should indeed work!

    > The point being, that you really shouldn't waste your time implementing the
    > broken BSD strlcpy crap as an actual first-class implementation. You're better
    > off just using a strscpy() as the primary engine, and then implementing the
    > broken strlcpy interfaces on top of it.
    >
    > Does the above work? I'd take a patch that implements that if it's tested and
    > somebody has thought about it a lot. But I don't like your patch that open-codes
    > the insane interface with complex and fragile code.

    So the below untested variant does that plus an overflow check - it's only FYI,
    not signed off yet.

    Thanks,

    Ingo

    ==============>
    Not-Signed-off-by: Ingo Molnar <mingo@kernel.org>

    lib/string.c | 60 ++++++++++++++++++++++++++++++++++--------------------------
    1 file changed, 34 insertions(+), 26 deletions(-)
    diff --git a/lib/string.c b/lib/string.c
    index 8dbb7b1eab50..6b89c035df74 100644
    --- a/lib/string.c
    +++ b/lib/string.c
    @@ -124,32 +124,6 @@ char *strncpy(char *dest, const char *src, size_t count)
    EXPORT_SYMBOL(strncpy);
    #endif

    -#ifndef __HAVE_ARCH_STRLCPY
    -/**
    - * strlcpy - Copy a C-string into a sized buffer
    - * @dest: Where to copy the string to
    - * @src: Where to copy the string from
    - * @size: size of destination buffer
    - *
    - * Compatible with *BSD: the result is always a valid
    - * NUL-terminated string that fits in the buffer (unless,
    - * of course, the buffer size is zero). It does not pad
    - * out the result like strncpy() does.
    - */
    -size_t strlcpy(char *dest, const char *src, size_t size)
    -{
    - size_t ret = strlen(src);
    -
    - if (size) {
    - size_t len = (ret >= size) ? size - 1 : ret;
    - memcpy(dest, src, len);
    - dest[len] = '\0';
    - }
    - return ret;
    -}
    -EXPORT_SYMBOL(strlcpy);
    -#endif
    -
    #ifndef __HAVE_ARCH_STRSCPY
    /**
    * strscpy - Copy a C-string into a sized buffer
    @@ -234,6 +208,40 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
    EXPORT_SYMBOL(strscpy);
    #endif

    +#ifndef __HAVE_ARCH_STRLCPY
    +/**
    + * strlcpy - Copy a C-string into a sized buffer
    + * @dst: Where to copy the string to
    + * @src: Where to copy the string from
    + * @dst_size: size of destination buffer
    + *
    + * Compatible with *BSD: the result is always a valid
    + * NUL-terminated string that fits in the buffer (unless,
    + * of course, the buffer size is zero). It does not pad
    + * out the result like strncpy() does.
    + */
    +size_t strlcpy(char *dst, const char *src, size_t dst_size)
    +{
    + int ret;
    +
    + /* Overflow check: */
    + if (unlikely((ssize_t)dst_size < 0)) {
    + WARN_ONCE(1, "strlcpy(): dst_size < 0 underflow!");
    + return strlen(src);
    + }
    +
    + ret = strscpy(dst, src, dst_size);
    +
    + /* Handle the insane and broken strlcpy() overflow return value: */
    + if (ret < 0)
    + return dst_size + strlen(src+dst_size);
    +
    + return ret;
    +}
    +EXPORT_SYMBOL(strlcpy);
    +#endif
    +
    +
    #ifndef __HAVE_ARCH_STRCAT
    /**
    * strcat - Append one %NUL-terminated string to another


    \
     
     \ /
      Last update: 2015-10-05 15:21    [W:2.530 / U:0.668 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site