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

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

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

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.

thanks,
Kay

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

diff -Nru a/namedev_parse.c b/namedev_parse.c
--- a/namedev_parse.c	Tue Feb 24 23:47:05 2004
+++ b/namedev_parse.c	Tue Feb 24 23:47:05 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	Tue Feb 24 23:47:05 2004
+++ b/udev-add.c	Tue Feb 24 23:47:05 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	Tue Feb 24 23:47:05 2004
+++ b/udev-remove.c	Tue Feb 24 23:47:05 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	Tue Feb 24 23:47:05 2004
+++ b/udev.h	Tue Feb 24 23:47:05 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	Tue Feb 24 23:47:05 2004
+++ b/udev_dbus.c	Tue Feb 24 23:47:05 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	Tue Feb 24 23:47:05 2004
+++ b/udevdb.c	Tue Feb 24 23:47:05 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	Tue Feb 24 23:47:05 2004
+++ b/udevinfo.c	Tue Feb 24 23:47:05 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	Tue Feb 24 23:47:05 2004
+++ b/udevsend.c	Tue Feb 24 23:47:05 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);
 }