udev - safer string handling all over the place

PATCH Applied Kay Sievers 2 participants 3 messages Feb 24, 2004 – Feb 26, 2004

lore

Share & tools

lore

Kay Sievers

author

2004-02-24 22:50 UTC #0 lore raw

PATCH 1/1 udev - safer string handling all over the place

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

namedev_parse.c +4 -4

diff -Nru a/namedev_parse.c b/namedev_parse.c
@@ -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);

udev-add.c +13 ~~-13~~

diff -Nru a/udev-add.c b/udev-add.c
@@ -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;

udev-remove.c +5 -5

diff -Nru a/udev-remove.c b/udev-remove.c
@@ -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);

udev.h +6 -0

diff -Nru a/udev.h b/udev.h
@@ -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);

udev_dbus.c +4 -4

diff -Nru a/udev_dbus.c b/udev_dbus.c
@@ -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",

udevdb.c +3 -3

diff -Nru a/udevdb.c b/udevdb.c
@@ -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;

udevinfo.c +5 -5

diff -Nru a/udevinfo.c b/udevinfo.c
@@ -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;

udevsend.c +3 -3

diff -Nru a/udevsend.c b/udevsend.c
@@ -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);
 }

Kay Sievers

author

2004-02-24 23:10 UTC #1 lore raw

Quoted 10 lines · Kay Sievers

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

namedev.c +5 -5

diff -Nru a/namedev.c b/namedev.c
@@ -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);

namedev_parse.c +4 -4

diff -Nru a/namedev_parse.c b/namedev_parse.c
@@ -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);

udev-add.c +13 ~~-13~~

diff -Nru a/udev-add.c b/udev-add.c
@@ -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;

udev-remove.c +5 -5

diff -Nru a/udev-remove.c b/udev-remove.c
@@ -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);

udev.h +6 -0

diff -Nru a/udev.h b/udev.h
@@ -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);

udev_dbus.c +4 -4

diff -Nru a/udev_dbus.c b/udev_dbus.c
@@ -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",

udevdb.c +3 -3

diff -Nru a/udevdb.c b/udevdb.c
@@ -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;

udevinfo.c +5 -5

diff -Nru a/udevinfo.c b/udevinfo.c
@@ -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;

udevsend.c +3 -3

diff -Nru a/udevsend.c b/udevsend.c
@@ -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);
 }

Greg KH

Applied

2004-02-26 20:56 UTC #2 lore raw

In reply to Kay Sievers

Quoted 8 lines · Kay Sievers

Quoted 7 lines · Kay Sievers
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.

Quoted 6 lines

Quoted 3 lines
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.

Applied, thanks.

greg k-h

-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now. Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id56&alloc_id438&op=click
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel