Re: scandir leak patch

[Jeff Johnston <jjohnstn at redhat dot com>]

Joel Sherrill wrote:
> Attached is the current newlib scandir.c with
> the "goto cleanup_and_bail" modification from
> the RTEMS one plus an explicit check for 0
> entries from the fstat.
>
> How does this look?
>
> 2008-11-24  Joel Sherrill <joel.sherrill@oarcorp.com>
>
>    * libc/posix/scandir.c: Fix memory leaks.
Hi Joel,

 Thanks.  I did some cleanup of your patch.  First of all you should 
have been using reallocf instead of realloc
since you reset the input pointer.  Secondly I wasn't thrilled with the 
cleanup_and_bail section occurring after the normal return so I added a 
flag to indicate "if successful"  and used it so there would only be the 
one return statement.  Let me know if I missed anything or whether you 
have any problems with the modifications.

 I have attached a new diff which would be applied to the original 
scandir.c.

-- Jeff J.
Index: scandir.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/posix/scandir.c,v
retrieving revision 1.6
diff -u -p -r1.6 scandir.c
--- scandir.c	31 Oct 2008 21:03:41 -0000	1.6
+++ scandir.c	24 Nov 2008 20:01:35 -0000
@@ -78,18 +78,25 @@ _DEFUN(scandir, (dirname, namelist, sele
 	struct stat stb;
 	long arraysz;
 	DIR *dirp;
+	int successful = 0;
+	int rc = 0;
+
+	dirp = NULL;
+	names = NULL;
 
 	if ((dirp = opendir(dirname)) == NULL)
 		return(-1);
 #ifdef HAVE_DD_LOCK
 	__lock_acquire_recursive(dirp->dd_lock);
 #endif
-	if (fstat(dirp->dd_fd, &stb) < 0) {
-#ifdef HAVE_DD_LOCK
-		__lock_release_recursive(dirp->dd_lock);
-#endif
-		return(-1);
-	}
+	if (fstat(dirp->dd_fd, &stb) < 0)
+		goto cleanup;
+
+	/*
+ 	 * If there were no directory entries, then bail.
+ 	 */
+	if (stb.st_size == 0)
+		goto cleanup;
 
 	/*
 	 * estimate the array size by taking the size of the directory file
@@ -97,12 +104,8 @@ _DEFUN(scandir, (dirname, namelist, sele
 	 */
 	arraysz = (stb.st_size / 24);
 	names = (struct dirent **)malloc(arraysz * sizeof(struct dirent *));
-	if (names == NULL) {
-#ifdef HAVE_DD_LOCK
-		__lock_release_recursive(dirp->dd_lock);
-#endif
-		return(-1);
-	}
+	if (names == NULL)
+		goto cleanup;
 
 	nitems = 0;
 	while ((d = readdir(dirp)) != NULL) {
@@ -112,12 +115,8 @@ _DEFUN(scandir, (dirname, namelist, sele
 		 * Make a minimum size copy of the data
 		 */
 		p = (struct dirent *)malloc(DIRSIZ(d));
-		if (p == NULL) {
-#ifdef HAVE_DD_LOCK
-			__lock_release_recursive(dirp->dd_lock);
-#endif
-			return(-1);
-		}
+		if (p == NULL)
+			goto cleanup;
 		p->d_ino = d->d_ino;
 		p->d_reclen = d->d_reclen;
 #ifdef _DIRENT_HAVE_D_NAMLEN
@@ -131,32 +130,38 @@ _DEFUN(scandir, (dirname, namelist, sele
 		 * realloc the maximum size.
 		 */
 		if (++nitems >= arraysz) {
-			if (fstat(dirp->dd_fd, &stb) < 0) {
-#ifdef HAVE_DD_LOCK
-				__lock_release_recursive(dirp->dd_lock);
-#endif
-				return(-1);	/* just might have grown */
-			}
+			if (fstat(dirp->dd_fd, &stb) < 0)
+				goto cleanup;
 			arraysz = stb.st_size / 12;
-			names = (struct dirent **)realloc((char *)names,
+			names = (struct dirent **)reallocf((char *)names,
 				arraysz * sizeof(struct dirent *));
-			if (names == NULL) {
-#ifdef HAVE_DD_LOCK
-				__lock_release_recursive(dirp->dd_lock);
-#endif
-				return(-1);
-			}
+			if (names == NULL)
+				goto cleanup;
 		}
 		names[nitems-1] = p;
 	}
+	successful = 1;
+cleanup:
 	closedir(dirp);
-	if (nitems && dcomp != NULL)
-		qsort(names, nitems, sizeof(struct dirent *), (void *)dcomp);
-	*namelist = names;
+	if (successful) {
+		if (nitems && dcomp != NULL)
+			qsort(names, nitems, sizeof(struct dirent *), (void *)dcomp);
+		*namelist = names;
+		rc = nitems;
+	} else {  /* We were unsuccessful, clean up storage and return -1.  */
+		if ( names ) {
+			int i;
+			for (i=0; i < nitems; i++ )
+				free( names[i] );
+			free( names );
+		}
+		rc = -1;
+	}
+
 #ifdef HAVE_DD_LOCK
 	__lock_release_recursive(dirp->dd_lock);
 #endif
-	return(nitems);
+	return(rc);
 }
 
 /*