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

[Daniel Lehman]

Signed-off-by: Daniel Lehman

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

[Alexandre Julliard]

Daniel Lehman  writes:
> @@ -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 );

I'm afraid it's not that simple, there's no guarantee that the source
handle is for the current process.

[Daniel Lehman]

> I'm afraid it's not that simple, there's no guarantee that the source
> handle is for the current process.

do you mean if the current process received a file handle from another
process and wanted to close the handle on the remote end?

thanks
daniel

[Alexandre Julliard]

Daniel Lehman  writes:
> > I'm afraid it's not that simple, there's no guarantee that the source
> > handle is for the current process.
>
> do you mean if the current process received a file handle from another
> process and wanted to close the handle on the remote end?

Yes, that's why there's a source process parameter.

[Daniel Lehman]

> > do you mean if the current process received a file handle from another
> > process and wanted to close the handle on the remote end?
>
> Yes, that's why there's a source process parameter.

yeah, my fix doesn't cover receiving a file handle (or sending on
behalf of another process).  there isn't an existing way (that i see)
to tell the other process to remove the cached fd

i was hoping to at least fix the race in this case where the current
process is sending

thanks
daniel

[Alexandre Julliard]

Daniel Lehman  writes:
> > > do you mean if the current process received a file handle from another
> > > process and wanted to close the handle on the remote end?
> >
> > Yes, that's why there's a source process parameter.
>
> yeah, my fix doesn't cover receiving a file handle (or sending on
> behalf of another process).  there isn't an existing way (that i see)
> to tell the other process to remove the cached fd
>
> i was hoping to at least fix the race in this case where the current
> process is sending

Note that there can be other reasons for the server call to fail, so we
can't just remove the handle from the cache unconditionally. This will
need more thought.