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

[Ken Thomases]

On Aug 27, 2015, at 2:08 PM, Matteo Bruni <mbruni at codeweavers.com> wrote:
> 
> ---
> The man page for getdirentries mentions that only positions previously
> returned from lseek or getdirentries should be used, I'm arguably abusing
> the pointer to store the index and then restoring the position using
> SEEK_CUR. It seems to work fine for me, not sure if that might cause
> issues with some specific filesystem setups.

You're not restoring it, you're setting it to a computed value.  The computation assumes that the file position is the index of the entry that was read.  For example, you get the current position and then you're setting the position to that original position minus 2 (for the synthesized . and .. entries).  That doesn't seem like a reasonable assumption to me.

In some quick testing (with different code), I get results like these:

rc 8172 base 0/0x00000000 filepos 469762300/0x1c0000fc entries 252/0x000000fc total 252/0x000000fc
rc 8188 base 469762300/0x1c0000fc filepos 469762640/0x1c000250 entries 340/0x00000154 total 592/0x00000250
rc 2364 base 469762640/0x1c000250 filepos 469762732/0x1c0002ac entries 92/0x0000005c total 684/0x000002ac

The file positions seem to be 0x1c000000 with the entry index added to it.  However, each run of my test program gives a different fixed baseline. The file positions are definitely not _just_ entry indices.

It does seem to work to seek to an entry index in some quick testing, but it makes me very nervous.


> ---
> dlls/ntdll/directory.c | 143 ++++++++++++++++++-------------------------------
> 1 file changed, 53 insertions(+), 90 deletions(-)
> 
> diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c
> index f9562d2..eb6cd45 100644
> --- a/dlls/ntdll/directory.c
> +++ b/dlls/ntdll/directory.c
> @@ -1820,27 +1820,27 @@ static int read_directory_getdirentries( int fd, IO_STATUS_BLOCK *io, void *buff
>                                          BOOLEAN restart_scan, FILE_INFORMATION_CLASS class )
> {
>     long restart_pos;
> -    ULONG_PTR restart_info_pos = 0;
> -    size_t size, initial_size = length;
> -    int res, fake_dot_dot = 1;
> -    char *data, local_buffer[8192];
> +    off_t newcursor;

This newcursor variable is only ever set and then never used.

> +    int total, res, i;
> +    char data[8192];
>     struct dirent *de;
> -    union file_directory_info *info, *last_info = NULL, *restart_last_info = NULL;
> +    union file_directory_info *info, *last_info = NULL;
> 
> -    size = initial_size;
> -    data = local_buffer;
> -    if (size > sizeof(local_buffer) && !(data = RtlAllocateHeap( GetProcessHeap(), 0, size )))
> +    io->u.Status = STATUS_SUCCESS;
> +
> +    if (restart_scan)
>     {
> -        io->u.Status = STATUS_NO_MEMORY;
> -        return io->u.Status;
> +        lseek( fd, 0, SEEK_SET );
> +        i = 0;
> +    }
> +    else
> +    {
> +        i = lseek( fd, 0, SEEK_CUR );
> +        lseek( fd, 0, SEEK_SET );
> +        newcursor = lseek( fd, i < 2 ? 0 : i - 2, SEEK_CUR );

Here's where you're seeking to an arbitrary position.  Also, the last two seeks can be combined (if they were right).  That is, seeking to the beginning (0, SEEK_SET) and then seeking relative to the current position is just seeking to the relative position as though it were absolute.


>     res = 0;
> done:
> -    if (data != local_buffer) RtlFreeHeap( GetProcessHeap(), 0, data );
>     return res;

This wasn't changed by your patch, but it appears that this function can only return 0 at this point, so it should just do so.  res is set to 0 just before the "done" label.  All "goto done" statements are immediately preceded by setting res to 0.  So, the conflation of "res" with a return value should be eliminated.  Setting it to 0 in those various places is unnecessary.

Once that's cleared up, I would rename res and total to something like bytes_left and bytes_read, respectively.  To me, on my first scan through your patch, I was expecting total to accumulate the total amount read over several iterations.  (Not that that would have been useful, but it confused me.)

-Ken