Re: [PATCH v2 3/3] scandir: fix wrong assumption about errno [BZ #17804]

[Florian Weimer <fw at deneb dot enyo dot de>]

* Aurelien Jarno:

> malloc and realloc may set errno to ENOMEM even if they are successful.
> The scandir code wrongly assume that they do not change errno, this
> causes scandir to fail with ENOMEM even if malloc succeed.
>
> The code already handles that readdir might set errno by calling
> __set_errno (0) to clear the error. Move that part at the end of the
> loop to also take malloc and realloc into account.
>
> Changelog:
> 	[BZ #17804]
> 	* dirent/scandir-tail.c (SCANDIR_TAIL): Move __set_errno (0) at the
> 	end of the loop. Improve comments.

I think it would be clearer if the set-errno-to-zero logic would
happen directly before the READDIR call, where it is needed.
Something like this (untested):

diff --git a/dirent/scandir-tail.c b/dirent/scandir-tail.c
index 068c644c4e..4a6f93bde9 100644
--- a/dirent/scandir-tail.c
+++ b/dirent/scandir-tail.c
@@ -37,8 +37,11 @@ SCANDIR_TAIL (DIR *dp,
   if (dp == NULL)
     return -1;
 
+  /* errno is set to zero before the call to READDIR.  The original
+     errno value is restored in case of success, so that the caller
+     does not observe that the zero errno value, which is not
+     permitted by POSIX.  */
   int save = errno;
-  __set_errno (0);
 
   int result;
   struct scandir_cancel_struct c = { .dp = dp };
@@ -47,22 +50,41 @@ SCANDIR_TAIL (DIR *dp,
   DIRENT_TYPE **v = NULL;
   size_t vsize = 0;
   DIRENT_TYPE *d;
-  while ((d = READDIR (dp)) != NULL)
+  while (true)
     {
+      __set_errno (0);
+      DIRENT_TYPE *d = READDIR (dp);
+
+      /* Check if reading the directory is complete.  */
+      if (d == NULL)
+	{
+	  if (errno == 0)
+	    {
+	      /* The entire directory was read successfully.  */
+	      __closedir (dp);
+
+	      /* Sort the list if we have a comparison function to
+		 sort with.  */
+	      if (cmp != NULL)
+		qsort (v, c.cnt, sizeof *v, (__compar_fn_t) cmp);
+
+	      *namelist = v;
+	      result = c.cnt;
+	    }
+	  else /* errno != 0 */
+	    {
+	      __scandir_cancel_handler (&c);
+	      result = -1;
+	    }
+	  break;
+	} /* Directory reading completed.  */
+
       if (select != NULL)
         {
           int selected = (*select) (d);
-
-	  /* The SELECT function might have changed errno.  It was
-	     zero before and it need to be again to make the later
-	     tests work.  */
-	  __set_errno (0);
-
           if (!selected)
             continue;
         }
-      else
-        __set_errno (0);
 
       if (__glibc_unlikely (c.cnt == vsize))
         {
@@ -83,24 +105,6 @@ SCANDIR_TAIL (DIR *dp,
       v[c.cnt++] = (DIRENT_TYPE *) memcpy (vnew, d, dsize);
     }
 
-  if (__glibc_likely (errno == 0))
-    {
-      __closedir (dp);
-
-      /* Sort the list if we have a comparison function to sort with.  */
-      if (cmp != NULL)
-	qsort (v, c.cnt, sizeof *v, (__compar_fn_t) cmp);
-
-      *namelist = v;
-      result = c.cnt;
-    }
-  else
-    {
-      /* This frees everything and calls closedir.  */
-      __scandir_cancel_handler (&c);
-      result = -1;
-    }
-
   __libc_cleanup_pop (0);
 
   if (result >= 0)