Skip to contentSkip to conversation

2.5.47: strdup()

PATCH In review Olaf Dietsche 6 participants 10 messages Nov 16, 2002 – Nov 18, 2002

Subsystems (2)
Library code Generic string library
lore
Share & tools
lore

Olaf Dietsche

author
#0 lore raw

PATCH 1/1 2.5.47: strdup()

This *untested* patch adds strdup(). There are about five or six different strdup() implementations in various parts of the kernel.

Regards, Olaf.

include/linux/string.h +1 -1
diff -urN a/include/linux/string.h b/include/linux/string.h
@@ -15,7 +15,7 @@
 extern char * strpbrk(const char *,const char *);
 extern char * strsep(char **,const char *);
 extern __kernel_size_t strspn(const char *,const char *);
-
+extern char *strdup(const char *);
 
 /*
  * Include machine specific inline routines
kernel/ksyms.c +1 -0
diff -urN a/kernel/ksyms.c b/kernel/ksyms.c
@@ -576,6 +576,7 @@
 EXPORT_SYMBOL(strnicmp);
 EXPORT_SYMBOL(strspn);
 EXPORT_SYMBOL(strsep);
+EXPORT_SYMBOL(strdup);
 
 /* software interrupts */
 EXPORT_SYMBOL(tasklet_init);
lib/string.c +15 -0
diff -urN a/lib/string.c b/lib/string.c
@@ -22,6 +22,7 @@
 #include <linux/types.h>
 #include <linux/string.h>
 #include <linux/ctype.h>
+#include <linux/slab.h>
 
 #ifndef __HAVE_ARCH_STRNICMP
 /**

@@ -479,6 +480,20 @@
 		s1++;
 	}
 	return NULL;
+}
+#endif
+
+#ifndef __HAVE_ARCH_STRDUP
+/**
+ * strdup - allocate memory and duplicate a string
+ */
+char *strdup(const char *s)
+{
+	char *p = kmalloc(strlen(s) + 1, GFP_KERNEL);
+	if (p)
+		strcpy(p, s);
+
+	return p;
 }
 #endif
#1 lore raw
Quoted 6 lines · Olaf Dietsche

This *untested* patch adds strdup(). There are about five or six different strdup() implementations in various parts of the kernel.

lib/string.c +2 -0
+#ifndef __HAVE_ARCH_STRDUP
+/**

lib/string.c line 497

Quoted 10 lines
lib/string.c +9 -0
+ * strdup - allocate memory and duplicate a string
+ */
+char *strdup(const char *s)
+{
+	char *p = kmalloc(strlen(s) + 1, GFP_KERNEL);
+	if (p)
+		strcpy(p, s);
+
+	return p;
 }

Comments:

* arch-specific strdup is unlikely

* IMO safer to create a strndup, and then update all strdup callers to use strndup...

#2 lore raw
In reply to Olaf Dietsche

lib/string.c line 497

Quoted 9 lines · Olaf Dietsche
lib/string.c +7 -0
+char *strdup(const char *s)
+{
+       char *p = kmalloc(strlen(s) + 1, GFP_KERNEL);
+       if (p)
+               strcpy(p, s);
+
+       return p;
 }

It's best to not assume GFP_KERNEL in there. Make the caller pass in the required allocation mode.

author
#3 lore raw
Quoted 13 lines · Andrew Morton

Review

Quoted 9 lines · Olaf Dietsche
(quoted diff) +7 -0
+char *strdup(const char *s)
+{
+       char *p = kmalloc(strlen(s) + 1, GFP_KERNEL);
+       if (p)
+               strcpy(p, s);
+
+       return p;
 }

It's best to not assume GFP_KERNEL in there. Make the caller pass in the required allocation mode.

Ok, here is my next try, trying to include your and the other suggestions. As before, it compiles, but is untested.

Regards, Olaf.

include/linux/string.h +5 -0
diff -urN a/include/linux/string.h b/include/linux/string.h
@@ -7,6 +7,7 @@
 
 #include <linux/types.h>	/* for size_t */
 #include <linux/stddef.h>	/* for NULL */
+#include <linux/gfp.h>		/* for GFP_KERNEL */
 
 #ifdef __cplusplus
 extern "C" {

@@ -15,7 +16,11 @@
 extern char * strpbrk(const char *,const char *);
 extern char * strsep(char **,const char *);
 extern __kernel_size_t strspn(const char *,const char *);
+extern void *kmemdup(const void *, size_t, int);
 
+static inline char *kstrdup(const char *s, int flags)
+	{ return kmemdup(s, strlen(s) + 1, flags); }
+static inline char *strdup(const char *s) { return kstrdup(s, GFP_KERNEL); }
 
 /*
  * Include machine specific inline routines
kernel/ksyms.c +1 -0
diff -urN a/kernel/ksyms.c b/kernel/ksyms.c
@@ -576,6 +576,7 @@
 EXPORT_SYMBOL(strnicmp);
 EXPORT_SYMBOL(strspn);
 EXPORT_SYMBOL(strsep);
+EXPORT_SYMBOL(kmemdup);
 
 /* software interrupts */
 EXPORT_SYMBOL(tasklet_init);
lib/string.c +15 -0
diff -urN a/lib/string.c b/lib/string.c
@@ -22,6 +22,7 @@
 #include <linux/types.h>
 #include <linux/string.h>
 #include <linux/ctype.h>
+#include <linux/slab.h>
 
 #ifndef __HAVE_ARCH_STRNICMP
 /**

@@ -479,6 +480,20 @@
 		s1++;
 	}
 	return NULL;
+}
+#endif
+
+#ifndef __HAVE_ARCH_KMEMDUP
+/**
+ * kmemdup - allocate memory and duplicate a string
+ */
+void *kmemdup(const void *s, size_t n, int flags)
+{
+	void *p = kmalloc(n, flags);
+	if (p)
+		memcpy(p, s, n);
+
+	return p;
 }
 #endif
#4 lore raw
Quoted 1 line · Olaf Dietsche

+static inline char *strdup(const char *s) { return kstrdup(s, GFP_KERNEL); }

Why hide what's really going on ? Better change the callers to use kstrdup.

- Werner

Signature · 3 lines
  _________________________________________________________________________
 / Werner Almesberger, Buenos Aires, Argentina         [email protected] /
/_http://www.almesberger.net/____________________________________________/
author
#5 lore raw
Quoted 5 lines · Werner Almesberger
Quoted 1 line · Olaf Dietsche

+static inline char *strdup(const char *s) { return kstrdup(s, GFP_KERNEL); }

Why hide what's really going on ? Better change the callers to use kstrdup.

Convenience and laziness. And I haven't seen any other strdup() use, which uses different allocation flags. But it's there for everybody to use, if they wish.

Regards, Olaf.

#6 lore raw
In reply to Olaf Dietsche

On Sat, Nov 16, 2002 at 07:21:09AM +0100, Olaf Dietsche wrote:
+ char *p = kmalloc(strlen(s) + 1, GFP_KERNEL);

I'm very concerned the implicit allocation will be abused and OOM highmem boxen. Can you at least optionally highmem-allocate the buffers?

Thanks,
Bill

#7 lore raw
In reply to Olaf Dietsche
Quoted 2 lines · Olaf Dietsche

This *untested* patch adds strdup(). There are about five or six different strdup() implementations in various parts of the kernel.

How many users of these functions are there? I really don't like certain functions which allocate memory in nebulous ways and almost would prefer all users of this are fixed to specifically allocate and str[n]cpy copy themselves making it clear who is allocating memory and also who should free it.

author
#8 lore raw
Quoted 10 lines · Chris Wedgwood
Quoted 2 lines · Olaf Dietsche

This *untested* patch adds strdup(). There are about five or six different strdup() implementations in various parts of the kernel.

How many users of these functions are there? I really don't like certain functions which allocate memory in nebulous ways and almost would prefer all users of this are fixed to specifically allocate and str[n]cpy copy themselves making it clear who is allocating memory and also who should free it.

So you like duplicate code? Well, to each his own.

Regards, Olaf.

#9 lore raw
Quoted 1 line · Olaf Dietsche

So you like duplicate code? Well, to each his own.

Not at all.

I'm just not sure I like strdup being *easy* to use at it leads to misuse. Admittedly this is a very poor argument for not having it.

--cw