Re: [PATCH] string: Improve the generic strlcpy() implementation

[Ingo Molnar <mingo@kernel.org>]

* 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