Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1239554
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH] string: Improve the generic strlcpy() implementation |
| Date | 2015-10-05 15:20 +0200 |
| Message-ID | <qgdQn-8oB-37@gated-at.bofh.it> (permalink) |
| References | <q7g13-2Rm-7@gated-at.bofh.it> <qfTRD-4Np-7@gated-at.bofh.it> <qgc7U-5TG-9@gated-at.bofh.it> <qgd3Y-7eX-27@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
Re: [GIT PULL] strscpy string copy function Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-04 18:00 +0200
[PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 13:30 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 14:00 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 15:20 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 16:10 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 16:10 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 16:40 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-05 17:40 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 18:10 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-05 14:30 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 15:20 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-06 00:30 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-06 10:00 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-06 10:10 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-07 00:10 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-07 09:20 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-07 11:10 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-07 11:30 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-08 10:50 +0200
Re: [PATCH] string: Improve the generic strlcpy() implementation Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-09 10:20 +0200
[RFC 0/3] eliminate potential race in string() (was: [PATCH] string: Improve the generic strlcpy() implementation) Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-09 11:20 +0200
[RFC 1/3] lib/vsprintf.c: pull out padding code from dentry_name() Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-09 11:20 +0200
[RFC 3/3] lib/vsprintf.c: eliminate potential race in string() Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-09 11:20 +0200
[RFC 2/3] lib/vsprintf.c: move string() below widen_string() Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-09 11:20 +0200
Re: [RFC 0/3] eliminate potential race in string() (was: [PATCH] string: Improve the generic strlcpy() implementation) Ingo Molnar <mingo@kernel.org> - 2015-10-10 09:50 +0200
csiph-web