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;