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

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.
---
 ChangeLog             |  6 ++++++
 dirent/scandir-tail.c | 13 ++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

On Fri, Dec 22, 2017 at 08:31:01PM +0100, Aurelien Jarno wrote:
> 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.
> ---
>  ChangeLog             |  6 ++++++
>  dirent/scandir-tail.c | 13 ++++++++-----
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 8368b3006d..713c5ade6d 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2017-12-22  Aurelien Jarno  <aurelien@aurel32.net>
> +
> +	[BZ #17804]
> +	* dirent/scandir-tail.c (SCANDIR_TAIL): Move __set_errno (0) at the
> +	end of the loop. Improve comments.
> +
>  2017-12-22  Zack Weinberg  <zackw@panix.com>
>  
>  	[BZ #22615]
> diff --git a/dirent/scandir-tail.c b/dirent/scandir-tail.c
> index 068c644c4e..927791fad0 100644
> --- a/dirent/scandir-tail.c
> +++ b/dirent/scandir-tail.c
> @@ -53,16 +53,14 @@ SCANDIR_TAIL (DIR *dp,
>          {
>            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.  */
> +	  /* The SELECT function might have set errno to non-zero on
> +	     success.  It was zero before and it need to be again to

s/it need to be/it needs to be/

> +	     make the later tests work.  */
>  	  __set_errno (0);
>  
>            if (!selected)
>              continue;
>          }
> -      else
> -        __set_errno (0);
>  
>        if (__glibc_unlikely (c.cnt == vsize))
>          {
> @@ -81,6 +79,11 @@ SCANDIR_TAIL (DIR *dp,
>        if (vnew == NULL)
>          break;
>        v[c.cnt++] = (DIRENT_TYPE *) memcpy (vnew, d, dsize);
> +
> +      /* Ignore errors from readdir, malloc or realloc.  These functions
> +	 might have set errno to non-zero on success.  It was zero before
> +	 and it need to be again to make the latter tests work.  */

Likewise.

> +      __set_errno (0);
>      }
>  
>    if (__glibc_likely (errno == 0))

Besides these two typos in comments, the patch is fine.

* 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)