Re: [PATCH] udev - safer string handling all over the place

[Kay Sievers <kay.sievers@vrfy.org>]

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

On Tue, Feb 24, 2004 at 11:50:52PM +0100, Kay Sievers wrote:
> Here is the first step towards a safer string handling.
> More will follow, but for now only the easy ones :)
> 
> Thanks to all who pointed this out. strncat() isn't a nice function. We
> all should remember that the destination string is not terminated if the
> given lenght is shorter than the strlen of the source string.
> 
> And shame on the various implementers of strfieldcat() I found in the
> unapplied patches on this list, it's not really better than strncpy()
> and hides the real problem.

Hmm, bk didn't checked in one file, maybe I edited it again as root.
Nevermind, here is the more complete version.

thanks,
KAy

[-- Attachment #2: 02-stringfield.patch --]
[-- Type: text/plain, Size: 8938 bytes --]

diff -Nru a/namedev.c b/namedev.c
--- a/namedev.c	Wed Feb 25 00:04:14 2004
+++ b/namedev.c	Wed Feb 25 00:04:14 2004
@@ -157,7 +157,7 @@
 static char *get_default_owner(void)
 {
 	if (strlen(default_owner_str) == 0)
-		strncpy(default_owner_str, "root", OWNER_SIZE);
+		strfieldcpy(default_owner_str, "root");
 
 	return default_owner_str;
 }
@@ -165,7 +165,7 @@
 static char *get_default_group(void)
 {
 	if (strlen(default_group_str) == 0)
-		strncpy(default_group_str, "root", GROUP_SIZE);
+		strfieldcpy(default_group_str, "root");
 
 	return default_group_str;
 }
@@ -276,7 +276,7 @@
 			if (attr != NULL)
 				i = atoi(attr);
 			if (i > 0) {
-				strncpy(temp1, udev->program_result, sizeof(temp1));
+				strfieldcpy(temp1, udev->program_result);
 				pos2 = temp1;
 				while (i) {
 					i--;
@@ -837,8 +837,8 @@
 	} else {
 		/* no matching perms found :( */
 		udev->mode = get_default_mode();
-		strncpy(udev->owner, get_default_owner(), OWNER_SIZE);
-		strncpy(udev->group, get_default_group(), GROUP_SIZE);
+		strfieldcpy(udev->owner, get_default_owner());
+		strfieldcpy(udev->group, get_default_group());
 	}
 	dbg("name, '%s' is going to have owner='%s', group='%s', mode = %#o",
 	    udev->name, udev->owner, udev->group, udev->mode);
diff -Nru a/namedev_parse.c b/namedev_parse.c
--- a/namedev_parse.c	Wed Feb 25 00:04:14 2004
+++ b/namedev_parse.c	Wed Feb 25 00:04:14 2004
@@ -319,21 +319,21 @@
 			dbg("cannot parse line '%s'", line);
 			continue;
 		}
-		strncpy(dev.name, temp2, sizeof(dev.name));
+		strfieldcpy(dev.name, temp2);
 
 		temp2 = strsep(&temp, ":");
 		if (!temp2) {
 			dbg("cannot parse line '%s'", line);
 			continue;
 		}
-		strncpy(dev.owner, temp2, sizeof(dev.owner));
+		strfieldcpy(dev.owner, temp2);
 
 		temp2 = strsep(&temp, ":");
 		if (!temp2) {
 			dbg("cannot parse line '%s'", line);
 			continue;
 		}
-		strncpy(dev.group, temp2, sizeof(dev.group));
+		strfieldcpy(dev.group, temp2);
 
 		if (!temp) {
 			dbg("cannot parse line: %s", line);
@@ -422,7 +422,7 @@
 	/* parse every file in the list */
 	list_for_each_entry_safe(loop_file, tmp_file, &file_list, list) {
 		strfieldcpy(file, filename);
-		strcat(file, loop_file->name);
+		strfieldcat(file, loop_file->name);
 		parser(file);
 		list_del(&loop_file->list);
 		free(loop_file);
diff -Nru a/udev-add.c b/udev-add.c
--- a/udev-add.c	Wed Feb 25 00:04:14 2004
+++ b/udev-add.c	Wed Feb 25 00:04:14 2004
@@ -78,7 +78,7 @@
 	int retval;
 	struct stat stats;
 	
-	strncpy(p, file, sizeof(p));
+	strfieldcpy(p, file);
 	pos = strchr(p+1, '/');
 	while (1) {
 		pos = strchr(pos+1, '/');
@@ -145,8 +145,8 @@
 	int i;
 	int tail;
 
-	strncpy(filename, udev_root, sizeof(filename));
-	strncat(filename, dev->name, sizeof(filename));
+	strfieldcpy(filename, udev_root);
+	strfieldcat(filename, dev->name);
 
 	switch (dev->type) {
 	case 'b':
@@ -225,8 +225,8 @@
 			if (linkname == NULL || linkname[0] == '\0')
 				break;
 
-			strncpy(filename, udev_root, sizeof(filename));
-			strncat(filename, linkname, sizeof(filename));
+			strfieldcpy(filename, udev_root);
+			strfieldcat(filename, linkname);
 			dbg("symlink '%s' to node '%s' requested", filename, dev->name);
 			if (!fake)
 				if (strrchr(linkname, '/'))
@@ -243,13 +243,13 @@
 			}
 			while (linkname[i] != '\0') {
 				if (linkname[i] == '/')
-					strcat(linktarget, "../");
+					strfieldcat(linktarget, "../");
 				i++;
 			}
 
 			if (linktarget[0] == '\0')
-				strcpy(linktarget, "./");
-			strcat(linktarget, &dev->name[tail]);
+				strfieldcpy(linktarget, "./");
+			strfieldcat(linktarget, &dev->name[tail]);
 
 			/* unlink existing files to ensure that our symlink is created */
 			if (!fake && (lstat(filename, &stats) == 0)) {
@@ -278,8 +278,8 @@
 	char dev_path[SYSFS_PATH_MAX];
 	struct sysfs_class_device *class_dev = NULL;
 
-	strcpy(dev_path, sysfs_path);
-	strcat(dev_path, device_name);
+	strfieldcpy(dev_path, sysfs_path);
+	strfieldcat(dev_path, device_name);
 	dbg("looking at '%s'", dev_path);
 
 	/* open up the sysfs class device for this thing... */
@@ -304,9 +304,9 @@
 	int loop = SECONDS_TO_WAIT_FOR_DEV;
 	int retval;
 
-	strcpy(filename, sysfs_path);
-	strcat(filename, path);
-	strcat(filename, "/dev");
+	strfieldcpy(filename, sysfs_path);
+	strfieldcat(filename, path);
+	strfieldcat(filename, "/dev");
 
 	while (loop--) {
 		struct stat buf;
diff -Nru a/udev-remove.c b/udev-remove.c
--- a/udev-remove.c	Wed Feb 25 00:04:14 2004
+++ b/udev-remove.c	Wed Feb 25 00:04:14 2004
@@ -72,8 +72,8 @@
 	int retval;
 	int i;
 
-	strncpy(filename, udev_root, sizeof(filename));
-	strncat(filename, dev->name, sizeof(filename));
+	strfieldcpy(filename, udev_root);
+	strfieldcat(filename, dev->name);
 
 	info("removing device node '%s'", filename);
 	retval = unlink(filename);
@@ -103,8 +103,8 @@
 			if (linkname == NULL)
 				break;
 
-			strncpy(filename, udev_root, sizeof(filename));
-			strncat(filename, linkname, sizeof(filename));
+			strfieldcpy(filename, udev_root);
+			strfieldcat(filename, linkname);
 
 			dbg("unlinking symlink '%s'", filename);
 			retval = unlink(filename);
@@ -141,7 +141,7 @@
 		temp = strrchr(path, '/');
 		if (temp == NULL)
 			return -ENODEV;
-		strncpy(dev.name, &temp[1], sizeof(dev.name));
+		strfieldcpy(dev.name, &temp[1]);
 	}
 
 	dbg("name is '%s'", dev.name);
diff -Nru a/udev.h b/udev.h
--- a/udev.h	Wed Feb 25 00:04:14 2004
+++ b/udev.h	Wed Feb 25 00:04:14 2004
@@ -61,6 +61,12 @@
 	strncpy(to, from, sizeof(to)-1); \
 } while (0)
 
+#define strfieldcat(to, from) \
+do { \
+	to[sizeof(to)-1] = '\0'; \
+	strncat(to, from, sizeof(to) - strlen(to) -1); \
+} while (0)
+
 extern int udev_add_device(char *path, char *subsystem, int fake);
 extern int udev_remove_device(char *path, char *subsystem);
 extern void udev_init_config(void);
diff -Nru a/udev_dbus.c b/udev_dbus.c
--- a/udev_dbus.c	Wed Feb 25 00:04:14 2004
+++ b/udev_dbus.c	Wed Feb 25 00:04:14 2004
@@ -80,8 +80,8 @@
 	if (sysbus_connection == NULL)
 		return;
 
-	strncpy(filename, udev_root, sizeof(filename));
-	strncat(filename, dev->name, sizeof(filename));
+	strfieldcpy(filename, udev_root);
+	strfieldcat(filename, dev->name);
 
 	/* object, interface, member */
 	message = dbus_message_new_signal("/org/kernel/udev/NodeMonitor", 
@@ -114,8 +114,8 @@
 	if (sysbus_connection == NULL)
 		return;
 
-	strncpy(filename, udev_root, sizeof(filename));
-	strncat(filename, name, sizeof(filename));
+	strfieldcpy(filename, udev_root);
+	strfieldcat(filename, name);
 
 	/* object, interface, member */
 	message = dbus_message_new_signal("/org/kernel/udev/NodeMonitor", 
diff -Nru a/udevdb.c b/udevdb.c
--- a/udevdb.c	Wed Feb 25 00:04:14 2004
+++ b/udevdb.c	Wed Feb 25 00:04:14 2004
@@ -53,7 +53,7 @@
 		return -ENODEV;
 
 	memset(keystr, 0, NAME_SIZE);
-	strcpy(keystr, path);
+	strfieldcpy(keystr, path);
 	key.dptr = keystr;
 	key.dsize = strlen(keystr) + 1;
 
@@ -91,7 +91,7 @@
 		return -EINVAL;
 
 	memset(keystr, 0, sizeof(keystr));
-	strcpy(keystr, path);
+	strfieldcpy(keystr, path);
 
 	key.dptr = keystr;
 	key.dsize = strlen(keystr) + 1;
@@ -180,7 +180,7 @@
 {
 	if (strncmp(dev->name, find_name, sizeof(dev->name)) == 0) {
 		memcpy(find_dev, dev, sizeof(*find_dev));
-		strncpy(find_path, path, NAME_SIZE);
+		strfieldcpy(find_path, path);
 		find_found = 1;
 		/* stop search */
 		return 1;
diff -Nru a/udevinfo.c b/udevinfo.c
--- a/udevinfo.c	Wed Feb 25 00:04:14 2004
+++ b/udevinfo.c	Wed Feb 25 00:04:14 2004
@@ -73,7 +73,7 @@
 
 	dlist_for_each_data(attributes, attr, struct sysfs_attribute) {
 		if (attr->value != NULL) {
-			strncpy(value, attr->value, SYSFS_VALUE_MAX);
+			strfieldcpy(value, attr->value);
 			len = strlen(value);
 			if (len == 0)
 				continue;
@@ -306,8 +306,8 @@
 			} else {
 				if (path[0] != '/') {
 					/* prepend '/' if missing */
-					strcat(temp, "/");
-					strncat(temp, path, sizeof(path));
+					strfieldcat(temp, "/");
+					strfieldcat(temp, path);
 					pos = temp;
 				} else {
 					pos = path;
@@ -343,7 +343,7 @@
 		case NAME:
 			if (root)
 				strfieldcpy(result, udev_root);
-			strncat(result, dev.name, sizeof(result));
+			strfieldcat(result, dev.name);
 			break;
 
 		case SYMLINK:
@@ -385,7 +385,7 @@
 				/* prepend sysfs mountpoint if not given */
 				strfieldcpy(temp, path);
 				strfieldcpy(path, sysfs_path);
-				strncat(path, temp, sizeof(path));
+				strfieldcat(path, temp);
 			}
 			print_device_chain(path);
 			return 0;
diff -Nru a/udevsend.c b/udevsend.c
--- a/udevsend.c	Wed Feb 25 00:04:14 2004
+++ b/udevsend.c	Wed Feb 25 00:04:14 2004
@@ -82,9 +82,9 @@
 	memset(msg, 0x00, sizeof(*msg));
 	strfieldcpy(msg->magic, UDEV_MAGIC);
 	msg->seqnum = seqnum;
-	strncpy(msg->action, action, 8);
-	strncpy(msg->devpath, devpath, 128);
-	strncpy(msg->subsystem, subsystem, 16);
+	strfieldcpy(msg->action, action);
+	strfieldcpy(msg->devpath, devpath);
+	strfieldcpy(msg->subsystem, subsystem);
 	return sizeof(struct hotplug_msg);
 }