[PATCH] Fix dangling pointer returned by attr_get_by_subsys_id()

[PATCH] Fix dangling pointer returned by attr_get_by_subsys_id()

[Alan Jenkins]

Temporary stack buffers should be allocated by caller, not callee :-).

After uncommenting the valgrind line in udev-test.pl:

./udev-test.pl 147
udev-test will run test number 147 only:

TEST 147: magic subsys/kernel lookup
device '/block/sda' expecting node '00:e0:00:fb:04:e1'
=18218= Memcheck, a memory error detector.
=18218= Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al.
=18218= Using LibVEX rev 1804, a library for dynamic binary translation.
=18218= Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP.
=18218= Using valgrind-3.3.0-Debian, a dynamic binary instrumentation framework.
=18218= Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al.
=18218= For more details, rerun with: -v
=18218=
=18218= Conditional jump or move depends on uninitialised value(s)
=18218=    at 0x40CF41: strlcat (udev_sysdeps.c:60)
=18218=    by 0x40DE5F: sysfs_attr_get_value (udev_sysfs.c:345)
=18218=    by 0x4081B4: udev_rules_apply_format (udev_rules.c:835)
=18218=    by 0x40A442: udev_rules_get_name (udev_rules.c:1497)
=18218=    by 0x403FB8: udev_device_event (udev_device_event.c:138)
=18218=    by 0x4104BC: main (test-udev.c:157)

(and more with same backtrace)

=18218= Conditional jump or move depends on uninitialised value(s)
=18218=    at 0x40CE84: strlcpy (udev_sysdeps.c:34)
=18218=    by 0x40DF3D: sysfs_attr_get_value (udev_sysfs.c:361)
...

=18218= Syscall param lstat(file_name) points to uninitialised byte(s)
=18218=    at 0x510BB15: _lxstat (lxstat.c:38)
=18218=    by 0x40DF61: sysfs_attr_get_value (udev_sysfs.c:365)
=18218=  Address 0x7feffe703 is on thread 1's stack
...

=18218= Syscall param open(filename) points to uninitialised byte(s)
=18218=    at 0x510C330: __open_nocancel (in /usr/lib/debug/libc-2.7.so)
=18218=    by 0x40E063: sysfs_attr_get_value (udev_sysfs.c:398)
...
=18218=  Address 0x7feffe703 is on thread 1's stack

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

diff --git a/udev/udev_rules.c b/udev/udev_rules.c
index 693bce2..cc0dbbb 100644
--- a/udev/udev_rules.c
+++ b/udev/udev_rules.c
@@ -566,7 +566,7 @@ static int wait_for_file(struct udevice *udev, const char *file, int timeout)
 }
 
 /* handle "[$SUBSYSTEM/$KERNEL]<attribute>" lookup */
-static int attr_get_by_subsys_id(const char *attrstr, char *devpath, size_t len, char **attr)
+static int attr_get_by_subsys_id(const char *attrstr, char *devpath, size_t pathlen, char *attr, size_t attrlen)
 {
 	char subsys[NAME_SIZE];
 	char *attrib;
@@ -590,12 +590,9 @@ static int attr_get_by_subsys_id(const char *attrstr, char *devpath, size_t len,
 	id[0] = '\0';
 	id = &id[1];
 
-	if (sysfs_lookup_devpath_by_subsys_id(devpath, len, subsys, id)) {
+	if (sysfs_lookup_devpath_by_subsys_id(devpath, pathlen, subsys, id)) {
 		if (attr != NULL) {
-			if (attrib[0] != '\0')
-				*attr = attrib;
-			else
-				*attr = NULL;
+			strlcpy(attr, attrib, attrlen);
 		}
 		found = 1;
 	}
@@ -826,12 +823,12 @@ found:
 				err("missing file parameter for attr\n");
 			else {
 				char devpath[PATH_SIZE];
-				char *attrib;
+				char attrib[NAME_SIZE];
 				const char *value = NULL;
 				size_t size;
 
-				if (attr_get_by_subsys_id(attr, devpath, sizeof(devpath), &attrib)) {
-					if (attrib != NULL)
+				if (attr_get_by_subsys_id(attr, devpath, sizeof(devpath), attrib, sizeof(attrib))) {
+					if (attrib[0] != '\0')
 						value = sysfs_attr_get_value(devpath, attrib);
 					else
 						break;
@@ -1074,17 +1071,17 @@ static int match_rule(struct udevice *udev, struct udev_rule *rule)
 	    rule->test.operation = KEY_OP_NOMATCH) {
 		char filename[PATH_SIZE];
 		char devpath[PATH_SIZE];
-		char *attr;
+		char attr[NAME_SIZE];
 		struct stat statbuf;
 		int match;
 
 		strlcpy(filename, key_val(rule, &rule->test), sizeof(filename));
 		udev_rules_apply_format(udev, filename, sizeof(filename));
 
-		if (attr_get_by_subsys_id(filename, devpath, sizeof(devpath), &attr)) {
+		if (attr_get_by_subsys_id(filename, devpath, sizeof(devpath), attr, sizeof(attr))) {
 			strlcpy(filename, sysfs_path, sizeof(filename));
 			strlcat(filename, devpath, sizeof(filename));
-			if (attr != NULL) {
+			if (attr[0] != '\0') {
 				strlcat(filename, "/", sizeof(filename));
 				strlcat(filename, attr, sizeof(filename));
 			}
@@ -1135,13 +1132,13 @@ static int match_rule(struct udevice *udev, struct udev_rule *rule)
 			const char *key_name = key_pair_name(rule, pair);
 			const char *key_value = key_val(rule, &pair->key);
 			char devpath[PATH_SIZE];
-			char *attrib;
+			char attrib[NAME_SIZE];
 			const char *value = NULL;
 			char val[VALUE_SIZE];
 			size_t len;
 
-			if (attr_get_by_subsys_id(key_name, devpath, sizeof(devpath), &attrib)) {
-				if (attrib != NULL)
+			if (attr_get_by_subsys_id(key_name, devpath, sizeof(devpath), attrib, sizeof(attrib))) {
+				if (attrib[0] != '\0')
 					value = sysfs_attr_get_value(devpath, attrib);
 				else
 					goto nomatch;
@@ -1324,13 +1321,13 @@ try_parent:
 		if (pair->key.operation = KEY_OP_ASSIGN) {
 			const char *key_name = key_pair_name(rule, pair);
 			char devpath[PATH_SIZE];
-			char *attrib;
+			char attrib[NAME_SIZE];
 			char attr[PATH_SIZE] = "";
 			char value[NAME_SIZE];
 			FILE *f;
 
-			if (attr_get_by_subsys_id(key_name, devpath, sizeof(devpath), &attrib)) {
-				if (attrib != NULL) {
+			if (attr_get_by_subsys_id(key_name, devpath, sizeof(devpath), attrib, sizeof(attrib))) {
+				if (attrib[0] != '\0') {
 					strlcpy(attr, sysfs_path, sizeof(attr));
 					strlcat(attr, devpath, sizeof(attr));
 					strlcat(attr, "/", sizeof(attr));

Re: [PATCH] Fix dangling pointer returned by attr_get_by_subsys_id()

[Kay Sievers]

On Sun, Aug 31, 2008 at 19:15, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
> Temporary stack buffers should be allocated by caller, not callee :-).
>
>  /* handle "[$SUBSYSTEM/$KERNEL]<attribute>" lookup */
> -static int attr_get_by_subsys_id(const char *attrstr, char *devpath, size_t len, char **attr)

Ah, attr is supposed to return the position in attstr. Should be fixed now.

Thanks,
Kay