[PATCH v6 0/4] MR6855: server: Fix renaming file to a hardlink of itself.

[Gabriel Ivăncescu]

--
  v6: server: When renaming, only fstat the source once.
      server: Handle hardlinks and casefolding when renaming the same file.
      kernel32/tests: Test renaming a file into a hardlink of itself.
      kernel32/tests: Use FindClose instead of CloseHandle when closing
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855

[PATCH v6 1/4] kernel32/tests: Use FindClose instead of CloseHandle when closing FindFirstFile handle.

[Gabriel Ivăncescu]

From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/kernel32/tests/file.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c
index b651fc1a1ba..2e5dd78a13c 100644
--- a/dlls/kernel32/tests/file.c
+++ b/dlls/kernel32/tests/file.c
@@ -1996,9 +1996,8 @@ static void test_MoveFileA(void)
     char tempdir[MAX_PATH];
     char source[MAX_PATH], dest[MAX_PATH];
     static const char prefix[] = "pfx";
+    HANDLE hfile, hfind, hmapfile;
     WIN32_FIND_DATAA find_data;
-    HANDLE hfile;
-    HANDLE hmapfile;
     DWORD ret;
     BOOL retok;
 
@@ -2078,14 +2077,14 @@ static void test_MoveFileA(void)
     ret = MoveFileA(source, tempdir);
     ok(ret, "MoveFileA: failed, error %ld\n", GetLastError());
 
-    hfile = FindFirstFileA(tempdir, &find_data);
-    ok(hfile != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError());
-    if (hfile != INVALID_HANDLE_VALUE)
+    hfind = FindFirstFileA(tempdir, &find_data);
+    ok(hfind != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError());
+    if (hfind != INVALID_HANDLE_VALUE)
     {
         todo_wine ok(!lstrcmpA(strrchr(tempdir, '\\') + 1, find_data.cFileName),
            "MoveFile failed to change casing on same file: got %s\n", find_data.cFileName);
     }
-    CloseHandle(hfile);
+    FindClose(hfind);
 
     /* test renaming another file "Remove Be" to "Remove Me", which replaces the existing "Remove me" */
     tempdir[lstrlenA(tempdir) - 2] = 'B';
@@ -2102,14 +2101,14 @@ static void test_MoveFileA(void)
 
     tempdir[lstrlenA(tempdir) - 2] = 'm';
 
-    hfile = FindFirstFileA(tempdir, &find_data);
-    ok(hfile != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError());
-    if (hfile != INVALID_HANDLE_VALUE)
+    hfind = FindFirstFileA(tempdir, &find_data);
+    ok(hfind != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError());
+    if (hfind != INVALID_HANDLE_VALUE)
     {
         ok(!lstrcmpA(strrchr(source, '\\') + 1, find_data.cFileName),
            "MoveFile failed to change casing on existing target file: got %s\n", find_data.cFileName);
     }
-    CloseHandle(hfile);
+    FindClose(hfind);
 
     ret = DeleteFileA(tempdir);
     ok(ret, "DeleteFileA: error %ld\n", GetLastError());
@@ -2123,14 +2122,14 @@ static void test_MoveFileA(void)
     ret = MoveFileA(source, tempdir);
     ok(ret, "MoveFileA: failed, error %ld\n", GetLastError());
 
-    hfile = FindFirstFileA(tempdir, &find_data);
-    ok(hfile != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError());
-    if (hfile != INVALID_HANDLE_VALUE)
+    hfind = FindFirstFileA(tempdir, &find_data);
+    ok(hfind != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError());
+    if (hfind != INVALID_HANDLE_VALUE)
     {
         todo_wine ok(!lstrcmpA(strrchr(tempdir, '\\') + 1, find_data.cFileName),
            "MoveFile failed to change casing on same directory: got %s\n", find_data.cFileName);
     }
-    CloseHandle(hfile);
+    FindClose(hfind);
 
     lstrcpyA(source, dest);
     lstrcpyA(dest, tempdir);
@@ -2145,12 +2144,11 @@ static void test_MoveFileA(void)
     {
         WIN32_FIND_DATAA fd;
         char temppath[MAX_PATH];
-        HANDLE hFind;
 
         lstrcpyA(temppath, tempdir);
         lstrcatA(temppath, "\\*.*");
-        hFind = FindFirstFileA(temppath, &fd);
-        if (INVALID_HANDLE_VALUE != hFind)
+        hfind = FindFirstFileA(temppath, &fd);
+        if (INVALID_HANDLE_VALUE != hfind)
         {
           LPSTR lpName;
           do
@@ -2160,8 +2158,8 @@ static void test_MoveFileA(void)
               lpName = fd.cFileName;
             ok(IsDotDir(lpName), "MoveFileA: wildcards file created!\n");
           }
-          while (FindNextFileA(hFind, &fd));
-          FindClose(hFind);
+          while (FindNextFileA(hfind, &fd));
+          FindClose(hfind);
         }
     }
     ret = DeleteFileA(source);
@@ -2895,7 +2893,7 @@ static void test_FindFirstFileA(void)
     if (handle == INVALID_HANDLE_VALUE)
         ok( err == ERROR_PATH_NOT_FOUND, "Bad Error number %d\n", err );
     else
-        CloseHandle( handle );
+        FindClose( handle );
 
     /* try FindFirstFileA on "c:\foo\nul\bar" */
     SetLastError( 0xdeadbeaf );
-- 
GitLab


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

[PATCH v6 2/4] kernel32/tests: Test renaming a file into a hardlink of itself.

[Gabriel Ivăncescu]

From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/kernel32/tests/file.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c
index 2e5dd78a13c..ec58e5e685a 100644
--- a/dlls/kernel32/tests/file.c
+++ b/dlls/kernel32/tests/file.c
@@ -2162,10 +2162,34 @@ static void test_MoveFileA(void)
           FindClose(hfind);
         }
     }
-    ret = DeleteFileA(source);
-    ok(ret, "DeleteFileA: error %ld\n", GetLastError());
     ret = DeleteFileA(dest);
     ok(!ret, "DeleteFileA: error %ld\n", GetLastError());
+
+    /* test renaming file to hardlink of itself with different case */
+    lstrcpyA(dest, tempdir);
+    lstrcatA(dest, "\\hardlink");
+    ret = CreateHardLinkA(dest, source, NULL);
+    ok(ret, "CreateHardLinkA: error %ld\n", GetLastError());
+    dest[lstrlenA(dest) - 4] = 'L';
+    ret = MoveFileA(source, dest);
+    ok(ret, "MoveFileA: error %ld\n", GetLastError());
+
+    hfind = FindFirstFileA(dest, &find_data);
+    ok(hfind != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError());
+    if (hfind != INVALID_HANDLE_VALUE)
+    {
+        todo_wine
+        ok(!lstrcmpA(strrchr(dest, '\\') + 1, find_data.cFileName),
+           "MoveFile failed to change casing on hardlink of itself: got %s\n", find_data.cFileName);
+    }
+    FindClose(hfind);
+    ret = GetFileAttributesA(source);
+    todo_wine
+    ok(ret == INVALID_FILE_ATTRIBUTES && GetLastError() == ERROR_FILE_NOT_FOUND, "GetFileAttributesA: error %ld\n", GetLastError());
+    if(ret != INVALID_FILE_ATTRIBUTES) DeleteFileA(source);
+    ret = DeleteFileA(dest);
+    ok(ret, "DeleteFileA: error %ld\n", GetLastError());
+
     ret = RemoveDirectoryA(tempdir);
     ok(ret, "DeleteDirectoryA: error %ld\n", GetLastError());
 }
-- 
GitLab


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

[PATCH v6 3/4] server: Handle hardlinks and casefolding when renaming the same file.

[Gabriel Ivăncescu]

From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>

Based on code and suggestions by Jinoh Kang.

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/kernel32/tests/file.c |   2 -
 server/fd.c                | 111 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 110 insertions(+), 3 deletions(-)

diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c
index ec58e5e685a..c15f4ac97ca 100644
--- a/dlls/kernel32/tests/file.c
+++ b/dlls/kernel32/tests/file.c
@@ -2184,9 +2184,7 @@ static void test_MoveFileA(void)
     }
     FindClose(hfind);
     ret = GetFileAttributesA(source);
-    todo_wine
     ok(ret == INVALID_FILE_ATTRIBUTES && GetLastError() == ERROR_FILE_NOT_FOUND, "GetFileAttributesA: error %ld\n", GetLastError());
-    if(ret != INVALID_FILE_ATTRIBUTES) DeleteFileA(source);
     ret = DeleteFileA(dest);
     ok(ret, "DeleteFileA: error %ld\n", GetLastError());
 
diff --git a/server/fd.c b/server/fd.c
index ce32e7f8397..a6e0fff9290 100644
--- a/server/fd.c
+++ b/server/fd.c
@@ -2531,6 +2531,109 @@ static void set_fd_disposition( struct fd *fd, unsigned int flags )
         ((fd->options & FILE_DELETE_ON_CLOSE) ? FILE_DISPOSITION_DELETE : 0);
 }
 
+/* rename the same file, dealing with casefolding and possibly different hardlinks to it */
+static void rename_same_file( const char *src, const char *dst, int is_dir )
+{
+    static const char tmpname_fmt[] = ".wine-rename-tmp-%08x";
+    static unsigned tmp_value;
+
+    char *dirname, tmpname[sizeof(tmpname_fmt) + 4 /* remaining of %08x */];
+    const char *srcname, *dstname = strrchr( dst, '/' ) + 1;
+    struct stat st, st2;
+    int dirfd, res = 0;
+    unsigned i;
+
+    /* first, if not a dir, check the directories they reside in */
+    if (!is_dir)
+    {
+        if (!(dirname = memdup( dst, dstname - dst )))
+            return;
+        dirname[dstname - dst - 1] = '\0';
+        if ((res = stat( dirname, &st )))
+            file_set_error();
+        free( dirname );
+        if (res)
+            return;
+    }
+
+    srcname = strrchr( src, '/' ) + 1;
+    if (!(dirname = memdup( src, srcname - src )))
+        return;
+    dirname[srcname - src - 1] = '\0';
+    if ((dirfd = open( dirname, O_RDONLY | O_NONBLOCK )) == -1)
+        file_set_error();
+    free( dirname );
+    if (dirfd == -1)
+        return;
+
+    if (!is_dir)
+    {
+        if (fstat( dirfd, &st2 ))
+        {
+            file_set_error();
+            goto ret;
+        }
+
+        /* if different directories, it must be a hardlink, so simply remove the source */
+        if (st.st_dev != st2.st_dev || st.st_ino != st2.st_ino)
+        {
+            if (unlinkat( dirfd, srcname, 0 ))
+                file_set_error();
+            goto ret;
+        }
+    }
+
+    /* same dentry means a no-op */
+    if (!strcmp( srcname, dstname )) goto ret;
+
+    /* This is more complicated now, because in case of a casefold (+F) directory, the destination may very well be the same dentry,
+     * even if the name doesn't match (if it differs just by case), in which case unlinking it is wrong and can be dangerous. Instead,
+     * we first rename the source to a temporary filename in the same directory. If this is a casefold dir, this will also remove the
+     * destination, otherwise the destination still exists. We then create a hardlink from the destination to our temporary name, and
+     * finally, unlink the temporary. This still works if the directory is case sensitive, so it's not a problem in either case. */
+    tmp_value += (current_time >> 16) + current_time;
+    for (i = 0; i < 0x8000; i++, tmp_value += 7777)
+    {
+        sprintf( tmpname, tmpname_fmt, tmp_value );
+        if (fstatat( dirfd, tmpname, &st, 0 ))  /* tmpname doesn't exist */
+            break;
+    }
+    if (i < 0x8000)
+    {
+        if (renameat( dirfd, srcname, dirfd, tmpname ))
+        {
+            file_set_error();
+            goto ret;
+        }
+
+        if (!is_dir)
+        {
+            if (linkat( dirfd, tmpname, dirfd, dstname, 0 ) && errno != EEXIST)
+            {
+                file_set_error();
+                /* Revert the temporary rename */
+                renameat( dirfd, tmpname, dirfd, srcname );
+                goto ret;
+            }
+
+            if (unlinkat( dirfd, tmpname, 0 ))
+                file_set_error();
+        }
+        else
+        {
+            /* directories can't have hardlinks, so just rename it to the destination */
+            if (renameat( dirfd, tmpname, dirfd, dstname ))
+            {
+                file_set_error();
+                goto ret;
+            }
+        }
+    }
+
+ret:
+    close( dirfd );
+}
+
 /* set new name for the fd */
 static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, data_size_t len,
                          struct unicode_str nt_name, int create_link, unsigned int flags )
@@ -2556,6 +2659,11 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da
         set_error( STATUS_OBJECT_PATH_SYNTAX_BAD );
         return;
     }
+
+    /* strip trailing slashes */
+    while (nameptr[len - 1] == '/')
+        len--;
+
     if (!(name = mem_alloc( len + 1 ))) return;
     memcpy( name, nameptr, len );
     name[len] = 0;
@@ -2583,7 +2691,8 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da
     {
         if (!fstat( fd->unix_fd, &st2 ) && st.st_ino == st2.st_ino && st.st_dev == st2.st_dev)
         {
-            if (create_link && !replace) set_error( STATUS_OBJECT_NAME_COLLISION );
+            if (!create_link) rename_same_file( fd->unix_name, name, S_ISDIR( st.st_mode ) );
+            else if (!replace) set_error( STATUS_OBJECT_NAME_COLLISION );
             free( name );
             return;
         }
-- 
GitLab


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

[PATCH v6 4/4] server: When renaming, only fstat the source once.

[Gabriel Ivăncescu]

From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 server/fd.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/server/fd.c b/server/fd.c
index a6e0fff9290..2cc5cd2cc54 100644
--- a/server/fd.c
+++ b/server/fd.c
@@ -319,6 +319,24 @@ static file_pos_t max_unix_offset = OFF_T_MAX;
         fprintf( stderr, "%lx", (unsigned long)(val) ); \
   } while (0)
 
+struct cached_stat
+{
+    struct stat st;
+    int init;
+};
+
+/* Caches fstat (must use on same fd), set 'init' to 0 before use */
+static inline int cached_fstat(int fd, struct cached_stat *cached_stat)
+{
+    int ret;
+
+    if (cached_stat->init)
+        return 0;
+    if (!(ret = fstat( fd, &cached_stat->st )))
+        cached_stat->init = 1;
+    return ret;
+}
+
 
 
 /****************************************************************/
@@ -2638,11 +2656,14 @@ ret:
 static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, data_size_t len,
                          struct unicode_str nt_name, int create_link, unsigned int flags )
 {
+    struct cached_stat cached_st;
     struct inode *inode;
-    struct stat st, st2;
+    struct stat st;
     char *name;
     const unsigned int replace = flags & FILE_RENAME_REPLACE_IF_EXISTS;
 
+    cached_st.init = 0;
+
     if (!fd->inode || !fd->unix_name)
     {
         set_error( STATUS_OBJECT_TYPE_MISMATCH );
@@ -2681,7 +2702,7 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da
     }
 
     /* when creating a hard link, source cannot be a dir */
-    if (create_link && !fstat( fd->unix_fd, &st ) && S_ISDIR( st.st_mode ))
+    if (create_link && !cached_fstat( fd->unix_fd, &cached_st ) && S_ISDIR( cached_st.st.st_mode ))
     {
         set_error( STATUS_FILE_IS_A_DIRECTORY );
         goto failed;
@@ -2689,7 +2710,7 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da
 
     if (!stat( name, &st ))
     {
-        if (!fstat( fd->unix_fd, &st2 ) && st.st_ino == st2.st_ino && st.st_dev == st2.st_dev)
+        if (!cached_fstat( fd->unix_fd, &cached_st ) && st.st_ino == cached_st.st.st_ino && st.st_dev == cached_st.st.st_dev)
         {
             if (!create_link) rename_same_file( fd->unix_name, name, S_ISDIR( st.st_mode ) );
             else if (!replace) set_error( STATUS_OBJECT_NAME_COLLISION );
@@ -2732,7 +2753,7 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da
 
         /* link() expects that the target doesn't exist */
         /* rename() cannot replace files with directories */
-        if (create_link || S_ISDIR( st2.st_mode ))
+        if (create_link || S_ISDIR( cached_st.st.st_mode ))
         {
             if (unlink( name ))
             {
@@ -2756,14 +2777,14 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da
         goto failed;
     }
 
-    if (is_file_executable( fd->unix_name ) != is_file_executable( name ) && !fstat( fd->unix_fd, &st ))
+    if (is_file_executable( fd->unix_name ) != is_file_executable( name ) && !cached_fstat( fd->unix_fd, &cached_st ))
     {
         if (is_file_executable( name ))
             /* set executable bit where read bit is set */
-            st.st_mode |= (st.st_mode & 0444) >> 2;
+            cached_st.st.st_mode |= (cached_st.st.st_mode & 0444) >> 2;
         else
-            st.st_mode &= ~0111;
-        fchmod( fd->unix_fd, st.st_mode );
+            cached_st.st.st_mode &= ~0111;
+        fchmod( fd->unix_fd, cached_st.st.st_mode );
     }
 
     free( fd->nt_name );
-- 
GitLab

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

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about server/fd.c:
> +    {
> +        sprintf( tmpname, tmpname_fmt, tmp_value );
> +        if (fstatat( dirfd, tmpname, &st, 0 ))  /* tmpname doesn't exist */
> +            break;
> +    }
> +    if (i < 0x8000)
> +    {
> +        if (renameat( dirfd, srcname, dirfd, tmpname ))
> +        {
> +            file_set_error();
> +            goto ret;
> +        }
> +
> +        if (!is_dir)
> +        {
> +            if (linkat( dirfd, tmpname, dirfd, dstname, 0 ) && errno != EEXIST)
> To avoid bikesheds, maybe add a comment before this line:

```suggestion:-0+0
            /* we can't just do rename(2) here; renaming a file to its hardlink is no-op */
            if (linkat( dirfd, tmpname, dirfd, dstname, 0 ) && errno != EEXIST)
```

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97184

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about server/fd.c:
> +    /* same dentry means a no-op */
> +    if (!strcmp( srcname, dstname )) goto ret;
> +
> +    /* This is more complicated now, because in case of a casefold (+F) directory, the destination may very well be the same dentry,
> +     * even if the name doesn't match (if it differs just by case), in which case unlinking it is wrong and can be dangerous. Instead,
> +     * we first rename the source to a temporary filename in the same directory. If this is a casefold dir, this will also remove the
> +     * destination, otherwise the destination still exists. We then create a hardlink from the destination to our temporary name, and
> +     * finally, unlink the temporary. This still works if the directory is case sensitive, so it's not a problem in either case. */
> +    tmp_value += (current_time >> 16) + current_time;
> +    for (i = 0; i < 0x8000; i++, tmp_value += 7777)
> +    {
> +        sprintf( tmpname, tmpname_fmt, tmp_value );
> +        if (fstatat( dirfd, tmpname, &st, 0 ))  /* tmpname doesn't exist */
> +            break;
> +    }
> +    if (i < 0x8000)
> Better flip the condition for readability. Also needs to set error.

```suggestion:-0+0
    if (i >= 0x8000)
    {
        set_error( STATUS_UNSUCCESSFUL );
        goto ret;
    }
```

...then dedent the following code.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97183

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about server/fd.c:
> +    {
> +        if (!(dirname = memdup( dst, dstname - dst )))
> +            return;
> +        dirname[dstname - dst - 1] = '\0';
> +        if ((res = stat( dirname, &st )))
> +            file_set_error();
> +        free( dirname );
> +        if (res)
> +            return;
> +    }
> +
> +    srcname = strrchr( src, '/' ) + 1;
> +    if (!(dirname = memdup( src, srcname - src )))
> +        return;
> +    dirname[srcname - src - 1] = '\0';
> +    if ((dirfd = open( dirname, O_RDONLY | O_NONBLOCK )) == -1)
> Better open the destination file's directory instead of the source file's directory.

1. We may only have search access to src directory but not read access (--x).
2. `O_PATH` is only available on Linux-like OSes. `O_SEARCH` is also not cross platform. Notably, macOS lacks `O_PATH`.

To achieve this, we can switch the order: do stat() on src dirname, and open/fstat on dest dirname.

If different directories, it must be a hardlink, so we simply remove the source and bail out. If the same, switching order did nothing, and we can proceed as normal.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97185

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about server/fd.c:
> +            goto ret;
> +        }
> +    }
> +
> +    /* same dentry means a no-op */
> +    if (!strcmp( srcname, dstname )) goto ret;
> +
> +    /* This is more complicated now, because in case of a casefold (+F) directory, the destination may very well be the same dentry,
> +     * even if the name doesn't match (if it differs just by case), in which case unlinking it is wrong and can be dangerous. Instead,
> +     * we first rename the source to a temporary filename in the same directory. If this is a casefold dir, this will also remove the
> +     * destination, otherwise the destination still exists. We then create a hardlink from the destination to our temporary name, and
> +     * finally, unlink the temporary. This still works if the directory is case sensitive, so it's not a problem in either case. */
> +    tmp_value += (current_time >> 16) + current_time;
> +    for (i = 0; i < 0x8000; i++, tmp_value += 7777)
> +    {
> +        sprintf( tmpname, tmpname_fmt, tmp_value );
> You shouldn't use sprintf on macOS:

```
../server/fd.c:2615:9: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]
        sprintf( tmpname, tmpname_fmt, tmp_value );
        ^
/Applications/Xcode15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/stdio.h:180:1: note: 'sprintf' has been explicitly marked deprecated here
__deprecated_msg("This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead.")
^
/Applications/Xcode15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/cdefs.h:218:48: note: expanded from macro '__deprecated_msg'
        #define __deprecated_msg(_msg) __attribute__((__deprecated__(_msg)))
                                                      ^
1 warning generated.
```

(That one `sprintf` call in server/ptrace.c isn't compiled in macOS. All other uses of `sprintf` have been removed.)

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97181

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about server/fd.c:
> set_error( STATUS_OBJECT_PATH_SYNTAX_BAD );
>          return;
>      }
> +
> +    /* strip trailing slashes */
> +    while (nameptr[len - 1] == '/')

To avoid `MoveFile("Z:\\", "Z:\\")` from crashing:

```suggestion:-0+0
    while (len > 1 && nameptr[len - 1] == '/')
```

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97182

[Jinoh Kang (＠iamahuman)]

Sorry it took this long.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97186

[Jinoh Kang (＠iamahuman)]

On Sat Nov 30 03:20:12 2024 +0000, Jinoh Kang wrote:
> Encapsulating init flag looks better, but we can do better still by just
> reusing dev/ino information from `fd->inode`. This way we can reduce
> fstat() calls to just two (the latter of which is arguably more rare in practice)
> Any progress on this front?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97187

[Gabriel Ivăncescu]

On Sat Mar  8 14:00:06 2025 +0000, Jinoh Kang wrote:
> Any progress on this front?
> Sorry completely forgot about it.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97324

[Gabriel Ivăncescu]

I'm not sure how to do it here. I can use the source's fd->inode and
device but:

1. How do I check if it's a directory without `stat`ing it? (needed for
the first fstat() but also below at the `if (create_link || S_ISDIR(
st2.st_mode ))` line)

2. `unlink_closed_fd` appears to think it can mismatch. I mean it makes
sense (since in Linux we can remove open fds), didn't know we even guard
against it though. Should it be a concern here?

[Gabriel Ivăncescu]

On Sat Mar  8 13:58:45 2025 +0000, Jinoh Kang wrote:
> Better open the destination file's directory instead of the source
> file's directory.
> 1. We may only have search access to src directory but not read access (--x).
> 2. `O_PATH` is only available on Linux-like OSes. `O_SEARCH` is also not
> cross platform. Notably, macOS lacks `O_PATH`.
> To achieve this, we can switch the order: do stat() on src dirname, and
> open/fstat on dest dirname.
> If different directories, it must be a hardlink, so we simply remove the
> source and bail out. If the same, switching order did nothing, and we
> can proceed as normal.
> Yeah, I hadn't realized that possibility, although probably rare in practice. I probably used this order so I can use `unlinkat` when removing the source, but it doesn't really matter, I'll swap.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97325

[Gabriel Ivăncescu]

On Mon Mar 10 20:58:40 2025 +0000, Gabriel Ivăncescu wrote:
> Sorry completely forgot about it.
> I'm not sure how to do it here. I can use the source's fd->inode and device but:

1. How do I check if it's a directory without `stat`ing it? (needed for the first fstat() but also below at the `if (create_link || S_ISDIR( st2.st_mode ))` line)

2. `unlink_closed_fd` appears to think it can mismatch. I mean it makes sense (since in Linux we can remove open fds), didn't know we even guard against it though. Should it be a concern here?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97330

[Jinoh Kang (＠iamahuman)]

On Mon Mar 10 20:59:30 2025 +0000, Gabriel Ivăncescu wrote:
> Yeah, I hadn't realized that possibility, although probably rare in
> practice. I probably used this order so I can use `unlinkat` when
> removing the source, but it doesn't really matter, I'll swap.
> On second thought, the current order *is* fine. You should keep opening the source directory since that's what's being modified (file unlinked). Sorry about that.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97333

[Jinoh Kang (＠iamahuman)]

> below at the `if (create_link || S_ISDIR( st2.st_mode ))` line)

We can probably eliminate the check and just handle the EISDIR/EEXISTS error from link()/rename() themselves. Would make sense for your goal too, which is perf optimization.

The last stat (for the executable bit) is probably not called very frequently anyway, so I think it's fine to skip the cache there too.
> `unlink_closed_fd` appears to think it can mismatch. I mean it makes sense (since in Linux we can remove open fds), didn't know we even guard against it though. Should it be a concern here?

`unlink_closed_fd` is calling `stat( fd->unix_name, ... )`, not `fstat( fd->unix_fd, ... )`. Note there is a difference.

Unix names can be basically moved around and renamed, but fd's dev/ino will stay the same.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97353

[PATCH v6 0/5] MR6855: server: Fix renaming file to a hardlink of itself.

[Gabriel Ivăncescu]

On Mon Mar 10 23:32:18 2025 +0000, Jinoh Kang wrote:
> > below at the `if (create_link || S_ISDIR( st2.st_mode ))` line)
> > We can probably eliminate the check and just handle the EISDIR/EEXISTS
> > error from link()/rename() themselves. Would make sense for your goal
> > too, which is perf optimization.
> > The last stat (for the executable bit) is probably not called very
> > frequently anyway, so I think it's fine to skip the cache there too.
> > `unlink_closed_fd` appears to think it can mismatch. I mean it makes
> > sense (since in Linux we can remove open fds), didn't know we even guard
> > against it though. Should it be a concern here?
> > `unlink_closed_fd` is calling `stat( fd->unix_name, ... )`, not `fstat(
> > fd->unix_fd, ... )`. Note there is a difference.
> > Unix names can be basically moved around and renamed, but fd's dev/ino
> > will stay the same.
> > Yeah, my issue was inaccurate. The ISDIR check only happens if
> > create_link is false, but the first fstat happens only when it's true,
> > so they can't both happen, which is fine.

I changed these to better fit your suggestions and split into 2
patches. But I have no problem dropping them from this MR, if needed.
The important part is to fix the hardlink issue anyway.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97470