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;
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?
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.
Index: include/linux/device.h
===================================================================
RCS file: /home/scm/linux-2.5/include/linux/device.h,v
retrieving revision 1.48
diff -u -u -r1.48 device.h
--- include/linux/device.h 29 Apr 2003 17:30:20 -0000 1.48
+++ include/linux/device.h 13 May 2003 07:47:39 -0000
@@ -35,7 +35,7 @@
#define DEVICE_NAME_SIZE 50
#define DEVICE_NAME_HALF __stringify(20) /* Less than half to accommodate slop */
#define DEVICE_ID_SIZE 32
-#define BUS_ID_SIZE 20
+#define BUS_ID_SIZE KOBJ_NAME_LEN
enum {
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.
Thanks,
-pat
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;
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".
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);
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
> 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/
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 ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
> 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
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?
Linus Torvalds <[email protected]> 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 <[email protected]> <http://azz.us-lot.org/>
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
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
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; ???
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/
>
> 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/
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
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 ;)
On Sun, 25 May 2003 19:24:40 +0200 Edgar Toernig <[email protected]> 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
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?
On Sun, May 25, 2003 at 09:05:09PM +0200, Ren? Scharfe wrote:
> On Sun, 25 May 2003 19:24:40 +0200 Edgar Toernig <[email protected]> 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/
On Sun, 25 May 2003 15:01:20 -0400 [email protected] 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?
On Sun, 25 May 2003 14:16:22 -0400 Ben Collins <[email protected]> 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 <[email protected]>,
* Matthew Hawkins <[email protected]>
* - Kissed strtok() goodbye
+ *
+ * * Sun May 25 2003, Ren? Scharfe <[email protected]>
+ * - 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
On Sun, May 25, 2003 at 02:13:02PM -0400, [email protected] 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
In article <[email protected]>,
Ren? Scharfe <[email protected]> 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
On Mon, May 26, 2003 at 01:13:05AM +0000, Linus Torvalds wrote:
> In article <[email protected]>,
> Ren? Scharfe <[email protected]> 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
> 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/
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
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.
--
** [email protected] ** 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. *