[PATCH] ntdll: Fix race condition with fd_cache when duplicating handle.
Signed-off-by: Daniel Lehman <dlehman25(a)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
Daniel Lehman <dlehman25(a)gmail.com> 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. -- Alexandre Julliard julliard(a)winehq.org
Daniel Lehman <dlehman25(a)gmail.com> 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. -- Alexandre Julliard julliard(a)winehq.org
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
Daniel Lehman <dlehman25(a)gmail.com> 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. -- Alexandre Julliard julliard(a)winehq.org
participants (2)
-
Alexandre Julliard -
Daniel Lehman