Re: [PATCH] Fix use after free in closedir

[Corinna Vinschen <vinschen at redhat dot com>]

On Nov 13 16:50, Federico Terraneo wrote:
> 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:
> > [...]
> > 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.

That sounds good to me.  I'd just like to wait a couple of days if
somebody has some insight in terms of this fd == -1 checks we're
both not aware of.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Attachment: pgprWyYubqYmO.pgp
Description: PGP signature