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

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Rogier Wolff]

On Sat, May 24, 2003 at 08:52:53PM -0700, Linus Torvalds wrote:
> 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;
> 	}

Just catching up... 

Most people will think: "But that's not efficient!" he first
determines the size using strlen, and only then does he start a
memcpy.

In fact most modern processors priming the cache and then doing the
copy is noticably faster or just a teeny little bit slower.

			Roger. 

-- 
** R.E.Wolff@BitWizard.nl ** http://www.BitWizard.nl/ ** +31-15-2600998 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
* I didn't say it was your fault. I said I was going to blame it on you. *

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Matt Mackall]

On Sun, May 25, 2003 at 02:13:02PM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Sun, 25 May 2003 10:51:02 CDT, Matt Mackall said:
> 
> > The return value here isn't particularly useful. The OpenBSD
> > strlcpy/strlcat variant tell you how big the result should have been
> > so that you can realloc if need be.
> 
> A quick grep of the current source tree seems to indicate that there aren't
> any uses of 'strncpy' that actually save or check the return value.
> 
> As such, actually *using*  the return value would make for a job for the
> kernel janitors, to actually do something useful at all 650 or so uses.

The kernel, fortunately, isn't Perl or the like and really isn't
interested in strings outside the context of things like pathnames,
which realistically have to have finite limits. So while there
probably aren't a lot of uses for a return val, for the places where
there are, min(bufsize, optimalsize) is going to be less useful than
just optimalsize as you already know bufsize.
 
> Given that the kernel probably *shouldn't* be trying to strlcpy() into
> a destination that it won't fit, it may be more useful to do a BUG_ON()
> or similar (feel free to use a 'goto too_long;' if you prefer ;)

Outside of pathnames type things, where there's a well-defined return
code (ENAMETOOLONG), we probably end up not caring overmuch about
truncation..

-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Valdis.Kletnieks]

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

On Sun, 25 May 2003 10:51:02 CDT, Matt Mackall said:

> The return value here isn't particularly useful. The OpenBSD
> strlcpy/strlcat variant tell you how big the result should have been
> so that you can realloc if need be.

A quick grep of the current source tree seems to indicate that there aren't
any uses of 'strncpy' that actually save or check the return value.

As such, actually *using*  the return value would make for a job for the
kernel janitors, to actually do something useful at all 650 or so uses.

Given that the kernel probably *shouldn't* be trying to strlcpy() into
a destination that it won't fit, it may be more useful to do a BUG_ON()
or similar (feel free to use a 'goto too_long;' if you prefer ;)


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

RE: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Riley Williams]

Hi Matt.

 >> 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;
 >>	}

 > The return value here isn't particularly useful. The OpenBSD
 > strlcpy/strlcat variant tell you how big the result should have been
 > so that you can realloc if need be.

Something along the lines of...

	int strlcpy(char *tgt, char *src, int len)
	{
		int size = strlen(src);

		if (size < len)
			strcpy(tgt, src);
		else {
			memcpy(tgt, src, len-1);
			tgt[len] = '\0';
		}
		return size;
	}

...reindented according to standards (which I don't have to hand).

Best wishes from Riley.
---
 * Nothing as pretty as a smile, nothing as ugly as a frown.

---
Outgoing mail is certified Virus Free.
Checked by AVG anti-virus system (http://www.grisoft.com).
Version: 6.0.483 / Virus Database: 279 - Release Date: 19-May-2003

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Linus Torvalds]

On 25 May 2003, Adam Sampson wrote:
> 
> If you're going to do this, it might make sense to call it "strlcpy"
> for consistency with the OpenBSD-introduced function of the same name
> that's getting included in a lot of userspace these days...

Sure, done. I'll check it in asap (and I'll make the devfs parts that Ben 
was unhappy about use it too).

Somebody (else ;) should probably go through our current uses of strncpy()  
and see if they make sense. Some of them probably do, but I suspect 
anything name/path related does not.

		Linus

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Ben Collins]

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

> 
> which is what pretty much everybody really _wants_ to have anyway? We 
> should deprecate "strncpy()" within the kernel entirely.

Let's try this one for size. First patch adds strlcpy and strlcat. The
second patch applies to my initial offender, drivers/base.

I'll do obvious conversions of the rest of tree over the next week and
hope that the architectures will be able to do optimized versions in the
same time frame. If everything works out ok, strncpy (and maybe strncat)
can be either marked deprecated or just removed altogether.

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

[-- Attachment #2: strlcpy-strlcat.diff --]
[-- Type: text/plain, Size: 5590 bytes --]

Index: kernel/ksyms.c
===================================================================
--- kernel/ksyms.c	(revision 10041)
+++ kernel/ksyms.c	(working copy)
@@ -578,6 +578,8 @@
 EXPORT_SYMBOL(strnicmp);
 EXPORT_SYMBOL(strspn);
 EXPORT_SYMBOL(strsep);
+EXPORT_SYMBOL(strlcat);
+EXPORT_SYMBOL(strlcpy);
 
 /* software interrupts */
 EXPORT_SYMBOL(tasklet_init);
Index: include/linux/string.h
===================================================================
--- include/linux/string.h	(revision 10041)
+++ include/linux/string.h	(working copy)
@@ -25,12 +25,18 @@
 #ifndef __HAVE_ARCH_STRCPY
 extern char * strcpy(char *,const char *);
 #endif
+#ifndef __HAVE_ARCH_STRLCPY
+extern size_t strlcpy(char *dest, const char *src, size_t size);
+#endif
 #ifndef __HAVE_ARCH_STRNCPY
 extern char * strncpy(char *,const char *, __kernel_size_t);
 #endif
 #ifndef __HAVE_ARCH_STRCAT
 extern char * strcat(char *, const char *);
 #endif
+#ifndef __HAVE_ARCH_STRLCAT
+extern size_t strlcat(char *dest, const char *src, size_t size);
+#endif
 #ifndef __HAVE_ARCH_STRNCAT
 extern char * strncat(char *, const char *, __kernel_size_t);
 #endif
Index: lib/string.c
===================================================================
--- lib/string.c	(revision 10041)
+++ lib/string.c	(working copy)
@@ -17,8 +17,40 @@
  * * Sat Feb 09 2002, Jason Thomas <jason@topic.com.au>,
  *                    Matthew Hawkins <matt@mh.dropbear.id.au>
  * -  Kissed strtok() goodbye
+ *
+ * * Sun May 25 2003, Ben Collins <bcollins@debian.org>
+ * -  Added strlcpy and strlcat, which will replace strncpy and strncat
+ *    since they are safer and guarantee a NUL-terminated dest.
  */
- 
+
+/* Following applies to strlcpy and strlcat */
+/*
+ * Copyright (c) 2001 Richard Kettlewell <rjk@greenend.org.uk>
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. The name of the author may not be used to endorse or promote products
+ *    derived from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY
+ * AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL
+ * THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
+ * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+ * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
+ * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
 #include <linux/types.h>
 #include <linux/string.h>
 #include <linux/ctype.h>
@@ -72,6 +104,35 @@
 }
 #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
+ * @size: The size of the @dest buffer
+ *
+ * Note that unlike strncpy, the result is always guaranteed to be
+ * %NUL-terminated but is not %NUL-padded. This is much safer than
+ * strncpy. The return value is basically strlen(src), which means you can
+ * use it to check if the result should have been longer than @size,
+ * meaning it was truncated.
+ */
+size_t strlcpy(char *dest, const char *src, size_t size)
+{
+	size_t l = strlen(src);
+
+	if (l >= size) {
+		if (size) {
+			memcpy(dest, src, size - 1);
+			dest[size - 1] = 0;
+		}
+	} else if (l)
+		memcpy(dest, src, l + 1);
+
+	return l;
+}
+#endif
+
 #ifndef __HAVE_ARCH_STRNCPY
 /**
  * strncpy - Copy a length-limited, %NUL-terminated string
@@ -113,6 +174,53 @@
 }
 #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
+ * @size: The size of the @dest buffer
+ *
+ * Note that even though strncat %NUL-terminates, this function is more
+ * useful since it is easier to check for truncation.
+ *
+ * Unlink strncat, @size is the total size of @dest, not just the amount
+ * left. The return value is strlen(src) + strlen(initial dest)).
+ *
+ * If the return value is greater than @size, truncation occured.
+ */
+size_t strlcat(char *dest, const char *src, size_t size)
+{
+	size_t sl = strlen(src);
+	size_t dl, tl;
+
+	/* strlcpy wont give us NULL termination if size is 0, so we can't
+	 * check strlen(dest) if someone does this scenario:
+	 *
+	 * strlcpy(dest, "My ", 0);
+	 * strlcat(dest, "string", 0);
+	 *
+	 * Stupid, I know, but just a check.
+	 */
+	if (!size)
+		return sl;
+
+	dl = strlen(dest);
+	tl = sl + dl;
+
+	if (tl >= size) {
+		/* Truncation occurs, but check for room */
+		if (size > dl) {
+			memcpy(dest + dl, src, size - dl - 1);
+			dest[size - 1] = 0;
+		}
+	} else
+		memcpy(dest + dl, src, sl + 1);
+
+	return tl;
+}
+#endif
+
 #ifndef __HAVE_ARCH_STRNCAT
 /**
  * strncat - Append a length-limited, %NUL-terminated string to another

[-- Attachment #3: base-strlcpy.diff --]
[-- Type: text/plain, Size: 2963 bytes --]

Index: drivers/base/class.c
===================================================================
--- drivers/base/class.c	(revision 10041)
+++ drivers/base/class.c	(working copy)
@@ -87,8 +87,8 @@
 
 	INIT_LIST_HEAD(&cls->children);
 	INIT_LIST_HEAD(&cls->interfaces);
-	
-	strncpy(cls->subsys.kset.kobj.name,cls->name,KOBJ_NAME_LEN);
+
+	strlcpy(cls->subsys.kset.kobj.name, cls->name, KOBJ_NAME_LEN);
 	subsys_set_kset(cls,class_subsys);
 	subsystem_register(&cls->subsys);
 
@@ -258,7 +258,7 @@
 		 class_dev->class_id);
 
 	/* first, register with generic layer. */
-	strncpy(class_dev->kobj.name, class_dev->class_id, KOBJ_NAME_LEN);
+	strlcpy(class_dev->kobj.name, class_dev->class_id, KOBJ_NAME_LEN);
 	kobj_set_kset_s(class_dev, class_obj_subsys);
 	if (parent)
 		class_dev->kobj.parent = &parent->subsys.kset.kobj;
Index: drivers/base/sys.c
===================================================================
--- drivers/base/sys.c	(revision 10041)
+++ drivers/base/sys.c	(working copy)
@@ -63,8 +63,8 @@
 
 	error = device_register(&root->dev);
 	if (!error) {
-		strncpy(root->sysdev.bus_id,"sys",BUS_ID_SIZE);
-		strncpy(root->sysdev.name,"System Bus",DEVICE_NAME_SIZE);
+		strlcpy(root->sysdev.bus_id,"sys",BUS_ID_SIZE);
+		strlcpy(root->sysdev.name,"System Bus",DEVICE_NAME_SIZE);
 		root->sysdev.parent = &root->dev;
 		error = device_register(&root->sysdev);
 	};
Index: drivers/base/core.c
===================================================================
--- drivers/base/core.c	(revision 10041)
+++ drivers/base/core.c	(working copy)
@@ -211,7 +211,7 @@
 		 dev->bus_id, dev->name);
 
 	/* first, register with generic layer. */
-	strncpy(dev->kobj.name,dev->bus_id,KOBJ_NAME_LEN);
+	strlcpy(dev->kobj.name, dev->bus_id, KOBJ_NAME_LEN);
 	if (parent)
 		dev->kobj.parent = &parent->kobj;
 
Index: drivers/base/bus.c
===================================================================
--- drivers/base/bus.c	(revision 10041)
+++ drivers/base/bus.c	(working copy)
@@ -431,7 +431,7 @@
 	if (bus) {
 		pr_debug("bus %s: add driver %s\n",bus->name,drv->name);
 
-		strncpy(drv->kobj.name,drv->name,KOBJ_NAME_LEN);
+		strlcpy(drv->kobj.name, drv->name, KOBJ_NAME_LEN);
 		drv->kobj.kset = &bus->drivers;
 
 		if ((error = kobject_register(&drv->kobj))) {
@@ -540,15 +540,15 @@
  */
 int bus_register(struct bus_type * bus)
 {
-	strncpy(bus->subsys.kset.kobj.name,bus->name,KOBJ_NAME_LEN);
+	strlcpy(bus->subsys.kset.kobj.name, bus->name, KOBJ_NAME_LEN);
 	subsys_set_kset(bus,bus_subsys);
 	subsystem_register(&bus->subsys);
 
-	snprintf(bus->devices.kobj.name,KOBJ_NAME_LEN,"devices");
+	strlcpy(bus->devices.kobj.name, "devices", KOBJ_NAME_LEN);
 	bus->devices.subsys = &bus->subsys;
 	kset_register(&bus->devices);
 
-	snprintf(bus->drivers.kobj.name,KOBJ_NAME_LEN,"drivers");
+	strlcpy(bus->drivers.kobj.name, "drivers", KOBJ_NAME_LEN);
 	bus->drivers.subsys = &bus->subsys;
 	bus->drivers.ktype = &ktype_driver;
 	kset_register(&bus->drivers);

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Ben Collins]

On Sun, May 25, 2003 at 10:10:56AM -0700, Linus Torvalds wrote:
> 
> On 25 May 2003, Adam Sampson wrote:
> > 
> > If you're going to do this, it might make sense to call it "strlcpy"
> > for consistency with the OpenBSD-introduced function of the same name
> > that's getting included in a lot of userspace these days...
> 
> Sure, done. I'll check it in asap (and I'll make the devfs parts that Ben 
> was unhappy about use it too).
> 
> Somebody (else ;) should probably go through our current uses of strncpy()  
> and see if they make sense. Some of them probably do, but I suspect 
> anything name/path related does not.

Please hold off. I have a better patch that includes strlcat. Actually
tested.

-- 
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

[Matt Mackall]

On Sat, May 24, 2003 at 08:52:53PM -0700, Linus Torvalds wrote:
> 
> 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;
> 	}

The return value here isn't particularly useful. The OpenBSD
strlcpy/strlcat variant tell you how big the result should have been
so that you can realloc if need be.

-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Adam Sampson]

Linus Torvalds <torvalds@transmeta.com> writes:

> How about just adding a sane
> 	int copy_string(char *dest, const char *src, int len)
[...]

If you're going to do this, it might make sense to call it "strlcpy"
for consistency with the OpenBSD-introduced function of the same name
that's getting included in a lot of userspace these days...

<http://www.courtesan.com/todd/papers/strlcpy.html>

-- 
Adam Sampson <azz@us-lot.org>                   <http://azz.us-lot.org/>

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Russell King]

On Sat, May 24, 2003 at 08:07:01PM -0400, Ben Collins wrote:
> Given that the problem with KOBJ_NAME_LEN == 20 affecting one snd driver
> has so far only been explained as a compiler bug, can I suggest this
> patch be applied?

No one has confirmed that it is a compiler bug yet.  We only suspect
that something in the yamaha driver is getting miscompiled.  We don't
know what, we don't know how, we don't know why.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Linus Torvalds]

On Sat, 24 May 2003, Ben Collins wrote:
>
> Given that the problem with KOBJ_NAME_LEN == 20 affecting one snd driver
> has so far only been explained as a compiler bug, can I suggest this
> patch be applied? Even aside from the KOBJ_NAME_LEN == 20, the snprintf
> changes will keep things from breaking in other ways that are current
> now.

I hate using snprintf() for this kind of mindless string copy opertation.

Yeah, "strncpy()" is a frigging disaster when it comes to '\0', in many
ways. We should probably disallow using strncpy(), and aim for a _sane_
implementation that does what we actually want (none of that zero-padding
crap, and _always_ put a NUL at the end). I bet that is what most current
strncpy() users actually would want.

But switching it over to "snprintf()" is overkill. 

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.

		Linus

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Ben Collins]

> which is what pretty much everybody really _wants_ to have anyway? We 
> should deprecate "strncpy()" within the kernel entirely.

Amen.

If you could atleast apply the KOBJ_NAME_LEN part of the patch, I'll
start doing a much wider scope patch of replacing strncpy in the whole
kernel.

-- 
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

[Ben Collins]

Given that the problem with KOBJ_NAME_LEN == 20 affecting one snd driver
has so far only been explained as a compiler bug, can I suggest this
patch be applied? Even aside from the KOBJ_NAME_LEN == 20, the snprintf
changes will keep things from breaking in other ways that are current
now.


Index: include/linux/kobject.h
===================================================================
--- include/linux/kobject.h	(revision 10014)
+++ include/linux/kobject.h	(working copy)
@@ -12,7 +12,7 @@
 #include <linux/rwsem.h>
 #include <asm/atomic.h>
 
-#define KOBJ_NAME_LEN	16
+#define KOBJ_NAME_LEN	20
 
 struct kobject {
 	char			name[KOBJ_NAME_LEN];
Index: drivers/base/class.c
===================================================================
--- drivers/base/class.c	(revision 10014)
+++ drivers/base/class.c	(working copy)
@@ -87,8 +87,9 @@
 
 	INIT_LIST_HEAD(&cls->children);
 	INIT_LIST_HEAD(&cls->interfaces);
-	
-	strncpy(cls->subsys.kset.kobj.name,cls->name,KOBJ_NAME_LEN);
+
+	snprintf(cls->subsys.kset.kobj.name, KOBJ_NAME_LEN, "%s",
+		 cls->name);
 	subsys_set_kset(cls,class_subsys);
 	subsystem_register(&cls->subsys);
 
@@ -258,7 +259,7 @@
 		 class_dev->class_id);
 
 	/* first, register with generic layer. */
-	strncpy(class_dev->kobj.name, class_dev->class_id, KOBJ_NAME_LEN);
+	snprintf(class_dev->kobj.name, KOBJ_NAME_LEN, "%s", class_dev->class_id);
 	kobj_set_kset_s(class_dev, class_obj_subsys);
 	if (parent)
 		class_dev->kobj.parent = &parent->subsys.kset.kobj;
Index: drivers/base/core.c
===================================================================
--- drivers/base/core.c	(revision 10014)
+++ drivers/base/core.c	(working copy)
@@ -211,7 +211,7 @@
 		 dev->bus_id, dev->name);
 
 	/* first, register with generic layer. */
-	strncpy(dev->kobj.name,dev->bus_id,KOBJ_NAME_LEN);
+	snprintf(dev->kobj.name, KOBJ_NAME_LEN, "%s", dev->bus_id);
 	if (parent)
 		dev->kobj.parent = &parent->kobj;
 
Index: drivers/base/bus.c
===================================================================
--- drivers/base/bus.c	(revision 10014)
+++ drivers/base/bus.c	(working copy)
@@ -431,7 +431,7 @@
 	if (bus) {
 		pr_debug("bus %s: add driver %s\n",bus->name,drv->name);
 
-		strncpy(drv->kobj.name,drv->name,KOBJ_NAME_LEN);
+		snprintf(drv->kobj.name, KOBJ_NAME_LEN, "%s", drv->name);
 		drv->kobj.kset = &bus->drivers;
 
 		if ((error = kobject_register(&drv->kobj))) {
@@ -540,7 +540,8 @@
  */
 int bus_register(struct bus_type * bus)
 {
-	strncpy(bus->subsys.kset.kobj.name,bus->name,KOBJ_NAME_LEN);
+	snprintf(bus->subsys.kset.kobj.name, KOBJ_NAME_LEN, "%s",
+		 bus->name);
 	subsys_set_kset(bus,bus_subsys);
 	subsystem_register(&bus->subsys);
 

Re: Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Felipe Alfaro Solana]

On Fri, 2003-05-16 at 02:20, Ben Collins wrote:
> On Tue, May 13, 2003 at 08:08:35AM -0700, Patrick Mochel wrote:
> > 
> > On Tue, 13 May 2003, Ben Collins wrote:
> > 
> > > On Tue, May 13, 2003 at 08:10:32AM +0100, Christoph Hellwig wrote:
> > > > On Tue, May 13, 2003 at 02:26:40AM -0400, Ben Collins wrote:
> > > > > This was causing me all sorts of problems with linux1394's 16-18 byte
> > > > > long bus_id lengths. The sysfs names were all broken.
> > > > > 
> > > > > This not only makes KOBJ_NAME_LEN match BUS_ID_SIZE, but fixes the
> > > > > strncpy's in drivers/base/ so that it can't happen again (atleast the
> > > > > strings will be null terminated).
> > > > 
> > > > What about defining BUS_ID_SIZE in terms of KOBJ_NAME_LEN?
> > > 
> > > Ok, then add this in addition to the previous patch.
> > 
> > I'll add this, and sync with Linus this week, if he doesn't pick it up.
> 
> *sigh* Patrick, you accepted the BUS_ID_SIZE change without the original
> patch, so now BUS_ID_SIZE is back to 16 bytes.
> 
> Linus, please apply this patch to get things right again.

Well, it seems this patch has bad interaction with the Yamaha ALSA sound
driver (aka snd-ymfpci). Don't know why, but changing KOBJ_NAME_LEN from
16 to 20, causes snd-ymfpci.ko to oops.

If you have the time, please take a look at another thread in the LKML
called "pccard oops while booting".

Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

[Ben Collins]

On Tue, May 13, 2003 at 08:08:35AM -0700, Patrick Mochel wrote:
> 
> On Tue, 13 May 2003, Ben Collins wrote:
> 
> > On Tue, May 13, 2003 at 08:10:32AM +0100, Christoph Hellwig wrote:
> > > On Tue, May 13, 2003 at 02:26:40AM -0400, Ben Collins wrote:
> > > > This was causing me all sorts of problems with linux1394's 16-18 byte
> > > > long bus_id lengths. The sysfs names were all broken.
> > > > 
> > > > This not only makes KOBJ_NAME_LEN match BUS_ID_SIZE, but fixes the
> > > > strncpy's in drivers/base/ so that it can't happen again (atleast the
> > > > strings will be null terminated).
> > > 
> > > What about defining BUS_ID_SIZE in terms of KOBJ_NAME_LEN?
> > 
> > Ok, then add this in addition to the previous patch.
> 
> I'll add this, and sync with Linus this week, if he doesn't pick it up.

*sigh* Patrick, you accepted the BUS_ID_SIZE change without the original
patch, so now BUS_ID_SIZE is back to 16 bytes.

Linus, please apply this patch to get things right again.

-------

This was causing me all sorts of problems with linux1394's 16-18 byte
long bus_id lengths. The sysfs names were all broken.

This not only makes KOBJ_NAME_LEN match BUS_ID_SIZE, but fixes the
strncpy's in drivers/base/ so that it can't happen again (atleast the
strings will be null terminated).

Index: include/linux/kobject.h
===================================================================
RCS file: /home/scm/linux-2.5/include/linux/kobject.h,v
retrieving revision 1.11
diff -u -r1.11 kobject.h
--- include/linux/kobject.h	12 Apr 2003 01:04:46 -0000	1.11
+++ include/linux/kobject.h	13 May 2003 06:56:30 -0000
@@ -12,7 +12,7 @@
 #include <linux/rwsem.h>
 #include <asm/atomic.h>
 
-#define KOBJ_NAME_LEN	16
+#define KOBJ_NAME_LEN	20
 
 struct kobject {
 	char			name[KOBJ_NAME_LEN];
Index: drivers/base/bus.c
===================================================================
RCS file: /home/scm/linux-2.5/drivers/base/bus.c,v
retrieving revision 1.24
diff -u -r1.24 bus.c
--- drivers/base/bus.c	29 Apr 2003 17:30:20 -0000	1.24
+++ drivers/base/bus.c	13 May 2003 06:56:30 -0000
@@ -432,6 +432,7 @@
 		pr_debug("bus %s: add driver %s\n",bus->name,drv->name);
 
 		strncpy(drv->kobj.name,drv->name,KOBJ_NAME_LEN);
+		drv->kobj.name[KOBJ_NAME_LEN-1] = '\0';
 		drv->kobj.kset = &bus->drivers;
 
 		if ((error = kobject_register(&drv->kobj))) {
@@ -541,6 +542,7 @@
 int bus_register(struct bus_type * bus)
 {
 	strncpy(bus->subsys.kset.kobj.name,bus->name,KOBJ_NAME_LEN);
+	bus->subsys.kset.kobj.name[KOBJ_NAME_LEN-1] = '\0';
 	subsys_set_kset(bus,bus_subsys);
 	subsystem_register(&bus->subsys);
 
Index: drivers/base/class.c
===================================================================
RCS file: /home/scm/linux-2.5/drivers/base/class.c,v
retrieving revision 1.18
diff -u -r1.18 class.c
--- drivers/base/class.c	11 May 2003 05:00:57 -0000	1.18
+++ drivers/base/class.c	13 May 2003 06:56:30 -0000
@@ -89,6 +89,7 @@
 	INIT_LIST_HEAD(&cls->interfaces);
 	
 	strncpy(cls->subsys.kset.kobj.name,cls->name,KOBJ_NAME_LEN);
+	cls->subsys.kset.kobj.name[KOBJ_NAME_LEN-1] = '\0';
 	subsys_set_kset(cls,class_subsys);
 	subsystem_register(&cls->subsys);
 
@@ -259,6 +260,7 @@
 
 	/* first, register with generic layer. */
 	strncpy(class_dev->kobj.name, class_dev->class_id, KOBJ_NAME_LEN);
+	class_dev->kobj.name[KOBJ_NAME_LEN-1] = '\0';
 	kobj_set_kset_s(class_dev, class_obj_subsys);
 	if (parent)
 		class_dev->kobj.parent = &parent->subsys.kset.kobj;
Index: drivers/base/core.c
===================================================================
RCS file: /home/scm/linux-2.5/drivers/base/core.c,v
retrieving revision 1.43
diff -u -r1.43 core.c
--- drivers/base/core.c	29 Apr 2003 17:30:20 -0000	1.43
+++ drivers/base/core.c	13 May 2003 06:56:30 -0000
@@ -214,6 +214,7 @@
 
 	/* first, register with generic layer. */
 	strncpy(dev->kobj.name,dev->bus_id,KOBJ_NAME_LEN);
+	dev->kobj.name[KOBJ_NAME_LEN-1] = '\0';
 	kobj_set_kset_s(dev,devices_subsys);
 	if (parent)
 		dev->kobj.parent = &parent->kobj;