On 2/26/07, Felix Nawothnig <flexo@holycrap.org> wrote:

Erich Hoover wrote:
> I assume you're referring to the file existence check and file delete,
> followed by the actual copy and move.  I implemented these checks in
> this manner in order to provide error codes consistent with the
> documentation.  I am not incredibly familiar with the Wine internals,
> but it looks to me like the functions called should handle a
> simultaneous call from another thread (without unnecessary code
> duplication).  For example, if the "replaced" file goes missing between
> the check and the CopyFileW call then CopyFileW will return an error.
> If the "replaced" file appears between the DeleteFileW call and the
> MoveFileExW call (since the MOVEFILE_REPLACE_EXISTING flag is not set),
> then the MoveFileExW call will fail.  The documentation's expected error
> codes for ReplaceFile seem to be consistent with these potential
> outcomes.

Reading MSDN that doesn't seem to be the case. The way I see it all
those errors documented could be caused by file permisions. MSDN doesn't
say anything about race cons.

Some notes about the patch:

+    /* Maker sure the "replacement" file exists before continuing */
+    if(!RtlDoesFileExists_U( lpReplacementFileName ))
+    {
+        SetLastError(ERROR_INVALID_PARAMETER);
+        return FALSE;
+    }

This would probably the right place to lock the file...

+    /*
+     * If the user wants a backup then that needs to be performed first
+     * * Do not fail to backup if "replaced" does not exist
+     */
+    if( lpBackupFileName && RtlDoesFileExists_U( lpReplacedFileName ) )

The call to RtlDoesFileExists_U() is redundant and introduces a race
con. CopyFileW() will check for existence, check GLE afterwards.

+    {
+        /* If an existing backup exists then copy over it */
+     */
+        if(!CopyFileW( lpReplacedFileName, lpBackupFileName, FALSE ))
+        {
+            SetLastError(ERROR_INVALID_PARAMETER);
+            return FALSE;
+        }
+    }
+    /*
+     * Now that the backup has been performed (if requested), copy the
replacement
+     * into place (make sure we can delete the original file first)
+     * * Do not fail if "replaced" does not exist
+     */
+    if(RtlDoesFileExists_U( lpReplacedFileName ) && !DeleteFileW(
lpReplacedFileName ))

Same here, DeleteFile() will check for existence.

+    {
+        SetLastError(ERROR_UNABLE_TO_REMOVE_REPLACED);
+        return FALSE;
+    }
+    if(!MoveFileExW( lpReplacementFileName, lpReplacedFileName, flags ))

Couldn't you just get rid of the DeleteFile, check why MoveFileEx()
failed and get rid of the call to RtlDoesFileExists_U(lpReplacedFileName)?

+    {
+        SetLastError(ERROR_UNABLE_TO_MOVE_REPLACEMENT);
+        return FALSE;
+    }
+    /* All done, file replacement successful */
+    return TRUE;