[PATCH v3 4/5] ntdll: Return STATUS_PENDING from NtWriteFile() for async write.

[Jacek Caban]

Hi Paul,

On 2/20/19 7:50 PM, Paul Gofman wrote:
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=42982
>
> This matches Vista+ behaviour.
>
> Signed-off-by: Paul Gofman <gofmanp at gmail.com>
> ---
> v2, v3: no changes.
>
>   dlls/ntdll/file.c       | 7 ++++---
>   dlls/ntdll/tests/file.c | 6 +++---
>   2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c
> index ffedbeb534..b6307a2436 100644
> --- a/dlls/ntdll/file.c
> +++ b/dlls/ntdll/file.c
> @@ -1199,7 +1199,7 @@ NTSTATUS WINAPI NtWriteFile(HANDLE hFile, HANDLE hEvent,
>       int result, unix_handle, needs_close;
>       unsigned int options;
>       struct io_timeouts timeouts;
> -    NTSTATUS status;
> +    NTSTATUS status, ret_status;
>       ULONG total = 0;
>       enum server_fd_type type;
>       ULONG_PTR cvalue = apc ? 0 : (ULONG_PTR)apc_user;
> @@ -1409,9 +1409,10 @@ err:
>           if (status != STATUS_PENDING && hEvent) NtResetEvent( hEvent, NULL );
>       }
>   
> -    if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total, FALSE );
> +    ret_status = async_write && status == STATUS_SUCCESS ? STATUS_PENDING : status;
> +    if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total, ret_status == STATUS_PENDING );
>   
> -    return status;
> +    return ret_status;


While it generally looks like Vista+ indeed returns STATUS_PENDING in 
quite a lot of situations, doing it always in all cases is probably not 
the right thing. For example, read()/write() on overlapped named pipes 
does not return STATUS_PENDING if the request may be completed 
immediately. Although named pipe read/write use different code path in 
Wine, the general concern remains: we can't assume that Windows does 
that unconditionally for all all overlapped requests.


That said, I think it needs more investigation. I know that our tests 
may not be great to check that as they accept both pre-Vista and Vista+ 
behaviour as valid, but maybe marking pre-Vista results as broken() 
would be a good start so that we can see an evidence that it's a correct 
thing to do (depending on outcome, those broken()s are may or may not be 
something we want in Wine, but it would surely be useful for 
investigation). Some experimentation with different object types could 
give us useful info as well.


Also note that such change may need throughout review of Wine to make 
sure that we don't depend on pre-Vista behaviour in our code. 
read()/write() should be safer, but I've seen STATUS_PENDING returned 
from ioctl()s as well (even on pipes), and changing that would surely 
break kernel32 code.


Thanks,

Jacek