Re: kernel32: Implement ReplaceFileA/ReplaceFileW

[Felix Nawothnig]

Erich Hoover wrote:
> Real Name:
>     Erich Hoover
> Description:
>     Implements the functions ReplaceFileA and ReplaceFileW in kernel32 
> (Bug #7544).  Also provides conformance test code to ensure proper 
> functionality.
> Changelog:
>     kernel32: Implement ReplaceFileA/ReplaceFileW

Your patch seems to introduce lots of race cons. It's possible these 
exist on Win32 too but can't we avoid them?

kernel32: Implement ReplaceFileA/ReplaceFileW

[Erich Hoover]

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.  I apologize if there's something I'm missing here, please let me
know.

Erich Hoover
[e-mail]

On 2/26/07, Felix Nawothnig  wrote:
> Erich Hoover wrote:
> > Real Name:
> >     Erich Hoover
> > Description:
> >     Implements the functions ReplaceFileA and ReplaceFileW in kernel32
> > (Bug #7544).  Also provides conformance test code to ensure proper
> > functionality.
> > Changelog:
> >     kernel32: Implement ReplaceFileA/ReplaceFileW
>
> Your patch seems to introduce lots of race cons. It's possible these
> exist on Win32 too but can't we avoid them?

kernel32: Implement ReplaceFileA/ReplaceFileW

[Felix Nawothnig]

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;

kernel32: Implement ReplaceFileA/ReplaceFileW

[Erich Hoover]

Is the attached more like what you're looking for?

Erich Hoover
[e-mail]

On 2/26/07, Felix Nawothnig  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;

kernel32: Implement ReplaceFileA/ReplaceFileW

[Felix Nawothnig]

Erich Hoover wrote:
> Is the attached more like what you're looking for?

I did some investigation and... actually I'm looking for an equivalent 
to the linux splice syscall. Win32 is the most braindead API ever. Duh.

The "right" way would probably to do the copying yourself by 
read/write.. but I dunno. Your code looks solid at the first glance.

Three things:
  - I'd remove the FILE_SHARE_DELETE and FILE_SHARE_WRITE.
  - Use MoveFile instead of CopyFile/DeleteFile at the end?
  - Are you sure you're supposed to set ERROR_INVALID_PARAMETER no matter
    what bad happens? If the function which failed already set LE you
    should probably keep it...

Felix