Message ID | 20171222193101.3396-4-aurelien@aurel32.net |
---|---|
State | New |
Headers | show |
Series | | expand |
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)
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 + 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. */ + __set_errno (0); } if (__glibc_likely (errno == 0))