[PATCH 0/1] MR10780: ntdll: Only take PEB lock to update PEB data in RtlSetCurrentDirectory_U().

[Paul Gofman (＠gofman)]

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10780

[PATCH 1/1] ntdll: Only take PEB lock to update PEB data in RtlSetCurrentDirectory_U().

[Paul Gofman]

From: Paul Gofman <[e-mail]>

---
 dlls/ntdll/path.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/dlls/ntdll/path.c b/dlls/ntdll/path.c
index 91593855c6c..9a511f8e11c 100644
--- a/dlls/ntdll/path.c
+++ b/dlls/ntdll/path.c
@@ -983,34 +983,27 @@ ULONG WINAPI RtlGetCurrentDirectory_U(ULONG buflen, LPWSTR buf)
 NTSTATUS WINAPI RtlSetCurrentDirectory_U(const UNICODE_STRING* dir)
 {
     FILE_FS_DEVICE_INFORMATION device_info;
+    HANDLE handle, old_handle;
     OBJECT_ATTRIBUTES attr;
     UNICODE_STRING newdir;
     IO_STATUS_BLOCK io;
     CURDIR *curdir;
-    HANDLE handle;
     NTSTATUS nts;
     ULONG size;
     PWSTR ptr;
 
     newdir.Buffer = NULL;
 
-    RtlAcquirePebLock();
-
-    if (NtCurrentTeb()->Tib.SubSystemTib)  /* FIXME: hack */
-        curdir = &((WIN16_SUBSYSTEM_TIB *)NtCurrentTeb()->Tib.SubSystemTib)->curdir;
-    else
-        curdir = &NtCurrentTeb()->Peb->ProcessParameters->CurrentDirectory;
-
-    if (!RtlDosPathNameToNtPathName_U( dir->Buffer, &newdir, NULL, NULL ))
-    {
-        nts = STATUS_OBJECT_NAME_INVALID;
-        goto out;
-    }
+    if (!RtlDosPathNameToNtPathName_U( dir->Buffer, &newdir, NULL, NULL )) return STATUS_OBJECT_NAME_INVALID;
 
     InitializeObjectAttributes( &attr, &newdir, OBJ_CASE_INSENSITIVE, 0, NULL );
     nts = NtOpenFile( &handle, FILE_TRAVERSE | SYNCHRONIZE, &attr, &io, FILE_SHARE_READ | FILE_SHARE_WRITE,
                       FILE_DIRECTORY_FILE | FILE_SYNCHRONOUS_IO_NONALERT );
-    if (nts != STATUS_SUCCESS) goto out;
+    if (nts != STATUS_SUCCESS)
+    {
+        RtlFreeUnicodeString( &newdir );
+        return nts;
+    }
 
     /* don't keep the directory handle open on removable media */
     if (!NtQueryVolumeInformationFile( handle, &io, &device_info,
@@ -1021,9 +1014,6 @@ NTSTATUS WINAPI RtlSetCurrentDirectory_U(const UNICODE_STRING* dir)
         handle = 0;
     }
 
-    if (curdir->Handle) NtClose( curdir->Handle );
-    curdir->Handle = handle;
-
     /* append trailing \ if missing */
     size = newdir.Length / sizeof(WCHAR);
     ptr = newdir.Buffer;
@@ -1039,14 +1029,23 @@ NTSTATUS WINAPI RtlSetCurrentDirectory_U(const UNICODE_STRING* dir)
         *ptr = '\\';
     }
 
+    RtlAcquirePebLock();
+    if (NtCurrentTeb()->Tib.SubSystemTib)  /* FIXME: hack */
+        curdir = &((WIN16_SUBSYSTEM_TIB *)NtCurrentTeb()->Tib.SubSystemTib)->curdir;
+    else
+        curdir = &NtCurrentTeb()->Peb->ProcessParameters->CurrentDirectory;
+
+    old_handle = curdir->Handle;
+    curdir->Handle = handle;
+
     memcpy( curdir->DosPath.Buffer, ptr, size * sizeof(WCHAR));
     curdir->DosPath.Buffer[size] = 0;
     curdir->DosPath.Length = size * sizeof(WCHAR);
+    RtlReleasePebLock();
 
     TRACE( "curdir now %s %p\n", debugstr_w(curdir->DosPath.Buffer), curdir->Handle );
 
- out:
     RtlFreeUnicodeString( &newdir );
-    RtlReleasePebLock();
+    if (old_handle) NtClose( old_handle );
     return nts;
 }
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/10780

[Paul Gofman (＠gofman)]

Fixes The Rising of The Shield Hero: Relieve the Animation hangs on start (and maybe something else with ENIGMA protector).  The protector implements virtual file system which hotpatches a lot of file related functions, in particular, NtOpenFile() and NtClose(). When NtOpenFile() is called from RtlSetCurrentDirectory_U() with peb lock held the hotpatcher takes its own lock and that can deadlock with other threads performing file / path operations ending up in get_full_path_helper() which also takes PEB lock. Once the hotpatcher is initialized the game only sets "." or the same full path as current and may do that rather often.

Back then I was trying to fix it in a different way avoiding setting the same directory path over and reopening the current directory file but that was wrong in various ways.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10780#note_138231