Han Boetes <hboetes@gmail.com> wrote:
> While reviewing echo.c I noticed that copy_list() carefully handles 
> malloc() failure with a full cleanup and return NULL, but the strdup() 
> call two lines below it has no error check. On OOM, this would result in 
> a NULL l_name being silently added to the list, potentially causing a 
> NULL dereference in the caller. The following patch makes the error 
> handling consistent.
> 
> No idea how to trigger it, but it sure looks better.

Good catch!  I've taken the liberty to rework the diff a little bit,
moving the freeing under a `goto fail' to dedup it a bit.

This is the diff that I've actually committed.

diff -s /home/op/w/mg
path + /home/op/w/mg (staged changes)
commit - bb4e7911d276b83dbf547938ad8bc12ff94c2d0e
blob - 9b90aaa5ef72752b4736b54829ff68fbf4567268
blob + 655c743a31becc116b6f800864aa64a61d612796
--- echo.c
+++ echo.c
@@ -1002,19 +1002,24 @@ copy_list(struct list *lp)
 	last = NULL;
 	while (lp) {
 		current = malloc(sizeof(struct list));
-		if (current == NULL) {
-			/* Free what we have allocated so far */
-			for (current = last; current; current = nxt) {
-				nxt = current->l_next;
-				free(current->l_name);
-				free(current);
-			}
-			return (NULL);
-		}
-		current->l_next = last;
+		if (current == NULL)
+			goto fail;
 		current->l_name = strdup(lp->l_name);
+		if (current->l_name == NULL) {
+			free(current);
+			goto fail;
+		}
+		current->l_next = last;
 		last = current;
 		lp = lp->l_next;
 	}
 	return (last);
+
+ fail:
+	for (current = last; current; current = nxt) {
+		nxt = current->l_next;
+		free(current->l_name);
+		free(current);
+	}
+	return (NULL);
 }


Thanks!
Omar Polo