[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    Re: [PATCH 2/4] ntdll: Fix read_directory_getdirentries().
From:       Matteo Bruni <matteo.mystral () gmail ! com>
Date:       2015-08-31 20:29:46
Message-ID: CABvNrtOnJ_6xzq4FmMOBxsjBuhdn6=roBVCK6bjn8OUyhmgmVQ () mail ! gmail ! com
[Download RAW message or body]

2015-08-31 21:09 GMT+02:00 Ken Thomases <ken@codeweavers.com>:
> On Aug 28, 2015, at 2:56 PM, Matteo Bruni <matteo.mystral@gmail.com> wrote:
> > 
> > We could swap the "." and ".." entries with the first two entries
> > returned from getdirentries(), in the same vein as
> > read_directory_getdents(). That should work but I'm not sure it's much
> > of an improvement overall.
> 
> Yikes!  I just had a look at read_directory_getdents().  It uses a static variable \
> to store the information needed to know if it should enumerate ".." or a real \
> entry.  That's terrible.  It's not thread safe.  It's not even safe against \
> enumerating two directories item by item in alternation.

Actually it should be safe for both. About thread safety, check
NtQueryDirectoryFile(), the various read_directory_* helpers are
protected by the "dir_section" critical section. BTW notice that fd is
opened by changing the current directory with fchdir().
As for the enumerating multiple directories by alternation, look at
the last_dir_id checks and related code.

Yes, all this is not nice by any means.

> > I'm attaching a v2 of the patch with your points addressed.
> 
> > +    /* Store the updated position in the fd. */
> > +    if (i == OFF_MAX - 2 || i == OFF_MAX - 1)
> > +    {
> > +        i = lseek( fd, i, SEEK_SET );
> > +    }
> > +    else
> > +    {
> > +        lseek( fd, restart_pos, SEEK_SET );
> > +        if (bytes_left != bytes_read)
> > +            wine_getdirentries( fd, data, bytes_read - bytes_left, &restart_pos \
> > ); }
> 
> Hmm.  This raises another problem.  I knew about it, but had forgotten.  You can't \
> rely on getdirentries() reading a specified number of entries (or bytes) and \
> leaving the file position such that the next read will resume with the next entry. 
> On some file systems, getdirentries() reads entries from the directory in whole \
> blocks.  It may read fewer entries than the supplied buffer could hold if it \
> couldn't fit all of the next block's worth of entries into the buffer.  On the \
> affected file systems, getdirentries() will always leave the file position on a \
> multiple of the block size and there's nothing you can do to change that.  (This is \
> CodeWeavers bug 12271, FYI.) 
> So, that last call to wine_getdirentries() won't do what you hope.  And, in \
> general, there's no good way to make read_directory_getdirentries() leave the file \
> position in a place such that it can resume at the next entry it would have \
> enumerated. 
> This is a nasty problem.  Sorry to have wasted your time working on this before \
> remembering this fundamental flaw.

Eh, right, that's real nasty. No problem, I should have realized that too.

> It may be that the only reliable fix that can be implemented in ntdll is to use a \
> directory index as the file position, after all.  Except that we'd never use that \
> as the place to read from.  Instead, we'd always seek to the beginning and read \
> entries, throwing away any that were before the index stored in the file position.  \
> Of course, this causes tons of redundant reading of directory entries.

I think I'll try this just in case, as a sort of "experiment", even
though it doesn't seem to be the correct way forward overall.

> Alexandre had a vague suggestion for a different approach in a previous thread \
> <https://www.winehq.org/pipermail/wine-devel/2014-February/102688.html> that I \
> didn't entirely follow.  Part of it seems to have been to read the whole directory \
> listing into memory when it is first requested (or a restart is requested) and then \
> return entries from this cache on subsequent calls.  His scheme may have required \
> additional support from the server to track metadata for handles.  I don't know how \
> reads of the same handle in multiple processes would be coordinated. 
> Perhaps he can be persuaded to describe the solution he has in mind more fully.  \
> Or, if you can understand what he wrote in that previous thread better than I, you \
> can try your hand at it.

Uh, I didn't recall this wine-devel thread at all but I'm sure I
followed it back then. FWIW my understanding pretty much matches yours
and I see why that would be the "right" fix. I guess I'll just try to
implement Alexandre's suggestion and see what happens. Expect either a
new set of patches or some kind of request for help from me in the
somewhat near future :P

> -Ken

Thanks again for the feedback!


[prev in list] [next in list] [prev in thread] [next in thread]