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

[Matteo Bruni]

2015-08-27 23:27 GMT+02:00 Ken Thomases <ken at codeweavers.com>:
> 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.

Yes, I didn't explain it clearly but that's what I meant with the
commit message. More below.

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

Right, will fix.

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

That's true only assuming that a lseek(SEEK_SET) and a lseek(SEEK_CUR)
turn out to be in practice the same operation. I instead thought that
they would be intrinsically different and that, while a SEEK_SET to an
arbitrary position is not fine, SEEK_SET to 0 + SEEK_CUR is okay. I
guess I'm wrong.

A "safer" option would be to store the position in the fd unchanged
but reserve two values (-1 and -2?) for the '.' and '..'. I'm not sure
that's entirely safe but I don't see anything better.

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

That's right, I missed it.

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

Good point, I'll probably take your suggested names.

Thanks for the review!