[PATCH] ntdll: Fix race condition with fd_cache when duplicating handle.

[Daniel Lehman]

Signed-off-by: Daniel Lehman <dlehman25 at gmail.com>

---
matches sequence in NtClose

i can submit a test that deliberately fails if preferred, but results vary
it takes ~4 seconds for 32 threads to hit it on my box

There's a race condition with DuplicateHandle and the fd_cache:
- Thread 1 calls DuplicateHandle(CLOSE_SOURCE), which is 3 operations:
    - req_dup_handle
    - remove_fd_from_cache
    - close
- Thread 2 calls CreateFile, then ReadFile (which calls server_get_unix_fd)
    - CreateFile
        - req_create_file
    - ReadFile
        - server_get_unix_fd
            - get_cached_fd (before lock)
            - if uncached:
                - get_cached_fd (inside lock)
                - req_get_handle_fd
                - add_fd_to_cache
        - read/pread

The problem is when a thread caches a handle in the fd_cache, then duplicates and closes it.
The closing of the handle is done on the wineserver during duplication, before it is removed
from the fd_cache on the client side.  This allows another thread opening a file to use the
just-closed handle for another file.  Because the fd_cache entry for that handle hasn't been
invalidated by the first thread, another thread can see the old fd entry.  This can lead
to accessing the wrong fd or seeing an invalid fd if the first thread calls close().

T1                                  T2                          fd_cache    Notes
---------------------               --------------------------- ----------- ------------------------
(T1 opened 'foo.txt' as 0x100)      -                           0x100 -> 4  T1 has already mapped 0x100 as fd 4 in fd_cache
req_dup_handle(0x100, CLOSE_SOURCE) -                           0x100 -> 4  T1 duplicates handle closing source
-                                   -                           0x100 -> 4  wineserver closes 0x100, handle now available
-                                   req_create_file(bar.txt)    0x100 -> 4  T2 opens 'bar.txt', gets 0x100, but fd_cache still has 0x100 -> 4
-                                   get_cached_fd(0x100) -> 4   0x100 -> 4  T2 sees existing 0x100 in fd_cache, gets fd 4 (foo.txt)
remove_fd_from_cache(0x100)         -                           0x100 -> X  T1 removes handle from cache _after_ T2 fetched it
-                                   pread(4 -> 'foo.txt)        0x100 -> X  T2 reads from fd 4 ('foo.txt') instead of 'bar.txt'
close(4)                            -                                       T1 finally closes fd 4.  this could also happen
                                                                            before T2's read and lead to EBADF
---
 dlls/ntdll/unix/server.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c
index db09d7759da..fc555a7f5f1 100644
--- a/dlls/ntdll/unix/server.c
+++ b/dlls/ntdll/unix/server.c
@@ -1639,7 +1639,10 @@ NTSTATUS WINAPI NtDuplicateObject( HANDLE source_process, HANDLE source, HANDLE
                                    ACCESS_MASK access, ULONG attributes, ULONG options )
 {
     NTSTATUS ret;
+    int fd = -1;
 
+    if (options & DUPLICATE_CLOSE_SOURCE)
+        fd = remove_fd_from_cache( source );
     SERVER_START_REQ( dup_handle )
     {
         req->src_process = wine_server_obj_handle( source_process );
@@ -1651,11 +1654,8 @@ NTSTATUS WINAPI NtDuplicateObject( HANDLE source_process, HANDLE source, HANDLE
         if (!(ret = wine_server_call( req )))
         {
             if (dest) *dest = wine_server_ptr_handle( reply->handle );
-            if (reply->closed && reply->self)
-            {
-                int fd = remove_fd_from_cache( source );
-                if (fd != -1) close( fd );
-            }
+            if (reply->closed && reply->self && (fd != -1))
+                close( fd );
         }
     }
     SERVER_END_REQ;
-- 
2.25.1