Skip to contentSkip to conversation

Use string bounded functions

PATCH In review Christoph Egger 3 participants 5 messages Jan 29, 2007

lore
Share & tools
lore

Christoph Egger

author
#0 lore raw

PATCH 1/1 Use string bounded functions

Hi!

The attached patch replaces sprintf with snprintf and strncpy with strlcpy.

There are various cases where no NULL-terminated strings are guaranteed and eventual possible overflows. This patch fixes them.

BTW: Since Xen kernel has its own string functions, can't we just remove sprintf() and strncpy()? IMO, Xen should not inherit the historical C relicts.

Christoph

xen/arch/x86/cpu/centaur.c +2 -1
diff -r f8ddcb758117 xen/arch/x86/cpu/centaur.c
@@ -437,7 +437,8 @@ static void __init init_centaur(struct c
 				/* Add L1 data and code cache sizes. */
 				c->x86_cache_size = (cc>>24)+(dd>>24);
 			}
-			sprintf( c->x86_model_id, "WinChip %s", name );
+			snprintf( c->x86_model_id, sizeof(c->x86_model_id),
+				"WinChip %s", name );
 			break;
 
 		case 6:
xen/arch/x86/cpu/common.c +2 -2
diff -r f8ddcb758117 xen/arch/x86/cpu/common.c
@@ -386,8 +386,8 @@ void __devinit identify_cpu(struct cpuin
 			strcpy(c->x86_model_id, p);
 		else
 			/* Last resort... */
-			sprintf(c->x86_model_id, "%02x/%02x",
-				c->x86_vendor, c->x86_model);
+			snprintf(c->x86_model_id, sizeof(c->x86_model_id),
+				"%02x/%02x", c->x86_vendor, c->x86_model);
 	}
 
 	/* Now the feature flags better reflect actual CPU features! */
xen/arch/x86/domain_build.c +2 -2
diff -r f8ddcb758117 xen/arch/x86/domain_build.c
@@ -821,7 +821,7 @@ int construct_dom0(struct domain *d,
     si->pt_base      = vpt_start + 2 * PAGE_SIZE * !!IS_COMPAT(d);
     si->nr_pt_frames = nr_pt_pages;
     si->mfn_list     = vphysmap_start;
-    sprintf(si->magic, "xen-%i.%i-x86_%d%s",
+    snprintf(si->magic, sizeof(si->magic), "xen-%i.%i-x86_%d%s",
             xen_major_version(), xen_minor_version(),
             elf_64bit(&elf) ? 64 : 32,
             parms.pae ? "p" : "");

@@ -871,7 +871,7 @@ int construct_dom0(struct domain *d,
 
     memset(si->cmd_line, 0, sizeof(si->cmd_line));
     if ( cmdline != NULL )
-        strncpy((char *)si->cmd_line, cmdline, sizeof(si->cmd_line)-1);
+        strlcpy((char *)si->cmd_line, cmdline, sizeof(si->cmd_line));
 
     if ( fill_console_start_info((void *)(si + 1)) )
     {
xen/arch/x86/hvm/intercept.c +1 -1
diff -r f8ddcb758117 xen/arch/x86/hvm/intercept.c
@@ -173,7 +173,7 @@ int hvm_register_savevm(struct domain *d
         return -1;
     }
 
-    strncpy(se->idstr, idstr, HVM_SE_IDSTR_LEN);
+    strlcpy(se->idstr, idstr, HVM_SE_IDSTR_LEN);
 
     se->instance_id = instance_id;
     se->version_id = version_id;
xen/arch/x86/oprofile/nmi_int.c +16 -19
diff -r f8ddcb758117 xen/arch/x86/oprofile/nmi_int.c
@@ -22,6 +22,7 @@
 #include <asm/regs.h>
 #include <asm/current.h>
 #include <xen/delay.h>
+#include <xen/string.h>
  
 #include "op_counter.h"
 #include "op_x86_model.h"

@@ -39,7 +40,6 @@ extern int active_id(struct domain *d);
 extern int active_id(struct domain *d);
 extern int is_profiled(struct domain *d);
 
-extern size_t strlcpy(char *dest, const char *src, size_t size);
 
 
 static int nmi_callback(struct cpu_user_regs *regs, int cpu)

@@ -276,20 +276,20 @@ static int __init p4_init(char * cpu_typ
 	}
 
 #ifndef CONFIG_SMP
-	strncpy (cpu_type, "i386/p4", XENOPROF_CPU_TYPE_SIZE - 1);
+	strlcpy (cpu_type, "i386/p4", XENOPROF_CPU_TYPE_SIZE);
 	model = &op_p4_spec;
 	return 1;
 #else
 	switch (smp_num_siblings) {
 		case 1:
-			strncpy (cpu_type, "i386/p4", 
-				 XENOPROF_CPU_TYPE_SIZE - 1);
+			strlcpy (cpu_type, "i386/p4", 
+				 XENOPROF_CPU_TYPE_SIZE);
 			model = &op_p4_spec;
 			return 1;
 
 		case 2:
-			strncpy (cpu_type, "i386/p4-ht", 
-				 XENOPROF_CPU_TYPE_SIZE - 1);
+			strlcpy (cpu_type, "i386/p4-ht", 
+				 XENOPROF_CPU_TYPE_SIZE);
 			model = &op_p4_ht2_spec;
 			return 1;
 	}

@@ -311,17 +311,17 @@ static int __init ppro_init(char *cpu_ty
 		return 0;
 	}
 	else if (cpu_model == 15)
-		strncpy (cpu_type, "i386/core_2", XENOPROF_CPU_TYPE_SIZE - 1);
+		strlcpy (cpu_type, "i386/core_2", XENOPROF_CPU_TYPE_SIZE);
 	else if (cpu_model == 14)
-		strncpy (cpu_type, "i386/core", XENOPROF_CPU_TYPE_SIZE - 1);
+		strlcpy (cpu_type, "i386/core", XENOPROF_CPU_TYPE_SIZE);
 	else if (cpu_model == 9)
-		strncpy (cpu_type, "i386/p6_mobile", XENOPROF_CPU_TYPE_SIZE - 1);
+		strlcpy (cpu_type, "i386/p6_mobile", XENOPROF_CPU_TYPE_SIZE);
 	else if (cpu_model > 5)
-		strncpy (cpu_type, "i386/piii", XENOPROF_CPU_TYPE_SIZE - 1);
+		strlcpy (cpu_type, "i386/piii", XENOPROF_CPU_TYPE_SIZE);
 	else if (cpu_model > 2)
-		strncpy (cpu_type, "i386/pii", XENOPROF_CPU_TYPE_SIZE - 1);
+		strlcpy (cpu_type, "i386/pii", XENOPROF_CPU_TYPE_SIZE);
 	else
-		strncpy (cpu_type, "i386/ppro", XENOPROF_CPU_TYPE_SIZE - 1);
+		strlcpy (cpu_type, "i386/ppro", XENOPROF_CPU_TYPE_SIZE);
 
 	model = &op_ppro_spec;
 	return 1;

@@ -346,9 +346,6 @@ int nmi_init(int *num_events, int *is_pr
 		}
 	}
  
-	/* Make sure string is NULL terminated */
-	cpu_type[XENOPROF_CPU_TYPE_SIZE - 1] = 0;
-
 	switch (vendor) {
 		case X86_VENDOR_AMD:
 			/* Needs to be at least an Athlon (or hammer in 32bit mode) */

@@ -361,15 +358,15 @@ int nmi_init(int *num_events, int *is_pr
 				return -ENODEV;
 			case 6:
 				model = &op_athlon_spec;
-				strncpy (cpu_type, "i386/athlon", 
-					 XENOPROF_CPU_TYPE_SIZE - 1);
+				strlcpy (cpu_type, "i386/athlon", 
+					 XENOPROF_CPU_TYPE_SIZE);
 				break;
 			case 0xf:
 				model = &op_athlon_spec;
 				/* Actually it could be i386/hammer too, but give
 				   user space an consistent name. */
-				strncpy (cpu_type, "x86-64/hammer", 
-					 XENOPROF_CPU_TYPE_SIZE - 1);
+				strlcpy (cpu_type, "x86-64/hammer", 
+					 XENOPROF_CPU_TYPE_SIZE);
 				break;
 			}
 			break;
xen/arch/x86/setup.c +44 -23

87 lines not on this page · raw message

xen/arch/x86/time.c +1 -1
diff -r f8ddcb758117 xen/arch/x86/time.c
@@ -274,7 +274,7 @@ static char *freq_string(u64 freq)
     unsigned int x, y;
     y = (unsigned int)do_div(freq, 1000000) / 1000;
     x = (unsigned int)freq;
-    sprintf(s, "%u.%03uMHz", x, y);
+    snprintf(s, sizeof(s), "%u.%03uMHz", x, y);
     return s;
 }
xen/common/gdbstub.c +1 -1
diff -r f8ddcb758117 xen/common/gdbstub.c
@@ -268,7 +268,7 @@ gdb_send_packet(struct gdb_context *ctx)
     char buf[3];
     int count;
 
-    sprintf(buf, "%.02x\n", ctx->out_csum);
+    snprintf(buf, sizeof(buf), "%.02x\n", ctx->out_csum);
 
     gdb_write_to_packet_char('#', ctx);
     gdb_write_to_packet(buf, 2, ctx);
xen/common/kernel.c +1 -2
diff -r f8ddcb758117 xen/common/kernel.c
@@ -72,8 +72,7 @@ void cmdline_parse(char *cmdline)
             switch ( param->type )
             {
             case OPT_STR:
-                strncpy(param->var, optval, param->len);
-                ((char *)param->var)[param->len-1] = '\0';
+                strlcpy(param->var, optval, param->len);
                 break;
             case OPT_UINT:
                 *(unsigned int *)param->var =
xen/common/keyhandler.c +2 -3
diff -r f8ddcb758117 xen/common/keyhandler.c
@@ -67,7 +67,7 @@ void register_keyhandler(
     ASSERT(key_table[key].u.handler == NULL);
     key_table[key].u.handler = handler;
     key_table[key].flags     = 0;
-    strncpy(key_table[key].desc, desc, STR_MAX);
+    strlcpy(key_table[key].desc, desc, STR_MAX);
     key_table[key].desc[STR_MAX-1] = '\0';
 }
 

@@ -77,8 +77,7 @@ void register_irq_keyhandler(
     ASSERT(key_table[key].u.irq_handler == NULL);
     key_table[key].u.irq_handler = handler;
     key_table[key].flags         = KEYHANDLER_IRQ_CALLBACK;
-    strncpy(key_table[key].desc, desc, STR_MAX);
-    key_table[key].desc[STR_MAX-1] = '\0';
+    strlcpy(key_table[key].desc, desc, STR_MAX);
 }
 
 static void show_handlers(unsigned char key)
xen/common/libelf/libelf-dominfo.c +8 -8
diff -r f8ddcb758117 xen/common/libelf/libelf-dominfo.c
@@ -128,16 +128,16 @@ int elf_xen_parse_note(struct elf_binary
     switch (type)
     {
     case XEN_ELFNOTE_LOADER:
-	strncpy(parms->loader, str, sizeof(parms->loader));
+	strlcpy(parms->loader, str, sizeof(parms->loader));
 	break;
     case XEN_ELFNOTE_GUEST_OS:
-	strncpy(parms->guest_os, str, sizeof(parms->guest_os));
+	strlcpy(parms->guest_os, str, sizeof(parms->guest_os));
 	break;
     case XEN_ELFNOTE_GUEST_VERSION:
-	strncpy(parms->guest_ver, str, sizeof(parms->guest_ver));
+	strlcpy(parms->guest_ver, str, sizeof(parms->guest_ver));
 	break;
     case XEN_ELFNOTE_XEN_VERSION:
-	strncpy(parms->xen_ver, str, sizeof(parms->xen_ver));
+	strlcpy(parms->xen_ver, str, sizeof(parms->xen_ver));
 	break;
     case XEN_ELFNOTE_PAE_MODE:
 	if (0 == strcmp(str, "yes"))

@@ -224,13 +224,13 @@ int elf_xen_parse_guest_info(struct elf_
 
 	/* strings */
 	if (0 == strcmp(name, "LOADER"))
-	    strncpy(parms->loader, value, sizeof(parms->loader));
+	    strlcpy(parms->loader, value, sizeof(parms->loader));
 	if (0 == strcmp(name, "GUEST_OS"))
-	    strncpy(parms->guest_os, value, sizeof(parms->guest_os));
+	    strlcpy(parms->guest_os, value, sizeof(parms->guest_os));
 	if (0 == strcmp(name, "GUEST_VER"))
-	    strncpy(parms->guest_ver, value, sizeof(parms->guest_ver));
+	    strlcpy(parms->guest_ver, value, sizeof(parms->guest_ver));
 	if (0 == strcmp(name, "XEN_VER"))
-	    strncpy(parms->xen_ver, value, sizeof(parms->xen_ver));
+	    strlcpy(parms->xen_ver, value, sizeof(parms->xen_ver));
 	if (0 == strcmp(name, "PAE"))
 	{
 	    if (0 == strcmp(value, "yes[extended-cr3]"))
xen/common/perfc.c +1 -2
diff -r f8ddcb758117 xen/common/perfc.c
@@ -148,9 +148,8 @@ static int perfc_copy_info(XEN_GUEST_HAN
     {
         for ( i = 0; i < NR_PERFCTRS; i++ )
         {
-            strncpy(perfc_d[i].name, perfc_info[i].name,
+            strlcpy(perfc_d[i].name, perfc_info[i].name,
                     sizeof(perfc_d[i].name));
-            perfc_d[i].name[sizeof(perfc_d[i].name)-1] = '\0';
 
             switch ( perfc_info[i].type )
             {
xen/common/rangeset.c +2 -3
diff -r f8ddcb758117 xen/common/rangeset.c
@@ -283,12 +283,11 @@ struct rangeset *rangeset_new(
 
     if ( name != NULL )
     {
-        strncpy(r->name, name, sizeof(r->name));
-        r->name[sizeof(r->name)-1] = '\0';
+        strlcpy(r->name, name, sizeof(r->name));
     }
     else
     {
-        sprintf(r->name, "(no name)");
+        snprintf(r->name, sizeof(r->name), "(no name)");
     }
 
     if ( (r->domain = d) != NULL )
xen/common/symbols.c +6 -4
diff -r f8ddcb758117 xen/common/symbols.c
@@ -142,15 +142,17 @@ void __print_symbol(const char *fmt, uns
     const char *name;
     unsigned long offset, size;
     char namebuf[KSYM_NAME_LEN+1];
-    char buffer[sizeof("%s+%#lx/%#lx [%s]") + KSYM_NAME_LEN +
-               2*(BITS_PER_LONG*3/10) + 1];
+
+#define BUFFER_SIZE sizeof("%s+%#lx/%#lx [%s]") + KSYM_NAME_LEN + \
+			2*(BITS_PER_LONG*3/10) + 1
+    char buffer[BUFFER_SIZE];
 
     name = symbols_lookup(address, &size, &offset, namebuf);
 
     if (!name)
-        sprintf(buffer, "???");
+        snprintf(buffer, BUFFER_SIZE, "???");
     else
-        sprintf(buffer, "%s+%#lx/%#lx", name, offset, size);
+        snprintf(buffer, BUFFER_SIZE, "%s+%#lx/%#lx", name, offset, size);
 
     printk(fmt, buffer);
 }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#1 lore raw
Quoted 7 lines · Christoph Egger

The attached patch replaces sprintf with snprintf and strncpy with strlcpy.

There are various cases where no NULL-terminated strings are guaranteed and eventual possible overflows. This patch fixes them.

BTW: Since Xen kernel has its own string functions, can't we just remove sprintf() and strncpy()? IMO, Xen should not inherit the historical C relicts.

This makes plenty of sense. Strncpy() in particular is dangerous and strlcpy() is always preferable. So I'd be happy to see strncat/strncpy die.

There are a few uses remaining (particularly in arch/ia64) that you'll have to fix first.

And please add 'signed-off-by' attribution when you post patches!

-- Keir

author
#2 lore raw
Quoted 7 lines · Keir Fraser
Quoted 6 lines · Christoph Egger

The attached patch replaces sprintf with snprintf and strncpy with strlcpy.

There are various cases where no NULL-terminated strings are guaranteed and eventual possible overflows. This patch fixes them.

Quoted 6 lines
Quoted 3 lines

BTW: Since Xen kernel has its own string functions, can't we just remove sprintf() and strncpy()? IMO, Xen should not inherit the historical C relicts.

This makes plenty of sense. Strncpy() in particular is dangerous and strlcpy() is always preferable. So I'd be happy to see strncat/strncpy die.

sprintf() is also dangerous. snprintf() is better. sprintf() should also die.

Quoted 2 lines

There are a few uses remaining (particularly in arch/ia64) that you'll have to fix first.

Yeah. But due to lack of hw, I can't even build test for ia64 and ppc. So when I send the patches, intel and ibm have to verify first that they don't break anything.

Quoted 1 line

And please add 'signed-off-by' attribution when you post patches!

Will do.

Christoph

#3 lore raw

I'm all for this.
Are we going to mark all the "bad" ones as deprecated for some time? -JX

Quoted 36 lines · Christoph Egger
Quoted 7 lines · Keir Fraser
Quoted 6 lines · Christoph Egger

The attached patch replaces sprintf with snprintf and strncpy with strlcpy.

There are various cases where no NULL-terminated strings are guaranteed and eventual possible overflows. This patch fixes them.

Quoted 6 lines
Quoted 3 lines

BTW: Since Xen kernel has its own string functions, can't we just remove sprintf() and strncpy()? IMO, Xen should not inherit the historical C relicts.

This makes plenty of sense. Strncpy() in particular is dangerous and strlcpy() is always preferable. So I'd be happy to see strncat/strncpy die.

sprintf() is also dangerous. snprintf() is better. sprintf() should also die.

Quoted 2 lines

There are a few uses remaining (particularly in arch/ia64) that you'll have to fix first.

Yeah. But due to lack of hw, I can't even build test for ia64 and ppc. So when I send the patches, intel and ibm have to verify first that they don't break anything.

Quoted 1 line

And please add 'signed-off-by' attribution when you post patches!

Will do.

Christoph

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#4 lore raw

That's overkill really. Once the uses are all gone from the repository we can just kill them off. Any out-of-tree patches can easily be fixed up.

-- Keir

Quoted 55 lines · Jimi Xenidis

I'm all for this.
Are we going to mark all the "bad" ones as deprecated for some time? -JX

Quoted 35 lines · Christoph Egger
Quoted 14 lines · Keir Fraser
Quoted 7 lines · Christoph Egger

The attached patch replaces sprintf with snprintf and strncpy with strlcpy.

There are various cases where no NULL-terminated strings are guaranteed
and eventual possible overflows. This patch fixes them.

Quoted 5 lines

BTW: Since Xen kernel has its own string functions, can't we just remove
sprintf() and strncpy()? IMO, Xen should not inherit the historical C
relicts.

Quoted 3 lines

This makes plenty of sense. Strncpy() in particular is dangerous and strlcpy() is always preferable. So I'd be happy to see strncat/ strncpy die.

sprintf() is also dangerous. snprintf() is better. sprintf() should also die.

Quoted 3 lines

There are a few uses remaining (particularly in arch/ia64) that you'll have
to fix first.

Yeah. But due to lack of hw, I can't even build test for ia64 and ppc. So when I send the patches, intel and ibm have to verify first that they don't
break anything.

Quoted 1 line

And please add 'signed-off-by' attribution when you post patches!

Will do.

Quoted 8 lines

Christoph

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel