[PATCH 2/4] ntdll: Fix read_directory_getdirentries().

[Matteo Bruni]

2015-08-31 21:09 GMT+02:00 Ken Thomases <ken at codeweavers.com>:
> On Aug 28, 2015, at 2:56 PM, Matteo Bruni <matteo.mystral at 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!