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

[Ken Thomases]

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.


> 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.

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.

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.

-Ken