This is the mail archive of the newlib@sourceware.org mailing list for the newlib project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 11/13/2013 04:17 PM, Corinna Vinschen wrote: > > Thanks! I think you found something very suspicious here. The > test for fd != -1 is supposed to check... what? > > Opendir fails if open on the directory fails, so a valid DIR* also > has a valid dd_fd value. The only time dd_fd can be -1 > programatically is when closedir has been called and in this case > DIR* is invalid, too. Or, if two threads call closedir for the same > DIR* at about the same time, one of them could find that dd_fd is > -1 and skip the close/free stuff. However, the code > > fd = dirp->dd_fd; if (fd != -1) { dirp->dd_fd = -1; > > is definitely not thread-safe, and closedir is not *supposed* to > be reentrant or thread-safe. Indeed, having a closer look at the opendir logic, I agree that dd_fd can never be -1, and the the code is definitely not thread safe anyways unless HAVE_DD_LOCK is defined. > > This test doesn't make sense. libc/sys/sparc64/closedir.c and > libc/sys/sysvi386/closedir.c both don't perform it either. > > What I would suggest is to simplify the call to this code instead: > > int _DEFUN(closedir, (dirp), register DIR *dirp) { int fd, rc; > > #ifdef HAVE_DD_LOCK __lock_acquire_recursive(dirp->dd_lock); > #endif fd = dirp->dd_fd; rc = close(fd); _cleanupdir(dirp); > (void)free((void *)dirp->dd_buf); #ifdef HAVE_DD_LOCK > __lock_release_recursive(dirp->dd_lock); > __lock_close_recursive(dirp->dd_lock); #endif (void)free((void > *)dirp); return rc; } > > Does that make sense? Did I miss something? > Yes, but I would remove useles tests for -1 also in readdir.c and readdir_r.c, see the attached patch. 2013-11-13 Terraneo Federico <fede.tft@hotmail.it> * libc/posix/closedir.c: Fix use after free. Remove useless test dd_fd != -1 * libc/posix/readdir.c: Remove useless test dd_fd == -1 * libc/posix/readdir_r.c: Ditto. > > Thanks, Corinna > -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBAgAGBQJSg5/FAAoJECkLFtN5Xr9fv4oH/2uShvGWKYd36t3Oa6qgM6kc FCAeKulz45oxfPv5BknKK9tzGc0ReEK/n2keSFj0ibFh/MiOBWZIl9XWaItGwIPC EV2BkYBtybHTQuHpBCvCyM56mA1m/zHExVNy5ypVKCMnG6MnmrlqBIQqKwufs2sU qrBQ+znCaN54zxLKBd3KVx1KYeWd84v1cSWrvKaYRK0ywa4yWzCE1o9hMJmGDoyz TkLFj2OUpSgI8z3S1D5HdoT8RjSQpIV+o3GK9VDfZaNrcrQIvCP9AfHr1fUAt13+ rLjMNFEwr79VqjqdDA3e+EzuSpLqctF0pMkvu+D/KIL6sSiaNtXwQeQrC/Xuzz4= =zW7h -----END PGP SIGNATURE-----
diff -ruN newlib-old/newlib/libc/posix/closedir.c newlib/newlib/libc/posix/closedir.c --- newlib-old/newlib/libc/posix/closedir.c 2003-06-06 21:57:51.000000000 +0200 +++ newlib/newlib/libc/posix/closedir.c 2013-11-13 16:41:27.277621610 +0100 @@ -52,25 +52,19 @@ _DEFUN(closedir, (dirp), register DIR *dirp) { - int fd, rc; + int rc; #ifdef HAVE_DD_LOCK __lock_acquire_recursive(dirp->dd_lock); #endif - rc = 0; - fd = dirp->dd_fd; - if (fd != -1) { - dirp->dd_fd = -1; - dirp->dd_loc = 0; - (void)free((void *)dirp->dd_buf); - (void)free((void *)dirp); - rc = close(fd); - _cleanupdir(dirp); - } + rc = close(dirp->dd_fd); + _cleanupdir(dirp); + free((void *)dirp->dd_buf); #ifdef HAVE_DD_LOCK __lock_release_recursive(dirp->dd_lock); __lock_close_recursive(dirp->dd_lock); #endif + free((void *)dirp); return rc; } diff -ruN newlib-old/newlib/libc/posix/readdir.c newlib/newlib/libc/posix/readdir.c --- newlib-old/newlib/libc/posix/readdir.c 2012-07-20 18:55:19.000000000 +0200 +++ newlib/newlib/libc/posix/readdir.c 2013-11-13 16:43:11.686139336 +0100 @@ -53,9 +53,6 @@ #ifdef HAVE_DD_LOCK __lock_acquire_recursive(dirp->dd_lock); #endif - - if (dirp->dd_fd == -1) - return NULL; for (;;) { if (dirp->dd_loc == 0) { diff -ruN newlib-old/newlib/libc/posix/readdir_r.c newlib/newlib/libc/posix/readdir_r.c --- newlib-old/newlib/libc/posix/readdir_r.c 2013-06-19 17:54:20.000000000 +0200 +++ newlib/newlib/libc/posix/readdir_r.c 2013-11-13 16:42:00.605786873 +0100 @@ -60,11 +60,6 @@ #ifdef HAVE_DD_LOCK __lock_acquire_recursive(dirp->dd_lock); #endif - - if (dirp->dd_fd == -1) { - *dpp = NULL; - return errno = EBADF; - } for (;;) { if (dirp->dd_loc == 0) {
Attachment:
closedir.patch.sig
Description: Binary data
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |