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

Message ID 20171221225453.13158-4-aurelien@aurel32.net
State New, archived
Headers

Commit Message

Aurelien Jarno Dec. 21, 2017, 10:54 p.m. UTC
  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.
---
 ChangeLog             | 6 ++++++
 dirent/scandir-tail.c | 7 +++++--
 2 files changed, 11 insertions(+), 2 deletions(-)
  

Comments

Carlos O'Donell Dec. 22, 2017, 3:27 a.m. UTC | #1
On 12/21/2017 02:54 PM, 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.

OK with changes below.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  ChangeLog             | 6 ++++++
>  dirent/scandir-tail.c | 7 +++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 7bf30d27ef..31fddf5794 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2017-12-21  Aurelien Jarno  <aurelien@aurel32.net>
> +
> +	[BZ #17804]
> +	* dirent/scandir-tail.c (SCANDIR_TAIL): Move __set_errno (0) at the
> +	end of the loop.
> +
>  2017-12-21  Aurelien Jarno  <aurelien@aurel32.net>
>  
>  	[BZ #22615]
> diff --git a/dirent/scandir-tail.c b/dirent/scandir-tail.c
> index 068c644c4e..00fe9ef030 100644
> --- a/dirent/scandir-tail.c
> +++ b/dirent/scandir-tail.c
> @@ -61,8 +61,6 @@ SCANDIR_TAIL (DIR *dp,
>            if (!selected)
>              continue;
>          }
> -      else
> -        __set_errno (0);

OK.

>  
>        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 changed errno.  It was zero before and it need to be
> +	 again to make the latter tests work.  */

Suggest:

/* Ignore error from readdir, malloc, or realloc.  These functions
   might have set errno to non-zero on success.  It was zero before
   and it needs to be again to make the later tests work.  */

Please also change the wording of the matching comment on line 56.

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

Patch

diff --git a/ChangeLog b/ChangeLog
index 7bf30d27ef..31fddf5794 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@ 
+2017-12-21  Aurelien Jarno  <aurelien@aurel32.net>
+
+	[BZ #17804]
+	* dirent/scandir-tail.c (SCANDIR_TAIL): Move __set_errno (0) at the
+	end of the loop.
+
 2017-12-21  Aurelien Jarno  <aurelien@aurel32.net>
 
 	[BZ #22615]
diff --git a/dirent/scandir-tail.c b/dirent/scandir-tail.c
index 068c644c4e..00fe9ef030 100644
--- a/dirent/scandir-tail.c
+++ b/dirent/scandir-tail.c
@@ -61,8 +61,6 @@  SCANDIR_TAIL (DIR *dp,
           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 changed errno.  It was zero before and it need to be
+	 again to make the latter tests work.  */
+      __set_errno (0);
     }
 
   if (__glibc_likely (errno == 0))