[PATCH 1/3] ntdll: NtReadFileScatter results are reported asynchronously

[Andrew Eikum]

This patch sequence fixes Wolfenstein II, Bug 43935.

Signed-off-by: Andrew Eikum <aeikum(a)codeweavers.com>
---
 dlls/kernel32/tests/file.c | 53 +++++++++++++++++++++++++++++++++++++++-------
 dlls/ntdll/file.c          |  2 +-
 2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c
index 0b8cef23f8..8f5f392822 100644
--- a/dlls/kernel32/tests/file.c
+++ b/dlls/kernel32/tests/file.c
@@ -4353,7 +4353,8 @@ static void test_WriteFileGather(void)
     FILE_SEGMENT_ELEMENT fse[2];
     OVERLAPPED ovl, *povl = NULL;
     SYSTEM_INFO si;
-    LPVOID buf = NULL;
+    LPVOID wbuf = NULL, rbuf1;
+    BOOL br;
 
     ret = GetTempPathA( MAX_PATH, temp_path );
     ok( ret != 0, "GetTempPathA error %d\n", GetLastError() );
@@ -4371,12 +4372,16 @@ static void test_WriteFileGather(void)
     ok( hiocp2 != 0, "CreateIoCompletionPort failed err %u\n", GetLastError() );
 
     GetSystemInfo( &si );
-    buf = VirtualAlloc( NULL, si.dwPageSize, MEM_COMMIT, PAGE_READWRITE );
-    ok( buf != NULL, "VirtualAlloc failed err %u\n", GetLastError() );
+    wbuf = VirtualAlloc( NULL, si.dwPageSize, MEM_COMMIT, PAGE_READWRITE );
+    ok( wbuf != NULL, "VirtualAlloc failed err %u\n", GetLastError() );
+
+    rbuf1 = VirtualAlloc( NULL, si.dwPageSize, MEM_COMMIT, PAGE_READWRITE );
+    ok( rbuf1 != NULL, "VirtualAlloc failed err %u\n", GetLastError() );
 
     memset( &ovl, 0, sizeof(ovl) );
     memset( fse, 0, sizeof(fse) );
-    fse[0].Buffer = buf;
+    memset( wbuf, 0x42, si.dwPageSize );
+    fse[0].Buffer = wbuf;
     if (!WriteFileGather( hfile, fse, si.dwPageSize, NULL, &ovl ))
         ok( GetLastError() == ERROR_IO_PENDING, "WriteFileGather failed err %u\n", GetLastError() );
 
@@ -4384,20 +4389,52 @@ static void test_WriteFileGather(void)
     ok( ret, "GetQueuedCompletionStatus failed err %u\n", GetLastError());
     ok( povl == &ovl, "wrong ovl %p\n", povl );
 
+    /* read exact size */
     memset( &ovl, 0, sizeof(ovl) );
     memset( fse, 0, sizeof(fse) );
-    fse[0].Buffer = buf;
-    if (!ReadFileScatter( hfile, fse, si.dwPageSize, NULL, &ovl ))
-        ok( GetLastError() == ERROR_IO_PENDING, "ReadFileScatter failed err %u\n", GetLastError() );
+    fse[0].Buffer = rbuf1;
+    memset( rbuf1, 0, si.dwPageSize );
+    br = ReadFileScatter( hfile, fse, si.dwPageSize, NULL, &ovl );
+    ok( br == FALSE, "ReadFileScatter should be asynchronous\n" );
+    ok( GetLastError() == ERROR_IO_PENDING, "ReadFileScatter failed err %u\n", GetLastError() );
 
     ret = GetQueuedCompletionStatus( hiocp2, &size, &key, &povl, 1000 );
     ok( ret, "GetQueuedCompletionStatus failed err %u\n", GetLastError());
     ok( povl == &ovl, "wrong ovl %p\n", povl );
 
+    ok( memcmp( rbuf1, wbuf, si.dwPageSize ) == 0,
+            "data was not read into buffer\n" );
+
+    /* start read at EOF */
+    memset( &ovl, 0, sizeof(ovl) );
+    S(U(ovl)).OffsetHigh = 0;
+    S(U(ovl)).Offset = si.dwPageSize;
+    memset( fse, 0, sizeof(fse) );
+    fse[0].Buffer = rbuf1;
+    br = ReadFileScatter( hfile, fse, si.dwPageSize, NULL, &ovl );
+    ok( br == FALSE, "ReadFileScatter should have failed\n" );
+    ok( GetLastError() == ERROR_HANDLE_EOF ||
+            GetLastError() == ERROR_IO_PENDING, "ReadFileScatter gave wrong error %u\n", GetLastError() );
+    if (GetLastError() == ERROR_IO_PENDING)
+    {
+        ret = GetQueuedCompletionStatus( hiocp2, &size, &key, &povl, 1000 );
+        ok( !ret, "GetQueuedCompletionStatus should have returned failure\n" );
+        ok( GetLastError() == ERROR_HANDLE_EOF, "Got wrong error: %u\n", GetLastError() );
+        ok( povl == &ovl, "wrong ovl %p\n", povl );
+    }
+    else
+    {
+        ret = GetQueuedCompletionStatus( hiocp2, &size, &key, &povl, 100 );
+        ok( !ret, "GetQueuedCompletionStatus failed err %u\n", GetLastError() );
+        ok( GetLastError() == WAIT_TIMEOUT, "GetQueuedCompletionStatus gave wrong error %u\n", GetLastError() );
+        ok( povl == NULL, "wrong ovl %p\n", povl );
+    }
+
     CloseHandle( hfile );
     CloseHandle( hiocp1 );
     CloseHandle( hiocp2 );
-    VirtualFree( buf, 0, MEM_RELEASE );
+    VirtualFree( wbuf, 0, MEM_RELEASE );
+    VirtualFree( rbuf1, 0, MEM_RELEASE );
     DeleteFileA( filename );
 }
 
diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c
index ca2afa0e89..12b1d0f6c5 100644
--- a/dlls/ntdll/file.c
+++ b/dlls/ntdll/file.c
@@ -1105,7 +1105,7 @@ NTSTATUS WINAPI NtReadFileScatter( HANDLE file, HANDLE event, PIO_APC_ROUTINE ap
 
     if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total );
 
-    return status;
+    return STATUS_PENDING;
 }
 
 
-- 
2.15.0

[Jacek Caban]

Hi Andrew,

On 30.11.2017 18:23, Andrew Eikum wrote:
> diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c
> index ca2afa0e89..12b1d0f6c5 100644
> --- a/dlls/ntdll/file.c
> +++ b/dlls/ntdll/file.c
> @@ -1105,7 +1105,7 @@ NTSTATUS WINAPI NtReadFileScatter( HANDLE file, HANDLE event, PIO_APC_ROUTINE ap
>
> if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total );
>
> -    return status;
> +    return STATUS_PENDING;

Are you sure it should always be a case? Your tests use overlaped file
handle, I guess that's what makes differences. At least that's what I've
seen with other similar APIs (eg. recent Windows versions always return
STATUS_PENDING for ioctls if handle is overlapped).


Thanks,

Jacek

[Andrew Eikum]

On Thu, Nov 30, 2017 at 07:59:37PM +0100, Jacek Caban wrote:
> Hi Andrew,
>
> On 30.11.2017 18:23, Andrew Eikum wrote:
> > diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c
> > index ca2afa0e89..12b1d0f6c5 100644
> > --- a/dlls/ntdll/file.c
> > +++ b/dlls/ntdll/file.c
> > @@ -1105,7 +1105,7 @@ NTSTATUS WINAPI NtReadFileScatter( HANDLE file, HANDLE event, PIO_APC_ROUTINE ap
> >
> > if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total );
> >
> > -    return status;
> > +    return STATUS_PENDING;
>
> Are you sure it should always be a case? Your tests use overlaped file
> handle, I guess that's what makes differences. At least that's what I've
> seen with other similar APIs (eg. recent Windows versions always return
> STATUS_PENDING for ioctls if handle is overlapped).

I thought it was weird that this behaved different from other ntdll
functions. I hadn't thought of the file handle type. I'll try with a
non-overlapped file and see what changes.

Thanks,
Andrew

[Andrew Eikum]

On Thu, Nov 30, 2017 at 01:16:35PM -0600, Andrew Eikum wrote:
> On Thu, Nov 30, 2017 at 07:59:37PM +0100, Jacek Caban wrote:
> > Hi Andrew,
> >
> > On 30.11.2017 18:23, Andrew Eikum wrote:
> > > diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c
> > > index ca2afa0e89..12b1d0f6c5 100644
> > > --- a/dlls/ntdll/file.c
> > > +++ b/dlls/ntdll/file.c
> > > @@ -1105,7 +1105,7 @@ NTSTATUS WINAPI NtReadFileScatter( HANDLE file, HANDLE event, PIO_APC_ROUTINE ap
> > >
> > > if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total );
> > >
> > > -    return status;
> > > +    return STATUS_PENDING;
> >
> > Are you sure it should always be a case? Your tests use overlaped file
> > handle, I guess that's what makes differences. At least that's what I've
> > seen with other similar APIs (eg. recent Windows versions always return
> > STATUS_PENDING for ioctls if handle is overlapped).
>
> I thought it was weird that this behaved different from other ntdll
> functions. I hadn't thought of the file handle type. I'll try with a
> non-overlapped file and see what changes.

Actually, MSDN says the handle must be created with
FILE_FLAG_OVERLAPPED, and a quick test confirms that.

Andrew

[Dmitry Timoshkov]

Andrew Eikum <aeikum(a)codeweavers.com> wrote:
> +    /* start read at EOF */
> +    memset( &ovl, 0, sizeof(ovl) );
> +    S(U(ovl)).OffsetHigh = 0;
> +    S(U(ovl)).Offset = si.dwPageSize;
> +    memset( fse, 0, sizeof(fse) );
> +    fse[0].Buffer = rbuf1;
> +    br = ReadFileScatter( hfile, fse, si.dwPageSize, NULL, &ovl );
> +    ok( br == FALSE, "ReadFileScatter should have failed\n" );
> +    ok( GetLastError() == ERROR_HANDLE_EOF ||
> +            GetLastError() == ERROR_IO_PENDING, "ReadFileScatter gave wrong error %u\n", GetLastError() );

A test needs to reset last error before an API call if the error
code is going to be checked after that. Existing tests also fail
to do this, so they should be fixed as well.

-- 
Dmitry.

[Andrew Eikum]

On Fri, Dec 01, 2017 at 10:18:19AM +0800, Dmitry Timoshkov wrote:
> Andrew Eikum <aeikum(a)codeweavers.com> wrote:
> > +    /* start read at EOF */
> > +    memset( &ovl, 0, sizeof(ovl) );
> > +    S(U(ovl)).OffsetHigh = 0;
> > +    S(U(ovl)).Offset = si.dwPageSize;
> > +    memset( fse, 0, sizeof(fse) );
> > +    fse[0].Buffer = rbuf1;
> > +    br = ReadFileScatter( hfile, fse, si.dwPageSize, NULL, &ovl );
> > +    ok( br == FALSE, "ReadFileScatter should have failed\n" );
> > +    ok( GetLastError() == ERROR_HANDLE_EOF ||
> > +            GetLastError() == ERROR_IO_PENDING, "ReadFileScatter gave wrong error %u\n", GetLastError() );
>
> A test needs to reset last error before an API call if the error
> code is going to be checked after that. Existing tests also fail
> to do this, so they should be fixed as well.

Thanks for the review. I'll add that to the existing tests, and onto
the new ones.

Andrew