[PATCH 2/3] sysfs: sysfs_attr_get_value() writes to a buffer instead

[Alan Jenkins <alan-jenkins@tuffmail.co.uk>]

This avoids lifetime issues with the returned string, for subsequent changes
to caching in sysfs.  All callers are changed accordingly.  Many callers
copied the result to a buffer already, so this actually simplifies some code.

diff --git a/extras/usb_id/usb_id.c b/extras/usb_id/usb_id.c
index 92492c8..fd4d2f2 100644
--- a/extras/usb_id/usb_id.c
+++ b/extras/usb_id/usb_id.c
@@ -224,9 +224,11 @@ static int usb_id(const char *devpath)
 	struct sysfs_device *dev;
 	struct sysfs_device *dev_interface;
 	struct sysfs_device *dev_usb;
-	const char *if_class, *if_subclass;
+	char if_class[NAME_SIZE];
+	char if_subclass[NAME_SIZE];
 	int if_class_num;
 	int protocol = 0;
+	int status;
 
 	dbg("devpath %s\n", devpath);
 
@@ -244,15 +246,17 @@ static int usb_id(const char *devpath)
 		return 1;
 	}
 
-	if_class = sysfs_attr_get_value(dev_interface->devpath, "bInterfaceClass");
-	if (!if_class) {
+	status = sysfs_attr_get_value(dev_interface->devpath, "bInterfaceClass",
+				      if_class, sizeof(if_class));
+	if (!status) {
 		info("%s: cannot get bInterfaceClass attribute\n", dev_interface->kernel);
 		return 1;
 	}
 	if_class_num = strtoul(if_class, NULL, 16);
 	if (if_class_num = 8) {
-		if_subclass = sysfs_attr_get_value(dev_interface->devpath, "bInterfaceSubClass");
-		if (if_subclass != NULL)
+		status = sysfs_attr_get_value(dev_interface->devpath, "bInterfaceSubClass",
+					      if_subclass, sizeof(if_subclass));
+		if (!status)
 			protocol = set_usb_mass_storage_ifsubtype(type_str, if_subclass, sizeof(type_str)-1);
 	} else
 		set_usb_iftype(type_str, if_class_num, sizeof(type_str)-1);
@@ -269,7 +273,10 @@ static int usb_id(const char *devpath)
 	/* mass storage */
 	if (protocol = 6 && !use_usb_info) {
 		struct sysfs_device *dev_scsi;
-		const char *scsi_model, *scsi_vendor, *scsi_type, *scsi_rev;
+		char scsi_model[NAME_SIZE];
+		char scsi_vendor[NAME_SIZE];
+		char scsi_type[NAME_SIZE];
+		char scsi_rev[NAME_SIZE];
 		int host, bus, target, lun;
 
 		/* get scsi device */
@@ -284,29 +291,33 @@ static int usb_id(const char *devpath)
 		}
 
 		/* Generic SPC-2 device */
-		scsi_vendor = sysfs_attr_get_value(dev_scsi->devpath, "vendor");
-		if (!scsi_vendor) {
+		status = sysfs_attr_get_value(dev_scsi->devpath, "vendor",
+					      scsi_vendor, sizeof(scsi_vendor));
+		if (!status) {
 			info("%s: cannot get SCSI vendor attribute\n", dev_scsi->kernel);
 			goto fallback;
 		}
 		set_str(vendor_str, scsi_vendor, sizeof(vendor_str)-1);
 
-		scsi_model = sysfs_attr_get_value(dev_scsi->devpath, "model");
-		if (!scsi_model) {
+		status = sysfs_attr_get_value(dev_scsi->devpath, "model",
+					      scsi_model, sizeof(scsi_model));
+		if (!status) {
 			info("%s: cannot get SCSI model attribute\n", dev_scsi->kernel);
 			goto fallback;
 		}
 		set_str(model_str, scsi_model, sizeof(model_str)-1);
 
-		scsi_type = sysfs_attr_get_value(dev_scsi->devpath, "type");
-		if (!scsi_type) {
+		status = sysfs_attr_get_value(dev_scsi->devpath, "type",
+					      scsi_type, sizeof(scsi_type));
+		if (!status) {
 			info("%s: cannot get SCSI type attribute\n", dev_scsi->kernel);
 			goto fallback;
 		}
 		set_scsi_type(type_str, scsi_type, sizeof(type_str)-1);
 
-		scsi_rev = sysfs_attr_get_value(dev_scsi->devpath, "rev");
-		if (!scsi_rev) {
+		status = sysfs_attr_get_value(dev_scsi->devpath, "rev",
+					      scsi_rev, sizeof(scsi_rev));
+		if (!status) {
 			info("%s: cannot get SCSI revision attribute\n", dev_scsi->kernel);
 			goto fallback;
 		}
@@ -322,15 +333,18 @@ static int usb_id(const char *devpath)
 fallback:
 	/* fallback to USB vendor & device */
 	if (vendor_str[0] = '\0') {
-		const char *usb_vendor = NULL;
+		char usb_vendor[NAME_SIZE];
 
+		status = 0;
 		if (!use_num_info)
-			usb_vendor = sysfs_attr_get_value(dev_usb->devpath, "manufacturer");
+			status = sysfs_attr_get_value(dev_usb->devpath, "manufacturer",
+						      usb_vendor, sizeof(usb_vendor));
 
-		if (!usb_vendor)
-			usb_vendor = sysfs_attr_get_value(dev_usb->devpath, "idVendor");
+		if (!status)
+			status = sysfs_attr_get_value(dev_usb->devpath, "idVendor",
+						      usb_vendor, sizeof(usb_vendor));
 
-		if (!usb_vendor) {
+		if (!status) {
 			info("No USB vendor information available\n");
 			return 1;
 		}
@@ -338,15 +352,18 @@ fallback:
 	}
 
 	if (model_str[0] = '\0') {
-		const char *usb_model = NULL;
+		char usb_model[NAME_SIZE];
 
+		status = 0;
 		if (!use_num_info)
-			usb_model = sysfs_attr_get_value(dev_usb->devpath, "product");
+			status = sysfs_attr_get_value(dev_usb->devpath, "product",
+						      usb_model, sizeof(usb_model));
 
-		if (!usb_model)
-			usb_model = sysfs_attr_get_value(dev_usb->devpath, "idProduct");
+		if (!status)
+			status = sysfs_attr_get_value(dev_usb->devpath, "idProduct",
+						      usb_model, sizeof(usb_model));
 
-		if (!usb_model) {
+		if (!status) {
 			dbg("No USB model information available\n");
 			return 1;
 		}
@@ -354,18 +371,20 @@ fallback:
 	}
 
 	if (revision_str[0] = '\0') {
-		const char *usb_rev;
+		char usb_rev[NAME_SIZE];
 
-		usb_rev = sysfs_attr_get_value(dev_usb->devpath, "bcdDevice");
-		if (usb_rev)
+		status = sysfs_attr_get_value(dev_usb->devpath, "bcdDevice",
+					      usb_rev, sizeof(usb_rev));
+		if (status)
 			set_str(revision_str, usb_rev, sizeof(revision_str)-1);
 	}
 
 	if (serial_str[0] = '\0') {
-		const char *usb_serial;
+		char usb_serial[NAME_SIZE];
 
-		usb_serial = sysfs_attr_get_value(dev_usb->devpath, "serial");
-		if (usb_serial)
+		status = sysfs_attr_get_value(dev_usb->devpath, "serial",
+					      usb_serial, sizeof(usb_serial));
+		if (status)
 			set_str(serial_str, usb_serial, sizeof(serial_str)-1);
 	}
 	return 0;
diff --git a/udev/udev.h b/udev/udev.h
index da86365..28b59db 100644
--- a/udev/udev.h
+++ b/udev/udev.h
@@ -117,7 +117,7 @@ extern void sysfs_device_set_values(struct sysfs_device *dev, const char *devpat
 extern struct sysfs_device *sysfs_device_get(const char *devpath);
 extern struct sysfs_device *sysfs_device_get_parent(struct sysfs_device *dev);
 extern struct sysfs_device *sysfs_device_get_parent_with_subsystem(struct sysfs_device *dev, const char *subsystem);
-extern char *sysfs_attr_get_value(const char *devpath, const char *attr_name);
+extern int sysfs_attr_get_value(const char *devpath, const char *attr_name, char *value, size_t len);
 extern int sysfs_resolve_link(char *path, size_t size);
 extern int sysfs_lookup_devpath_by_subsys_id(char *devpath, size_t len, const char *subsystem, const char *id);
 
diff --git a/udev/udev_device.c b/udev/udev_device.c
index 130c714..13000b0 100644
--- a/udev/udev_device.c
+++ b/udev/udev_device.c
@@ -71,12 +71,11 @@ void udev_device_cleanup(struct udevice *udev)
 
 dev_t udev_device_get_devt(struct udevice *udev)
 {
-	const char *attr;
+	char attr[NAME_SIZE];
 	unsigned int maj, min;
 
 	/* read it from sysfs  */
-	attr = sysfs_attr_get_value(udev->dev->devpath, "dev");
-	if (attr != NULL) {
+	if (sysfs_attr_get_value(udev->dev->devpath, "dev", attr, sizeof(attr))) {
 		if (sscanf(attr, "%u:%u", &maj, &min) = 2)
 			return makedev(maj, min);
 	}
diff --git a/udev/udev_node.c b/udev/udev_node.c
index 33183c8..a29c4b7 100644
--- a/udev/udev_node.c
+++ b/udev/udev_node.c
@@ -376,12 +376,11 @@ int udev_node_add(struct udevice *udev)
 	/* create all_partitions if requested */
 	if (udev->partitions) {
 		char partitionname[PATH_SIZE];
-		char *attr;
+		char attr[NAME_SIZE];
 		int range;
 
 		/* take the maximum registered minor range */
-		attr = sysfs_attr_get_value(udev->dev->devpath, "range");
-		if (attr != NULL) {
+		if (sysfs_attr_get_value(udev->dev->devpath, "range", attr, sizeof(attr))) {
 			range = atoi(attr);
 			if (range > 1)
 				udev->partitions = range-1;
diff --git a/udev/udev_rules.c b/udev/udev_rules.c
index f7acd9c..53c483a 100644
--- a/udev/udev_rules.c
+++ b/udev/udev_rules.c
@@ -829,40 +829,40 @@ found:
 			else {
 				char devpath[PATH_SIZE];
 				char *attrib;
-				const char *value = NULL;
 				size_t size;
+				int found = 0;
 
 				if (attr_get_by_subsys_id(attr, devpath, sizeof(devpath), &attrib)) {
 					if (attrib != NULL)
-						value = sysfs_attr_get_value(devpath, attrib);
+						found = sysfs_attr_get_value(devpath, attrib,
+									     temp2, sizeof(temp2));
 					else
 						break;
 				}
 
 				/* try the current device, other matches may have selected */
-				if (value = NULL && udev->dev_parent != NULL && udev->dev_parent != udev->dev)
-					value = sysfs_attr_get_value(udev->dev_parent->devpath, attr);
-
+				if (!found && udev->dev_parent != NULL && udev->dev_parent != udev->dev)
+					found = sysfs_attr_get_value(udev->dev_parent->devpath, attr,
+								     temp2, sizeof(temp2));
 				/* look at all devices along the chain of parents */
-				if (value = NULL) {
+				if (!found) {
 					struct sysfs_device *dev_parent = udev->dev;
 
 					do {
 						dbg("looking at '%s'\n", dev_parent->devpath);
-						value = sysfs_attr_get_value(dev_parent->devpath, attr);
-						if (value != NULL)
+						found = sysfs_attr_get_value(dev_parent->devpath, attr,
+									     temp2, sizeof(temp2));
+						if (found)
 							break;
 						dev_parent = sysfs_device_get_parent(dev_parent);
 					} while (dev_parent != NULL);
 				}
 
-				if (value = NULL)
+				if (!found)
 					break;
 
 				/* strip trailing whitespace, and replace unwanted characters */
-				size = strlcpy(temp2, value, sizeof(temp2));
-				if (size >= sizeof(temp2))
-					size = sizeof(temp2)-1;
+				size = strlen(temp2);
 				while (size > 0 && isspace(temp2[size-1]))
 					temp2[--size] = '\0';
 				count = replace_chars(temp2, ALLOWED_CHARS_INPUT);
@@ -1136,21 +1136,22 @@ static int match_rule(struct udevice *udev, struct udev_rule *rule)
 			const char *key_value = key_val(rule, &pair->key);
 			char devpath[PATH_SIZE];
 			char *attrib;
-			const char *value = NULL;
 			char val[VALUE_SIZE];
 			size_t len;
+			int found = 0;
 
 			if (attr_get_by_subsys_id(key_name, devpath, sizeof(devpath), &attrib)) {
 				if (attrib != NULL)
-					value = sysfs_attr_get_value(devpath, attrib);
+					found = sysfs_attr_get_value(devpath, attrib,
+								     val, sizeof(val));
 				else
 					goto nomatch;
 			}
-			if (value = NULL)
-				value = sysfs_attr_get_value(udev->dev->devpath, key_name);
-			if (value = NULL)
+			if (!found)
+				found = sysfs_attr_get_value(udev->dev->devpath, key_name,
+							     val, sizeof(val));
+			if (!found)
 				goto nomatch;
-			strlcpy(val, value, sizeof(val));
 
 			/* strip trailing whitespace of value, if not asked to match for it */
 			len = strlen(key_value);
@@ -1189,16 +1190,17 @@ static int match_rule(struct udevice *udev, struct udev_rule *rule)
 			    pair->key.operation = KEY_OP_NOMATCH) {
 				const char *key_name = key_pair_name(rule, pair);
 				const char *key_value = key_val(rule, &pair->key);
-				const char *value;
 				char val[VALUE_SIZE];
 				size_t len;
-
-				value = sysfs_attr_get_value(udev->dev_parent->devpath, key_name);
-				if (value = NULL)
-					value = sysfs_attr_get_value(udev->dev->devpath, key_name);
-				if (value = NULL)
+				int found;
+
+				found = sysfs_attr_get_value(udev->dev_parent->devpath, key_name,
+							     val, sizeof(val));
+				if (!found)
+					found = sysfs_attr_get_value(udev->dev->devpath, key_name,
+								     val, sizeof(val));
+				if (!found)
 					goto try_parent;
-				strlcpy(val, value, sizeof(val));
 
 				/* strip trailing whitespace of value, if not asked to match for it */
 				len = strlen(key_value);
diff --git a/udev/udev_sysfs.c b/udev/udev_sysfs.c
index 91f5997..2d2457f 100644
--- a/udev/udev_sysfs.c
+++ b/udev/udev_sysfs.c
@@ -174,11 +174,11 @@ struct sysfs_device *sysfs_device_get(const char *devpath)
 
 	/* look for device already in cache (we never put an untranslated path in the cache) */
 	list_for_each_entry(dev_loop, &dev_list, node) {
-		if (strcmp(dev_loop->devpath, devpath_real) = 0) {
-			dbg("found in cache '%s'\n", dev_loop->devpath);
-			return dev_loop;
+			if (strcmp(dev_loop->devpath, devpath_real) = 0) {
+				dbg("found in cache '%s'\n", dev_loop->devpath);
+				return dev_loop;
+			}
 		}
-	}
 
 	/* if we got a link, resolve it to the real device */
 	strlcpy(path, sysfs_path, sizeof(path));
@@ -193,12 +193,12 @@ struct sysfs_device *sysfs_device_get(const char *devpath)
 
 		/* now look for device in cache after path translation */
 		list_for_each_entry(dev_loop, &dev_list, node) {
-			if (strcmp(dev_loop->devpath, devpath_real) = 0) {
-				dbg("found in cache '%s'\n", dev_loop->devpath);
-				return dev_loop;
+				if (strcmp(dev_loop->devpath, devpath_real) = 0) {
+					dbg("found in cache '%s'\n", dev_loop->devpath);
+					return dev_loop;
+				}
 			}
 		}
-	}
 
 	/* it is a new device */
 	dbg("new uncached device '%s'\n", devpath_real);
@@ -323,11 +323,11 @@ struct sysfs_device *sysfs_device_get_parent_with_subsystem(struct sysfs_device
 	return NULL;
 }
 
-char *sysfs_attr_get_value(const char *devpath, const char *attr_name)
+int sysfs_attr_get_value(const char *devpath, const char *attr_name, char *value, size_t len)
 {
 	char path_full[PATH_SIZE];
 	const char *path;
-	char value[NAME_SIZE];
+	char val[NAME_SIZE];
 	struct sysfs_attr *attr_loop;
 	struct sysfs_attr *attr;
 	struct stat statbuf;
@@ -346,76 +346,85 @@ char *sysfs_attr_get_value(const char *devpath, const char *attr_name)
 
 	/* look for attribute in cache */
 	list_for_each_entry(attr_loop, &attr_list, node) {
-		if (strcmp(attr_loop->path, path) = 0) {
-			dbg("found in cache '%s'\n", attr_loop->path);
-			return attr_loop->value;
+			if (strcmp(attr_loop->path, path) = 0) {
+				dbg("found in cache '%s'\n", attr_loop->path);
+				if (!attr_loop->value)
+					return 0;
+
+				attr = attr_loop;
+				goto out;
+			}
 		}
-	}
 
-	/* store attribute in cache (also negatives are kept in cache) */
-	dbg("new uncached attribute '%s'\n", path_full);
-	attr = malloc(sizeof(struct sysfs_attr));
-	if (attr = NULL)
-		return NULL;
-	memset(attr, 0x00, sizeof(struct sysfs_attr));
-	strlcpy(attr->path, path, sizeof(attr->path));
-	dbg("add to cache '%s'\n", path_full);
+		/* store attribute in cache (also negatives are kept in cache) */
+		dbg("new uncached attribute '%s'\n", path_full);
+		attr = malloc(sizeof(struct sysfs_attr));
+		if (attr = NULL)
+			return 0;
+		memset(attr, 0x00, sizeof(struct sysfs_attr));
+		strlcpy(attr->path, path, sizeof(attr->path));
+		dbg("add to cache '%s'\n", path_full);
 	list_add(&attr->node, &attr_list);
 
 	if (lstat(path_full, &statbuf) != 0) {
 		dbg("stat '%s' failed: %s\n", path_full, strerror(errno));
-		goto out;
+		return 0;
 	}
 
 	if (S_ISLNK(statbuf.st_mode)) {
 		/* links return the last element of the target path */
 		char link_target[PATH_SIZE];
-		int len;
 		const char *pos;
 
-		len = readlink(path_full, link_target, sizeof(link_target));
-		if (len > 0) {
-			link_target[len] = '\0';
-			pos = strrchr(link_target, '/');
-			if (pos != NULL) {
-				dbg("cache '%s' with link value '%s'\n", path_full, value);
-				strlcpy(attr->value_local, &pos[1], sizeof(attr->value_local));
-				attr->value = attr->value_local;
-			}
-		}
+		size = readlink(path_full, link_target, sizeof(link_target));
+		if (size < 0)
+			return 0;
+		if (size = sizeof(link_target))
+			return 0;
+
+		link_target[size] = '\0';
+		pos = strrchr(link_target, '/');
+		if (pos = NULL)
+			return 0;
+
+		strlcpy(val, &pos[1], sizeof(val));
 		goto out;
 	}
 
 	/* skip directories */
 	if (S_ISDIR(statbuf.st_mode))
-		goto out;
+		return 0;
 
 	/* skip non-readable files */
 	if ((statbuf.st_mode & S_IRUSR) = 0)
-		goto out;
+		return 0;
 
 	/* read attribute value */
 	fd = open(path_full, O_RDONLY);
 	if (fd < 0) {
 		dbg("attribute '%s' can not be opened\n", path_full);
-		goto out;
+		return 0;
 	}
-	size = read(fd, value, sizeof(value));
+	size = read(fd, val, sizeof(val));
 	close(fd);
 	if (size < 0)
-		goto out;
-	if (size = sizeof(value))
-		goto out;
+		return 0;
+	if (size = sizeof(val))
+		return 0;
 
 	/* got a valid value, store and return it */
-	value[size] = '\0';
-	remove_trailing_chars(value, '\n');
-	dbg("cache '%s' with attribute value '%s'\n", path_full, value);
-	strlcpy(attr->value_local, value, sizeof(attr->value_local));
-	attr->value = attr->value_local;
+	val[size] = '\0';
+	remove_trailing_chars(val, '\n');
 
 out:
-	return attr->value;
+	strlcpy(attr->value_local, val, sizeof(attr->value_local));
+	attr->value = attr->value_local;
+	dbg("cache '%s' with link value '%s'\n", path_full, attr->value);
+
+	if (strlcpy(value, val, len) > len-1)
+		return 0;
+
+	return 1;
 }
 
 int sysfs_lookup_devpath_by_subsys_id(char *devpath_full, size_t len, const char *subsystem, const char *id)
diff --git a/udev/udevinfo.c b/udev/udevinfo.c
index 9b75364..b76d11d 100644
--- a/udev/udevinfo.c
+++ b/udev/udevinfo.c
@@ -47,7 +47,6 @@ static void print_all_attributes(const char *devpath, const char *key)
 		for (dent = readdir(dir); dent != NULL; dent = readdir(dir)) {
 			struct stat statbuf;
 			char filename[PATH_SIZE];
-			char *attr_value;
 			char value[NAME_SIZE];
 			size_t len;
 
@@ -67,12 +66,9 @@ static void print_all_attributes(const char *devpath, const char *key)
 			if (S_ISLNK(statbuf.st_mode))
 				continue;
 
-			attr_value = sysfs_attr_get_value(devpath, dent->d_name);
-			if (attr_value = NULL)
+			if (!sysfs_attr_get_value(devpath, dent->d_name, value, sizeof(value)))
 				continue;
-			len = strlcpy(value, attr_value, sizeof(value));
-			if(len >= sizeof(value))
-				len = sizeof(value) - 1;
+			len = strlen(value);
 			dbg("attr '%s'='%s'(%zi)\n", dent->d_name, value, len);
 
 			/* skip nonprintable attributes */