Skip to content

Commit

Permalink
Convert all (remaining) format exporters to safe-overwrite strategy
Browse files Browse the repository at this point in the history
I'm excited to finally have this finished.  Here's the gist:

1) User opens file vacation.jpg in PhotoDemon
2) User makes changes to vacation.jpg, then clicks Save

Some old code paths in PD would delete vacation.jpg, then save a new copy.  But what happens if there's a power outage in the middle of this process?

Now, these code paths have all been rewritten to leave vacation.jpg untouched and save to a temporary file.  After the temporary file has been saved *and* verified, the ReplaceFileW API is used to replace the original vacation.jpg with the temporary file, with full support for rollbacks in case of e.g. a power outage during the replace.

This guarantees that PD can never lose original data, even in the case of power failures, PC crashes, meteor strikes, etc.
  • Loading branch information
tannerhelland committed Jun 22, 2022
1 parent 6b769ea commit 18a6a15
Show file tree
Hide file tree
Showing 8 changed files with 346 additions and 92 deletions.
79 changes: 46 additions & 33 deletions Classes/pdFSO.cls
Original file line number Diff line number Diff line change
Expand Up @@ -1044,18 +1044,23 @@ Friend Function FileReadData(ByVal srcHandle As Long, ByVal ptrToDestinationObje

End Function

'Replace an arbitrary file with another arbitrary file, with optional backup file of the original created during the process.
'Replace an arbitrary file with another arbitrary file, with an optional backup file of the original
' created during the process.
'
'All files must reside on the same volume. The target file should exist, but to be safe, I have added some custom code to address instances
' where the target file does not exist (this function will silently fall back to a "Move file" action instead).
'All files must reside on the same volume. The target file should exist, but to be safe, I have added
' some custom code to address instances where the target file does not exist (this function will silently
' fall back to a "Move file" action instead).
'
'IMPORTANT NOTE: the ReplaceFileW function has rare-but-not-impossible failure outcomes where the original file is deleted, but the new file is
' not renamed. Because this failure could be catastrophic during an update process, PD will forcibly create backups of any files
' patched via this function, and restore them as necessary. The "customBackupFile" parameter only exists if you want to specify
' a CUSTOM backup filename + location; it does not affect whether or not a backup IS created. (TL;DR - backups are always created.)
'IMPORTANT NOTE: the ReplaceFileW function has rare-but-not-impossible failure outcomes where the original
' file is deleted, but the new file is not renamed. Because this failure could be
' catastrophic during an update process, PD will forcibly create backups of any files patched
' via this function and restore them as necessary. The "customBackupFile" parameter only exists
' if you want to specify a CUSTOM backup filename + location; it does not affect whether or not
' a backup IS created. (TL;DR - backups are always created.)
Friend Function FileReplace(ByVal oldFile As String, ByVal newFile As String, Optional ByVal customBackupFile As String = vbNullString) As PD_FILE_PATCH_RESULT

'Because this function is capable of botching an existing PD install if catastrophic failure occurs, error handling must be comprehensive.
'Because this function is capable of botching an existing PD install if catastrophic failure occurs,
' error handling must be comprehensive.
On Error GoTo ReplaceFailure

Dim backupFile As String
Expand All @@ -1067,8 +1072,9 @@ Friend Function FileReplace(ByVal oldFile As String, ByVal newFile As String, Op
'Consider forcibly specifying a backup path if the user didn't provide their own
If (LenB(customBackupFile) = 0) Then

'IMPORTANT NOTE! If the files you're patching are critical, I STRONGLY recommend forcibly creating a backup here.
' Otherwise, you risk a scenario where something goes wrong during the replace step, and the original file is gone forever.
'IMPORTANT NOTE! If the files you're patching are critical, I STRONGLY recommend forcibly creating
' a backup here. Otherwise, you risk a scenario where something goes wrong during the replace step,
' and the original file is lost forever.

'For example, in PhotoDemon I use the standard Data/Updates folder for backups when patching, like this:
'backupFile = UserPrefs.GetUpdatePath & FileGetName(oldFile)
Expand All @@ -1079,8 +1085,9 @@ Friend Function FileReplace(ByVal oldFile As String, ByVal newFile As String, Op
backupFile = customBackupFile
End If

'ReplaceFileW returns a non-zero value if successful, or zero if it fails. Because this function is used to patch PhotoDemon.exe,
' there is no margin for failure, so detailed handling of every possible outcome is required.
'ReplaceFileW returns a non-zero value if successful, or zero if it fails. Because this function is used
' to patch PhotoDemon.exe, there is no margin for failure, so detailed handling of every possible outcome
' is necessary.
rfReturn = ReplaceFileW(StrPtr(oldFile), StrPtr(newFile), StrPtr(backupFile), REPLACEFILE_IGNORE_MERGE_ERRORS, 0&, 0&)

'Success means we can exit immediately
Expand All @@ -1096,15 +1103,17 @@ Friend Function FileReplace(ByVal oldFile As String, ByVal newFile As String, Op

Select Case Err.LastDllError

'The replacement file could not be renamed. If lpBackupFileName was specified, the replaced and replacement files retain their original
' file names. Otherwise, the replaced file no longer exists and the replacement file exists under its original name.
'The replacement file could not be renamed. If lpBackupFileName was specified, the replaced and
' replacement files retain their original file names. Otherwise, the replaced file no longer exists
' and the replacement file exists under its original name.
Case ERROR_UNABLE_TO_MOVE_REPLACEMENT

'See if the old file exists
If Me.FileExists(oldFile) Then

'The old file exists. Remove the backup file (if any), then exit. Because the system is in an identical place as it was prior
' to this function being invoked, the caller is free to try again.
'The old file exists. Remove the backup file (if any), then exit. Because the system is in
' an identical place as it was prior to this function being invoked, the caller is free to
' try again.
Me.FileDelete customBackupFile
FileReplace = FPR_FAIL_NOTHING_CHANGED
Exit Function
Expand All @@ -1117,8 +1126,8 @@ Friend Function FileReplace(ByVal oldFile As String, ByVal newFile As String, Op
'The backup file exists. Try to restore it to the location of the original file.
mfReturn = MoveFileExW(StrPtr(backupFile), StrPtr(oldFile), MOVEFILE_REPLACE_EXISTING Or MOVEFILE_COPY_ALLOWED)

'The move succeeded! Perform a failsafe deletion of the backup file, then notify the caller that the system has been
' restored to its original state.
'The move succeeded! Perform a failsafe deletion of the backup file, then notify the
' caller that the system has been restored to its original state.
If (mfReturn <> 0) Then
Me.FileDelete backupFile
FileReplace = FPR_FAIL_NOTHING_CHANGED
Expand All @@ -1140,16 +1149,18 @@ Friend Function FileReplace(ByVal oldFile As String, ByVal newFile As String, Op

End If

'The replacement file could not be moved. The replacement file still exists under its original name; however, it has inherited the file streams
' and attributes from the file it is replacing. The file to be replaced still exists with a different name. If lpBackupFileName is specified,
'The replacement file could not be moved. The replacement file still exists under its original name;
' however, it has inherited the file streams and attributes from the file it is replacing.
' The file to be replaced still exists with a different name. If lpBackupFileName is specified,
' it will be the name of the replaced file.
Case ERROR_UNABLE_TO_MOVE_REPLACEMENT_2

'See if the old file exists; this should never happen.
If Me.FileExists(oldFile) Then

'The old file exists. Remove the backup file (if any), then exit. Because the system is in an identical place as it was prior
' to this function being invoked, the caller is free to try again.
'The old file exists. Remove the backup file (if any), then exit. Because the system is in
' an identical place as it was prior to this function being invoked, the caller is free to
' try again.
Me.FileDelete customBackupFile

FSOError "FileReplace", "Impossible outcome 3 occurred in FileReplace. Please investigate."
Expand All @@ -1158,14 +1169,15 @@ Friend Function FileReplace(ByVal oldFile As String, ByVal newFile As String, Op

Else

'The old file no longer exists. Restore the backup file. (The backup file is supposedly guaranteed to exist with this error code.)
'The old file no longer exists. Restore the backup file.
' (The backup file is supposedly guaranteed to exist with this error code.)
If Me.FileExists(backupFile) Then

'The backup file exists. Try to restore it to the location of the original file.
mfReturn = MoveFileExW(StrPtr(backupFile), StrPtr(oldFile), MOVEFILE_REPLACE_EXISTING Or MOVEFILE_COPY_ALLOWED)

'The move succeeded! Perform a failsafe deletion of the backup file, then notify the caller that the system has been
' restored to its original state.
'The move succeeded! Perform a failsafe deletion of the backup file, then notify
' the caller that the system has been restored to its original state.
If (mfReturn <> 0) Then
Me.FileDelete backupFile
FileReplace = FPR_FAIL_NOTHING_CHANGED
Expand Down Expand Up @@ -1195,8 +1207,8 @@ Friend Function FileReplace(ByVal oldFile As String, ByVal newFile As String, Op
FileReplace = FPR_FAIL_NOTHING_CHANGED
Exit Function

'Other failure states are more problematic. Defer to the primary error handler, which will try to restore the
' original file setup as best as it can.
'Other failure states are more problematic. Defer to the primary error handler,
' which will try to restore the original file setup as best as it can.
Case Else
PDDebug.LogAction "pdFSO.ReplaceFile last DLL error: " & Err.LastDllError
GoTo ReplaceFailure
Expand Down Expand Up @@ -1230,15 +1242,16 @@ Friend Function FileReplace(ByVal oldFile As String, ByVal newFile As String, Op
Exit Function


'Arbitrary failure states are problematic. Basically, our goal is to restore the original file setup as best as we can. We do this by attempting
' to restore the backup file (if it exists), and applying any renames or moves required to get everything back to the way it was.
'Arbitrary failure states are problematic. Basically, our goal is to restore the original file setup
' as best as we can. We do this by attempting to restore the backup file (if it exists), and applying
' any renames or moves required to get everything back to the way it was.
ReplaceFailure:

'See if the old file exists
If Me.FileExists(oldFile) Then

'The old file exists. Remove the backup file (if any), then exit. Because the system is in an identical place as it was prior
' to this function being invoked, the caller is free to try again.
'The old file exists. Remove the backup file (if any), then exit. Because the system is in an
' identical place as it was prior to this function being invoked, the caller is free to try again.
Me.FileDelete customBackupFile

If Me.FileExists(newFile) Then
Expand All @@ -1258,8 +1271,8 @@ ReplaceFailure:
'The backup file exists. Try to restore it to the location of the original file.
mfReturn = MoveFileExW(StrPtr(backupFile), StrPtr(oldFile), MOVEFILE_REPLACE_EXISTING Or MOVEFILE_COPY_ALLOWED)

'The move succeeded! Perform a failsafe deletion of the backup file, then notify the caller that the system has been
' restored to its original state.
'The move succeeded! Perform a failsafe deletion of the backup file, then notify the caller
' that the system has been restored to its original state.
If (mfReturn <> 0) Then

Me.FileDelete backupFile
Expand Down
4 changes: 3 additions & 1 deletion Forms/Image_CreateLUT.frm
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,9 @@ Public Sub CreateDifferenceLUT(ByRef listOfParameters As String)
' the original file remains untouched).
Dim tmpFilename As String
If Files.FileExists(dstFilename) Then
tmpFilename = dstFilename & Hex$(PDMath.GetCompletelyRandomInt()) & ".pdtmp"
Do
tmpFilename = dstFilename & Hex$(PDMath.GetCompletelyRandomInt()) & ".pdtmp"
Loop While Files.FileExists(tmpFilename)
Else
tmpFilename = dstFilename
End If
Expand Down
38 changes: 30 additions & 8 deletions Modules/GDIPlus.bas
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ Attribute VB_Name = "GDI_Plus"
'GDI+ Interface
'Copyright 2012-2022 by Tanner Helland
'Created: 1/September/12
'Last updated: 16/February/22
'Last update: add wrapper function for filling regions
'Last updated: 21/June/22
'Last update: ensure GDI+ image export uses safe overwriting (e.g. do not overwrite an existing file
' until we verify that the export was successful)
'
'This interface provides a means for interacting with various GDI+ features. GDI+ was originally
' used as a fallback for image loading and saving if the FreeImage DLL was not found, but over time
Expand Down Expand Up @@ -2545,9 +2546,6 @@ Public Function GDIPlusSavePicture(ByRef srcPDImage As pdImage, ByVal dstFilenam
End With

End Select

'With our encoder prepared, we can finally continue with the save
Files.FileDeleteIfExists dstFilename

'Convert our list of params to a format GDI+ understands.
Dim tmpEncodeParams() As Byte, tmpEncodeParamSize As Long
Expand All @@ -2569,11 +2567,35 @@ Public Function GDIPlusSavePicture(ByRef srcPDImage As pdImage, ByVal dstFilenam
Next i
End If

'If the target file already exists, use "safe" file saving (e.g. write the save data to a new file,
' and if it's saved successfully, overwrite the original file - this way, if an error occurs mid-save,
' the original file remains untouched).
Dim tmpFilename As String
If Files.FileExists(dstFilename) Then
Do
tmpFilename = dstFilename & Hex$(PDMath.GetCompletelyRandomInt()) & ".pdtmp"
Loop While Files.FileExists(tmpFilename)
Else
tmpFilename = dstFilename
End If

'Pass all completed structs to GDI+ and let it handle everything from here
Dim gpReturn As GP_Result
gpReturn = GdipSaveImageToFile(hImage, StrPtr(dstFilename), VarPtr(exportGuid(0)), VarPtr(tmpEncodeParams(0)))

If (gpReturn <> GP_OK) Then
gpReturn = GdipSaveImageToFile(hImage, StrPtr(tmpFilename), VarPtr(exportGuid(0)), VarPtr(tmpEncodeParams(0)))

If (gpReturn = GP_OK) Then

'Safe saving: if the destination file already existed, attempt to replace it now
If Strings.StringsNotEqual(dstFilename, tmpFilename) Then
Dim overwriteOK As Boolean
overwriteOK = (Files.FileReplace(dstFilename, tmpFilename) = FPR_SUCCESS)
If (Not overwriteOK) Then
Files.FileDelete tmpFilename
PDDebug.LogAction "WARNING! Safe save did not overwrite original file (is it open elsewhere?)"
End If
End If

Else
InternalGDIPlusError "GdipSaveImageToFile", "GDI+ failure", gpReturn
GDI_Plus.ReleaseGDIPlusImage hImage
GDIPlusSavePicture = False
Expand Down
Loading

0 comments on commit 18a6a15

Please sign in to comment.