[PATCH resend] server: Avoid reqeuests with null pointer but invalid size.

[Bernhard Übelacker]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51770
Signed-off-by: Bernhard Übelacker 
---
v1: https://www.winehq.org/pipermail/wine-devel/2021-December/202913.html
---
 dlls/kernel32/tests/volume.c | 16 ++++++++++++++++
 dlls/ntdll/unix/file.c       |  2 ++
 include/wine/server.h        |  2 +-
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/dlls/kernel32/tests/volume.c b/dlls/kernel32/tests/volume.c
index 9166cf228d9..4bee4207a37 100644
--- a/dlls/kernel32/tests/volume.c
+++ b/dlls/kernel32/tests/volume.c
@@ -618,6 +618,7 @@ static void test_disk_query_property(void)
     STORAGE_PROPERTY_QUERY query = {0};
     STORAGE_DESCRIPTOR_HEADER header = {0};
     STORAGE_DEVICE_DESCRIPTOR descriptor = {0};
+    STORAGE_DEVICE_NUMBER device_number = {0};
     HANDLE handle;
     DWORD error;
     DWORD size;
@@ -654,6 +655,21 @@ static void test_disk_query_property(void)
     ok(descriptor.Version == sizeof(descriptor), "got descriptor.Version %ld\n", descriptor.Version);
     ok(descriptor.Size >= sizeof(descriptor), "got descriptor.Size %ld\n", descriptor.Size);

+    SetLastError(0xdeadbeef);
+    ret = DeviceIoControl(handle, IOCTL_STORAGE_GET_DEVICE_NUMBER, NULL, 0, &device_number, sizeof(device_number), &size, NULL);
+    error = GetLastError();
+    ok(ret, "expect ret %#x, got %#x\n", TRUE, ret);
+    ok(error == 0xdeadbeef, "expect err %#x, got err %#x\n", 0xdeadbeef, error);
+    ok(size == sizeof(device_number), "got size %d\n", size);
+
+    /* unclean call with valid in_buffer=NULL but incorrect in_size=4 */
+    SetLastError(0xdeadbeef);
+    ret = DeviceIoControl(handle, IOCTL_STORAGE_GET_DEVICE_NUMBER, NULL, 4, &device_number, sizeof(device_number), &size, NULL);
+    error = GetLastError();
+    ok(ret, "expect ret %#x, got %#x\n", TRUE, ret);
+    ok(error == 0xdeadbeef, "expect err %#x, got err %#x\n", 0xdeadbeef, error);
+    ok(size == sizeof(device_number), "got size %d\n", size);
+
     CloseHandle(handle);
 }

diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c
index cc8bf0c6e82..3193a21c48f 100644
--- a/dlls/ntdll/unix/file.c
+++ b/dlls/ntdll/unix/file.c
@@ -4913,6 +4913,8 @@ static NTSTATUS server_ioctl_file( HANDLE handle, HANDLE event,

if (status != STATUS_PENDING) free( async );

+    if (wait_handle && status == STATUS_ACCESS_VIOLATION)
+        ERR("Sending request failed but wait requested. Expect the application to hang.\n");
     if (wait_handle) status = wait_async( wait_handle, (options & FILE_SYNCHRONOUS_IO_ALERT) );
     return status;
 }
diff --git a/include/wine/server.h b/include/wine/server.h
index b05271b7522..d44997bf74f 100644
--- a/include/wine/server.h
+++ b/include/wine/server.h
@@ -71,7 +71,7 @@ static inline data_size_t wine_server_reply_size( const void *reply )
 static inline void wine_server_add_data( void *req_ptr, const void *ptr, data_size_t size )
 {
     struct __server_request_info * const req = req_ptr;
-    if (size)
+    if (size && ptr != NULL)
     {
         req->data[req->data_count].ptr = ptr;
         req->data[req->data_count++].size = size;

[Alexandre Julliard]

Bernhard Übelacker  writes:
> @@ -71,7 +71,7 @@ static inline data_size_t wine_server_reply_size( const void *reply )
>  static inline void wine_server_add_data( void *req_ptr, const void *ptr, data_size_t size )
>  {
>      struct __server_request_info * const req = req_ptr;
> - if (size)
> - if (size && ptr != NULL)
> {
>     req->data[req->data_count].ptr = ptr;
>     req->data[req->data_count++].size = size;

That doesn't belong here, it's up to the callers to validate the
pointers when necessary.

[Bernhard Übelacker]

Am 19.04.22 um 17:43 schrieb Alexandre Julliard:
> Bernhard Übelacker  writes:
> > @@ -71,7 +71,7 @@ static inline data_size_t wine_server_reply_size( const void *reply )
> >   static inline void wine_server_add_data( void *req_ptr, const void *ptr, data_size_t size )
> >   {
> >       struct __server_request_info * const req = req_ptr;
> > - if (size)
> > - if (size && ptr != NULL)
> >  {
> >      req->data[req->data_count].ptr = ptr;
> >      req->data[req->data_count++].size = size;
>
> That doesn't belong here, it's up to the callers to validate the
> pointers when necessary.

Thank you very much for the review.
I sent in another version which moved the pointer
validation to the server_ioctl_file function.