Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[René Scharfe]

> How about just adding a sane
> 
> 	int copy_string(char *dest, const char *src, int len)
> 	{
> 		int size;
> 
> 		if (!len)
> 			return 0;
> 		size = strlen(src);
> 		if (size >= len)
> 			size = len-1;
> 		memcpy(dest, src, size);
> 		dest[size] = '\0';
> 		return size;
> 	}
> 
> which is what pretty much everybody really _wants_ to have anyway? We 
> should deprecate "strncpy()" within the kernel entirely.

This looks suspiciously like strlcpy() from the *BSDs. Why not name it
so?

The following patch is based on Samba's implementation (found in
source/lib/replace.c). I just reformatted it somewhat, there are no
functional changes. What do you think?

René



diff -ur linux-a/kernel/ksyms.c linux-b/kernel/ksyms.c
--- linux-a/kernel/ksyms.c	2003-05-05 01:52:49.000000000 +0200
+++ linux-b/kernel/ksyms.c	2003-05-25 11:02:22.000000000 +0200
@@ -588,6 +588,7 @@
 EXPORT_SYMBOL(strnicmp);
 EXPORT_SYMBOL(strspn);
 EXPORT_SYMBOL(strsep);
+EXPORT_SYMBOL(strlcpy);
 
 /* software interrupts */
 EXPORT_SYMBOL(tasklet_init);
diff -ur linux-a/lib/string.c linux-b/lib/string.c
--- linux-a/lib/string.c	2003-05-05 01:53:40.000000000 +0200
+++ linux-b/lib/string.c	2003-05-25 11:12:58.000000000 +0200
@@ -527,3 +527,28 @@
 }
 
 #endif
+
+#ifndef __HAVE_ARCH_STRLCPY
+/**
+ * strlcpy - Copy a length-limited, %NUL-terminated string
+ * @dest: Where to copy the string to
+ * @src: Where to copy the string from
+ * @bufsize: Size of the destination buffer
+ *
+ * Returns the length of @src, or 0 if @bufsize is 0. Unlike strncpy(),
+ * strlcpy() always NUL-terminates @dest and does no padding.
+ */
+size_t strlcpy(char *dest, const char *src, size_t bufsize)
+{
+	size_t len = strlen(src);
+	size_t ret = len;
+
+	if (bufsize == 0)
+		return 0;
+	if (len >= bufsize)
+		len = bufsize-1;
+	memcpy(dest, src, len);
+	dest[len] = '\0';   
+	return ret;
+}
+#endif

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Christoph Hellwig]

On Sun, May 25, 2003 at 11:21:50AM +0200, René Scharfe wrote:
> This looks suspiciously like strlcpy() from the *BSDs. Why not name it
> so?
> 
> The following patch is based on Samba's implementation (found in
> source/lib/replace.c). I just reformatted it somewhat, there are no
> functional changes. What do you think?

Looks good, you probably need a prototype in include/linux/string.h and
a samba copyright boilerplate in lib/string.c.

What about strlcat?

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Edgar Toernig]

René Scharfe wrote:
> +size_t strlcpy(char *dest, const char *src, size_t bufsize)
> +{
> +       size_t len = strlen(src);
> +       size_t ret = len;
> +
> +       if (bufsize == 0)
> +               return 0;

                  return ret; ???

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Ben Collins]

On Sun, May 25, 2003 at 09:05:09PM +0200, Ren? Scharfe wrote:
> On Sun, 25 May 2003 19:24:40 +0200 Edgar Toernig <froese@gmx.de> wrote:
> > Ren? Scharfe wrote:
> > > +       if (bufsize == 0)
> > > +               return 0;
> > 
> >                   return ret; ???
> 
> Yes, Samba's and the BSDs' strlcpy() deviate in that point. It's a very
> unusual case to have a zero-sized buffer, though, so probably it doesn't
> matter much.
> 
> Anyway, I corrected this. Patch below contains a "BSD-compatible" version,
> and also a strlcat().
> 
> Ben, I think this one is better than your's because it's shorter and
> already GPL'd (there's not more license than C code :). Linus?

> +size_t strlcat(char *dest, const char *src, size_t bufsize)
> +{
> +	size_t len1 = strlen(dest);
> +	size_t len2 = strlen(src);
> +	size_t ret = len1 + len2;
> +
> +	if (len1+len2 >= bufsize)
> +		len2 = bufsize - (len1+1);
> +	if (len2 > 0) {
> +		memcpy(dest+len1, src, len2);
> +		dest[len1+len2] = '\0';
> +	}
> +	return ret;
> +}
> +#endif

Your strlcat doesn't take into consideration that a zero bufsize could
mean that dest is not NUL-terminated, in which case strlen(dest) could
blow up in your face. Since strlcpy can handle zero bufsize, strlcat
should be able to handle being called just after strlcpy being called
with zero bufsize (see my patch).

I personally am not concerned either way, but it is definitely worth
noting.

-- 
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
Deqo       - http://www.deqo.com/

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 324 bytes --]

On Sun, 25 May 2003 21:05:09 +0200, =?ISO-8859-1?Q?Ren=E9?= Scharfe said:

> +size_t strlcpy(char *dest, const char *src, size_t bufsize)
> +{
> +	size_t len = strlen(src);
> +	size_t ret = len;
> +
> +	if (bufsize > 0)
> +		return ret;

Umm... Rene?  Either you or I need more caffeine, this looks b0rked to me?

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[René Scharfe]

On Sun, 25 May 2003 19:24:40 +0200 Edgar Toernig <froese@gmx.de> wrote:
> René Scharfe wrote:
> > +       if (bufsize == 0)
> > +               return 0;
> 
>                   return ret; ???

Yes, Samba's and the BSDs' strlcpy() deviate in that point. It's a very
unusual case to have a zero-sized buffer, though, so probably it doesn't
matter much.

Anyway, I corrected this. Patch below contains a "BSD-compatible" version,
and also a strlcat().

Ben, I think this one is better than your's because it's shorter and
already GPL'd (there's not more license than C code :). Linus?

René



diff -ur linux-a/include/linux/string.h linux-b/include/linux/string.h
--- linux-a/include/linux/string.h	2003-05-05 01:53:13.000000000 +0200
+++ linux-b/include/linux/string.h	2003-05-25 19:25:01.000000000 +0200
@@ -77,6 +77,12 @@
 #ifndef __HAVE_ARCH_MEMCHR
 extern void * memchr(const void *,int,__kernel_size_t);
 #endif
+#ifndef __HAVE_ARCH_STRLCPY
+__kernel_size_t strlcpy(char *, const char *, __kernel_size_t);
+#endif
+#ifndef __HAVE_ARCH_STRLCAT
+__kernel_size_t strlcat(char *, const char *, __kernel_size_t);
+#endif
 
 #ifdef __cplusplus
 }
diff -ur linux-a/kernel/ksyms.c linux-b/kernel/ksyms.c
--- linux-a/kernel/ksyms.c	2003-05-05 01:52:49.000000000 +0200
+++ linux-b/kernel/ksyms.c	2003-05-25 19:25:21.000000000 +0200
@@ -588,6 +588,8 @@
 EXPORT_SYMBOL(strnicmp);
 EXPORT_SYMBOL(strspn);
 EXPORT_SYMBOL(strsep);
+EXPORT_SYMBOL(strlcpy);
+EXPORT_SYMBOL(strlcat);
 
 /* software interrupts */
 EXPORT_SYMBOL(tasklet_init);
diff -ur linux-a/lib/string.c linux-b/lib/string.c
--- linux-a/lib/string.c	2003-05-05 01:53:40.000000000 +0200
+++ linux-b/lib/string.c	2003-05-25 20:54:34.000000000 +0200
@@ -5,6 +5,11 @@
  */
 
 /*
+ * The implementations of strlcpy() and strlcat() are taken from Samba, and
+ * Copyright (C) 2001 Andrew Tridgell.
+ */
+
+/*
  * stupid library routines.. The optimized versions should generally be found
  * as inline code in <asm-xx/string.h>
  *
@@ -527,3 +532,54 @@
 }
 
 #endif
+
+#ifndef __HAVE_ARCH_STRLCPY
+/**
+ * strlcpy - Copy a length-limited, %NUL-terminated string
+ * @dest: Where to copy the string to
+ * @src: Where to copy the string from
+ * @bufsize: Size of the destination buffer
+ *
+ * Returns the length of @src. Unlike strncpy(), strlcpy() always
+ * %NUL-terminates @dest (unless @bufsize is 0) and does no padding.
+ */
+size_t strlcpy(char *dest, const char *src, size_t bufsize)
+{
+	size_t len = strlen(src);
+	size_t ret = len;
+
+	if (bufsize > 0)
+		return ret;
+	if (len >= bufsize)
+		len = bufsize-1;
+	memcpy(dest, src, len);
+	dest[len] = '\0';   
+	return ret;
+}
+#endif
+
+#ifndef __HAVE_ARCH_STRLCAT
+/**
+ * strlcat - Append a length-limited, %NUL-terminated string to another
+ * @dest: The string to be appended to
+ * @src: The string to append to it
+ * @bufsize: Size of the destination buffer
+ *
+ * Returns the sum of the initial lengths of @src and @dest. The resulting
+ * string is always %NUL-terminated (unless @bufsize is 0).
+ */
+size_t strlcat(char *dest, const char *src, size_t bufsize)
+{
+	size_t len1 = strlen(dest);
+	size_t len2 = strlen(src);
+	size_t ret = len1 + len2;
+
+	if (len1+len2 >= bufsize)
+		len2 = bufsize - (len1+1);
+	if (len2 > 0) {
+		memcpy(dest+len1, src, len2);
+		dest[len1+len2] = '\0';
+	}
+	return ret;
+}
+#endif

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[René Scharfe]

On Sun, 25 May 2003 15:01:20 -0400 Valdis.Kletnieks@vt.edu wrote:
> On Sun, 25 May 2003 21:05:09 +0200, =?ISO-8859-1?Q?Ren=E9?= Scharfe said:
> > +	if (bufsize > 0)
> > +		return ret;
> 
> Umm... Rene?  Either you or I need more caffeine, this looks b0rked to me?

Mmpf. Thanks for noting. I'll be back in a minute with a fresh cup of tea..

René

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[René Scharfe]

On Sun, 25 May 2003 14:16:22 -0400 Ben Collins <bcollins@debian.org> wrote:
> Your strlcat doesn't take into consideration that a zero bufsize could
> mean that dest is not NUL-terminated, in which case strlen(dest) could
> blow up in your face.

You are right. How about that one?

René



diff -ur linux-a/include/linux/string.h linux-b/include/linux/string.h
--- linux-a/include/linux/string.h	2003-05-05 01:53:13.000000000 +0200
+++ linux-b/include/linux/string.h	2003-05-25 19:25:01.000000000 +0200
@@ -77,6 +77,12 @@
 #ifndef __HAVE_ARCH_MEMCHR
 extern void * memchr(const void *,int,__kernel_size_t);
 #endif
+#ifndef __HAVE_ARCH_STRLCPY
+__kernel_size_t strlcpy(char *, const char *, __kernel_size_t);
+#endif
+#ifndef __HAVE_ARCH_STRLCAT
+__kernel_size_t strlcat(char *, const char *, __kernel_size_t);
+#endif
 
 #ifdef __cplusplus
 }
diff -ur linux-a/kernel/ksyms.c linux-b/kernel/ksyms.c
--- linux-a/kernel/ksyms.c	2003-05-05 01:52:49.000000000 +0200
+++ linux-b/kernel/ksyms.c	2003-05-25 19:25:21.000000000 +0200
@@ -588,6 +588,8 @@
 EXPORT_SYMBOL(strnicmp);
 EXPORT_SYMBOL(strspn);
 EXPORT_SYMBOL(strsep);
+EXPORT_SYMBOL(strlcpy);
+EXPORT_SYMBOL(strlcat);
 
 /* software interrupts */
 EXPORT_SYMBOL(tasklet_init);
diff -ur linux-a/lib/string.c linux-b/lib/string.c
--- linux-a/lib/string.c	2003-05-05 01:53:40.000000000 +0200
+++ linux-b/lib/string.c	2003-05-25 22:04:06.000000000 +0200
@@ -5,6 +5,11 @@
  */
 
 /*
+ * The implementations of strlcpy() and strlcat() are taken from Samba, and
+ * Copyright (C) Andrew Tridgell 2001.
+ */
+
+/*
  * stupid library routines.. The optimized versions should generally be found
  * as inline code in <asm-xx/string.h>
  *
@@ -17,6 +22,11 @@
  * * Sat Feb 09 2002, Jason Thomas <jason@topic.com.au>,
  *                    Matthew Hawkins <matt@mh.dropbear.id.au>
  * -  Kissed strtok() goodbye
+ *
+ * * Sun May 25 2003, René Scharfe <l.s.r@web.de>
+ * - Imported strlcpy() and strlcat() from Samba.
+ * - Fixed strlcpy() return value in case of a zero-sized buffer.
+ * - Made strlcat() able to cope with a zero-sized buffer.
  */
  
 #include <linux/types.h>
@@ -527,3 +537,56 @@
 }
 
 #endif
+
+#ifndef __HAVE_ARCH_STRLCPY
+/**
+ * strlcpy - Copy a length-limited, %NUL-terminated string
+ * @dest: Where to copy the string to
+ * @src: Where to copy the string from
+ * @bufsize: Size of the destination buffer
+ *
+ * Returns the length of @src. Unlike strncpy(), strlcpy() always
+ * %NUL-terminates @dest (unless @bufsize is 0) and does no padding.
+ */
+size_t strlcpy(char *dest, const char *src, size_t bufsize)
+{
+	size_t len = strlen(src);
+	size_t ret = len;
+
+	if (bufsize == 0)
+		return ret;
+	if (len >= bufsize)
+		len = bufsize-1;
+	memcpy(dest, src, len);
+	dest[len] = '\0';   
+	return ret;
+}
+#endif
+
+#ifndef __HAVE_ARCH_STRLCAT
+/**
+ * strlcat - Append a length-limited, %NUL-terminated string to another
+ * @dest: The string to be appended to
+ * @src: The string to append to it
+ * @bufsize: Size of the destination buffer
+ *
+ * Returns the sum of the initial lengths of @src and @dest. The resulting
+ * string is always %NUL-terminated (unless @bufsize is 0).
+ */
+size_t strlcat(char *dest, const char *src, size_t bufsize)
+{
+	size_t len1 = strnlen(dest, bufsize);
+	size_t len2 = strlen(src);
+	size_t ret = len1 + len2;
+
+	if (bufsize == 0)
+		return ret;
+	if (len1+len2 >= bufsize)
+		len2 = bufsize - (len1+1);
+	if (len2 > 0) {
+		memcpy(dest+len1, src, len2);
+		dest[len1+len2] = '\0';
+	}
+	return ret;
+}
+#endif

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Linus Torvalds]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 766 bytes --]

In article <20030525210509.09429aaa.l.s.r@web.de>,
René Scharfe  <l.s.r@web.de> wrote:
>
>Anyway, I corrected this. Patch below contains a "BSD-compatible" version,
>and also a strlcat().

Ok, I did my own versions, since (a) I had already started and your
patches wouldn't apply, and (b) I hate adding a zillion lines of extra
copyright notices for a 5-line function and (c) I think your patch had
EXPORT_SYMBOL wrong.

In particular, if any architecture ever decides to roll their own
version of strlcpy/strlcat, the EXPORT_SYMBOL() in ksyms.c would be the
wrong thing to do. 

The current BK tree has my totally untested versions of these functions
("Famous last words: 'how hard can it be?'"), along with what I think
is the proper way to export them.

			Linus

[RFC] [2.5 patch] Change strlcpy and strlcat

[Adrian Bunk]

On Mon, May 26, 2003 at 01:13:05AM +0000, Linus Torvalds wrote:
> In article <20030525210509.09429aaa.l.s.r@web.de>,
> René Scharfe  <l.s.r@web.de> wrote:
> >
> >Anyway, I corrected this. Patch below contains a "BSD-compatible" version,
> >and also a strlcat().
> 
> Ok, I did my own versions, since (a) I had already started and your
> patches wouldn't apply, and (b) I hate adding a zillion lines of extra
> copyright notices for a 5-line function and (c) I think your patch had
> EXPORT_SYMBOL wrong.
> 
> In particular, if any architecture ever decides to roll their own
> version of strlcpy/strlcat, the EXPORT_SYMBOL() in ksyms.c would be the
> wrong thing to do. 
> 
> The current BK tree has my totally untested versions of these functions
> ("Famous last words: 'how hard can it be?'"), along with what I think
> is the proper way to export them.

Your API is compatible with *BSD but I'm wondering whether something 
slightly different might make error handling for callers easier:

Currently strlcpy returns the total length of the string it tried to 
create, whether it was too long or not. This means a check whether the 
complete string was copied is always something like

  if (strlcpy(dest, src, sizeof(dest)) >= sizeof(dest))
      goto toolong;

If strlcpy would return 0 for successful copies this would simplify to

  if (!strlcpy(dest, src, sizeof(dest)))
      goto toolong;


A patch is below.

I've tested only the compilation, any comments are welcome.


Another possible change to the semantics might be to copy/cat only if
dest is big enough to make error handling especially in the case of
strlcat easier (without additional information it's currently impossible
to get the original contents of dest if only parts of src were copied).


> 			Linus

cu
Adrian


--- linux-2.5.69/lib/string.c.old	2003-05-26 11:29:21.000000000 +0200
+++ linux-2.5.69/lib/string.c	2003-05-26 15:19:56.000000000 +0200
@@ -102,20 +102,30 @@
  * @src: Where to copy the string from
  * @size: size of destination buffer
  *
- * Compatible with *BSD: the result is always a valid
+ * Similar to *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.
+ * out the result like strncpy() does. Unlike *BSD it
+ * returns 0 for a successful copy.
  */
 size_t strlcpy(char *dest, const char *src, size_t size)
 {
 	size_t ret = strlen(src);
+	size_t len;
+
+	if (!size)
+		return 0;
+
+	if (ret >= size)
+		len = size - 1;
+	else {
+		len = ret;
+		ret = 0;
+		}
+
+	memcpy(dest, src, len);
+	dest[len] = '\0';
 
-	if (size) {
-		size_t len = (ret >= size) ? size-1 : ret;
-		memcpy(dest, src, len);
-		dest[len] = '\0';
-	}
 	return ret;
 }
 EXPORT_SYMBOL(strlcpy);
@@ -175,23 +185,33 @@
  * @dest: The string to be appended to
  * @src: The string to append to it
  * @count: The size of the destination buffer.
+ *
+ * Returns 0 if successful, the number of bytes needed in dest
+ * otherwise.
  */
 size_t strlcat(char *dest, const char *src, size_t count)
 {
 	size_t dsize = strlen(dest);
 	size_t len = strlen(src);
-	size_t res = dsize + len;
+	size_t ret;
 
 	/* This would be a bug */
 	BUG_ON(dsize >= count);
 
 	dest += dsize;
 	count -= dsize;
+
 	if (len >= count)
-		len = count-1;
+		{
+		ret = dsize + len;
+		len = count - 1;
+		}
+	else
+		ret = 0;
+
 	memcpy(dest, src, len);
-	dest[len] = 0;
-	return res;
+	dest[len] = '\0';
+	return ret;
 }
 EXPORT_SYMBOL(strlcat);
 #endif

Re: [RFC] [2.5 patch] Change strlcpy and strlcat

[Ben Collins]

>   if (strlcpy(dest, src, sizeof(dest)) >= sizeof(dest))
>       goto toolong;
> 
> If strlcpy would return 0 for successful copies this would simplify to
> 
>   if (!strlcpy(dest, src, sizeof(dest)))
>       goto toolong;

Part of the idea behind strlcpy was to explicity return the sizeof of
the result. Read the papers on the function itself.  The idea is that a
lot of callers will usually do strlen() after calling strncpy.

Now, I'll be honest, I don't remember seeing one single usage of strncpy
that was able to use the return of strlcpy. In fact, a few places that
could have, shouldn't have since the value returned didn't translate to
what the actual size of the string was. For example, you could not do:

	len = strlcpy(dest, myLongString, sizeof(dest));
	copy_to_user((void *)arg, dest, len);

If truncation occured, len would be the length it should have been, and
not what it really was.

Also, 99% of the users didn't care about truncation either because it
was known that it should never happen (e.g. netdevice names) or because
truncation didn't cause any problems (e.g. informational strings). The
use of strlcpy here was merely as a way to keep the kernel from choking
on itself in the event of something really stupid happening (it's a lot
easier to see a broken string than to trace overwriting random memory
from a strcpy gone long).

So your return value is as useless as the current return value. But I
think changing our strlcpy to mismatch *BSD would be even worse than
what is there now.

What might be nice is a function that has two bits of info. a) Truncation,
and b) The actual length of the resulting string. Maybe something like:

int strxcpy(char *dest, const char *src, size_t size, size_t *result)
{
	size_t src_size = strlen(src);
	size_t len = 0;

	if (size) {
		len = (src_size >= size) ? size - 1 : src_size;
		memcpy(dest, src, len);
		dest[len] = '\0';
		if (result)
			*result = len;
	}

	if (result)
		*result = len;

	return src_size >= size;
}

So the return value is zero for success and non-zero for truncation. In
addition, if *result is non-NULL, the actual length of the result is set
in that variable. The above example could then be:

	size_t len;
	strxcpy(dest, myLongString, sizeof(dest), &len);
        copy_to_user((void *)arg, dest, len);

IMO, this would only be for a few users, which is < 5% from what I can
see. One place where I saw this might be useful is in the usb-sound
driver where it creates an information string based on local info from
the device firmware and from the USB structures, depending on whether
the data was available in one place or both. It was complex, and the
result size would have been very useful (instead I had to write it to
use strlen in between strlcpy calls). Again, that's _one_ place out of
hundreds.

-- 
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
Deqo       - http://www.deqo.com/

Re: [RFC] [2.5 patch] Change strlcpy and strlcat

[Linus Torvalds]

On Mon, 26 May 2003, Adrian Bunk wrote:
> 
> Your API is compatible with *BSD but I'm wondering whether something 
> slightly different might make error handling for callers easier:

Well, judging by the fact that pretty much 99.9% of all users won't ever
care about the return value. And coupled with the fact that the current
behaviour is compatible with BSD (except for the BUG_ON() that I added,
and I have no idea what BSD does for that case, since it pretty clearly
_is_ a bug), I definitely prefer having standard return values than trying 
to make it "easier".

The BSD return values do actually make sense for nested operations.

		Linus