Bug 1530960 - Add return value checks when removing files in updater.cpp. r=mhowell
authorRobert Strong <robert.bugzilla@gmail.com>
Wed, 27 Feb 2019 21:42:54 +0000
changeset 519414 a09495fb2304c28b55d847f5af99c60565059047
parent 519413 32c7dc092efda4c745b4b1fe3590249d0d0513df
child 519415 c923d2b7817e1ca387fd822bcaeb77b4b5abbf18
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmhowell
bugs1530960
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1530960 - Add return value checks when removing files in updater.cpp. r=mhowell Differential Revision: https://phabricator.services.mozilla.com/D21396
toolkit/mozapps/update/updater/updater.cpp
--- a/toolkit/mozapps/update/updater/updater.cpp
+++ b/toolkit/mozapps/update/updater/updater.cpp
@@ -118,25 +118,25 @@ struct UpdateServerThreadArgs {
 BOOL PathAppendSafe(LPWSTR base, LPCWSTR extra);
 BOOL PathGetSiblingFilePath(LPWSTR destinationBuffer, LPCWSTR siblingFilePath,
                             LPCWSTR newFileName);
 #  include "updatehelper.h"
 
 // Closes the handle if valid and if the updater is elevated returns with the
 // return code specified. This prevents multiple launches of the callback
 // application by preventing the elevated process from launching the callback.
-#  define EXIT_WHEN_ELEVATED(path, handle, retCode)             \
-    {                                                           \
-      if (handle != INVALID_HANDLE_VALUE) {                     \
-        CloseHandle(handle);                                    \
-      }                                                         \
-      if (_waccess(path, F_OK) == 0 && NS_tremove(path) != 0) { \
-        ImpersonatedLogFinish();                                \
-        return retCode;                                         \
-      }                                                         \
+#  define EXIT_WHEN_ELEVATED(path, handle, retCode) \
+    {                                               \
+      if (handle != INVALID_HANDLE_VALUE) {         \
+        CloseHandle(handle);                        \
+      }                                             \
+      if (NS_tremove(path) && errno != ENOENT) {    \
+        ImpersonatedLogFinish();                    \
+        return retCode;                             \
+      }                                             \
     }
 #endif
 
 //-----------------------------------------------------------------------------
 
 // This BZ2_crc32Table variable lives in libbz2. We just took the
 // data structure from bz2 and created crctables.h
 
@@ -903,17 +903,21 @@ static int remove_recursive_on_reboot(co
   if (rv) {
     // This error is benign
     return rv;
   }
 
   if (!S_ISDIR(sInfo.st_mode)) {
     NS_tchar tmpDeleteFile[MAXPATHLEN];
     GetUUIDTempFilePath(deleteDir, L"rep", tmpDeleteFile);
-    NS_tremove(tmpDeleteFile);
+    if (NS_tremove(tmpDeleteFile) && errno != ENOENT) {
+      LOG(("remove_recursive_on_reboot: failed to remove temporary file: " LOG_S
+           ", err: %d",
+           tmpDeleteFile, errno));
+    }
     rv = rename_file(path, tmpDeleteFile, false);
     if (MoveFileEx(rv ? path : tmpDeleteFile, nullptr,
                    MOVEFILE_DELAY_UNTIL_REBOOT)) {
       LOG(
           ("remove_recursive_on_reboot: file will be removed on OS "
            "reboot: " LOG_S,
            rv ? path : tmpDeleteFile));
     } else {
@@ -1405,17 +1409,21 @@ int AddFile::Execute() {
 
 void AddFile::Finish(int status) {
   LOG(("FINISH ADD " LOG_S, mRelPath.get()));
   // Staged updates don't create backup files.
   if (!sStagedUpdate) {
     // When there is an update failure and a file has been added it is removed
     // here since there might not be a backup to replace it.
     if (status && mAdded) {
-      NS_tremove(mFile.get());
+      if (NS_tremove(mFile.get()) && errno != ENOENT) {
+        LOG(("non-fatal error after update failure removing added file: " LOG_S
+             ", err: %d",
+             mFile.get(), errno));
+      }
     }
     backup_finish(mFile.get(), mRelPath.get(), status);
   }
 }
 
 class PatchFile : public Action {
  public:
   PatchFile() : mPatchFile(nullptr), mPatchIndex(-1), buf(nullptr) {}
@@ -1449,21 +1457,18 @@ PatchFile::~PatchFile() {
   // but not until some indeterminate future time, and we want determinism.
   // Normally this happens at the end of Execute, when we close the stream;
   // this call is here in case Execute errors out.
 #ifdef XP_WIN
   if (mPatchStream) {
     UnlockFile((HANDLE)_get_osfhandle(fileno(mPatchStream)), 0, 0, -1, -1);
   }
 #endif
-
-  // delete the temporary patch file
-  if (spath[0]) {
-    NS_tremove(spath);
-  }
+  // Patch files are written to the <working_dir>/updating directory which is
+  // removed after the update has finished so don't delete patch files here.
 
   if (buf) {
     free(buf);
   }
 }
 
 int PatchFile::LoadSourceFile(FILE *ofile) {
   struct stat os;
@@ -1553,17 +1558,23 @@ int PatchFile::Prepare() {
   LOG(("PREPARE PATCH " LOG_S, mFileRelPath.get()));
 
   // extract the patch to a temporary file
   mPatchIndex = sPatchIndex++;
 
   NS_tsnprintf(spath, sizeof(spath) / sizeof(spath[0]),
                NS_T("%s/updating/%d.patch"), gWorkingDirPath, mPatchIndex);
 
-  NS_tremove(spath);
+  // The removal of pre-existing patch files here is in case a previous update
+  // crashed and left these files behind.
+  if (NS_tremove(spath) && errno != ENOENT) {
+    LOG(("failure removing pre-existing patch file: " LOG_S ", err: %d", spath,
+         errno));
+    return WRITE_ERROR;
+  }
 
   mPatchStream = NS_tfopen(spath, NS_T("wb+"));
   if (!mPatchStream) {
     return WRITE_ERROR;
   }
 
 #ifdef XP_WIN
   // Lock the patch file, so it can't be messed with between
@@ -1705,17 +1716,18 @@ int PatchFile::Execute() {
   // Make sure mPatchStream gets unlocked on Windows; the system will do that,
   // but not until some indeterminate future time, and we want determinism.
 #ifdef XP_WIN
   UnlockFile((HANDLE)_get_osfhandle(fileno(mPatchStream)), 0, 0, -1, -1);
 #endif
   // Set mPatchStream to nullptr to make AutoFile close the file,
   // so it can be deleted on Windows.
   mPatchStream = nullptr;
-  NS_tremove(spath);
+  // Patch files are written to the <working_dir>/updating directory which is
+  // removed after the update has finished so don't delete patch files here.
   spath[0] = NS_T('\0');
   free(buf);
   buf = nullptr;
 
   return rv;
 }
 
 void PatchFile::Finish(int status) {
@@ -2338,17 +2350,20 @@ static int ProcessReplaceRequest() {
   // old installation directory to the new installation directory.
   NS_tchar tmpLog[MAXPATHLEN];
   NS_tsnprintf(tmpLog, sizeof(tmpLog) / sizeof(tmpLog[0]),
                NS_T("%s/updates/last-update.log"), tmpDir);
   if (!NS_taccess(tmpLog, F_OK)) {
     NS_tchar destLog[MAXPATHLEN];
     NS_tsnprintf(destLog, sizeof(destLog) / sizeof(destLog[0]),
                  NS_T("%s/updates/last-update.log"), destDir);
-    NS_tremove(destLog);
+    if (NS_tremove(destLog) && errno != ENOENT) {
+      LOG(("non-fatal error removing log file: " LOG_S ", err: %d", destLog,
+           errno));
+    }
     NS_trename(tmpLog, destLog);
   }
 #endif
 
   LOG(("Now, remove the tmpDir"));
   rv = ensure_remove_recursive(tmpDir, true);
   if (rv) {
     LOG(("Removing tmpDir failed, err: %d", rv));
@@ -2480,18 +2495,17 @@ static void UpdateThreadFunc(void *param
         int retries = 0;
         while (retries++ < max_retries) {
 #  ifdef XP_WIN
           Sleep(100);
 #  else
           usleep(100000);
 #  endif
           // Continue after the continue file exists and is removed.
-          if (!NS_taccess(continueFilePath, F_OK) &&
-              !NS_tremove(continueFilePath)) {
+          if (!NS_tremove(continueFilePath)) {
             break;
           }
         }
         rv = OK;
       } else {
         rv = CopyInstallDirToDestDir();
       }
 #else
@@ -3055,18 +3069,17 @@ int NS_main(int argc, NS_tchar **argv) {
       NS_tsnprintf(updateLockFilePath,
                    sizeof(updateLockFilePath) / sizeof(updateLockFilePath[0]),
                    NS_T("%s.update_in_progress.lock"), argv[callbackIndex]);
     }
 
     // The update_in_progress.lock file should only exist during an update. In
     // case it exists attempt to remove it and exit if that fails to prevent
     // simultaneous updates occurring.
-    if (!_waccess(updateLockFilePath, F_OK) &&
-        NS_tremove(updateLockFilePath) != 0) {
+    if (NS_tremove(updateLockFilePath) && errno != ENOENT) {
       // Try to fall back to the old way of doing updates if a staged
       // update fails.
       if (sStagedUpdate || sReplaceRequest) {
         // Note that this could fail, but if it does, there isn't too much we
         // can do in order to recover anyways.
         WriteStatusFile("pending");
       }
       LOG(("Update already in progress! Exiting"));
@@ -3109,18 +3122,17 @@ int NS_main(int argc, NS_tchar **argv) {
         if (gUserToken && !impersonated) {
           fprintf(stderr,
                   "Unable to impersonate when creating elevated "
                   "lock file. Exiting\n");
           return 1;
         }
 #  endif
 
-        if (!_waccess(elevatedLockFilePath, F_OK) &&
-            NS_tremove(elevatedLockFilePath) != 0) {
+        if (NS_tremove(elevatedLockFilePath) && errno != ENOENT) {
           fprintf(stderr, "Unable to create elevated lock file! Exiting\n");
           return 1;
         }
 
         elevatedFileHandle = CreateFileW(
             elevatedLockFilePath, GENERIC_READ | GENERIC_WRITE, 0, nullptr,
             OPEN_ALWAYS, FILE_FLAG_DELETE_ON_CLOSE, nullptr);
       }
@@ -3482,18 +3494,17 @@ int NS_main(int argc, NS_tchar **argv) {
 
         // Don't attempt to launch the callback when the callback path is
         // longer than expected.
         EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 1);
         return 1;
       }
 
       // Make a copy of the callback executable so it can be read when patching.
-      NS_tremove(gCallbackBackupPath);
-      if (!CopyFileW(argv[callbackIndex], gCallbackBackupPath, true)) {
+      if (!CopyFileW(argv[callbackIndex], gCallbackBackupPath, false)) {
         DWORD copyFileError = GetLastError();
         LOG(("NS_main: failed to copy callback file " LOG_S
              " into place at " LOG_S,
              argv[callbackIndex], gCallbackBackupPath));
         ImpersonatedLogFinish();
         if (copyFileError == ERROR_ACCESS_DENIED) {
           WriteStatusFile(WRITE_ERROR_ACCESS_DENIED);
         } else {
@@ -3544,17 +3555,22 @@ int NS_main(int argc, NS_tchar **argv) {
                argv[callbackIndex]));
           ImpersonatedLogFinish();
           if (lastWriteError == ERROR_ACCESS_DENIED) {
             WriteStatusFile(WRITE_ERROR_ACCESS_DENIED);
           } else {
             WriteStatusFile(WRITE_ERROR_CALLBACK_APP);
           }
 
-          NS_tremove(gCallbackBackupPath);
+          if (NS_tremove(gCallbackBackupPath) && errno != ENOENT) {
+            LOG(
+                ("NS_main: unable to remove backup of callback app file, "
+                 "path: " LOG_S,
+                 gCallbackBackupPath));
+          }
           EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 1);
           LaunchCallbackApp(argv[5], argc - callbackIndex, argv + callbackIndex,
                             sUsingService);
           return 1;
         }
         LOG(
             ("NS_main: callback app file in use, continuing without "
              "exclusive access for executable file: " LOG_S,
@@ -3601,17 +3617,22 @@ int NS_main(int argc, NS_tchar **argv) {
   t.Join();
 
 #ifdef XP_WIN
   if (argc > callbackIndex && !sReplaceRequest) {
     if (callbackFile != INVALID_HANDLE_VALUE) {
       CloseHandle(callbackFile);
     }
     // Remove the copy of the callback executable.
-    NS_tremove(gCallbackBackupPath);
+    if (NS_tremove(gCallbackBackupPath) && errno != ENOENT) {
+      LOG(
+          ("NS_main: non-fatal error removing backup of callback app file, "
+           "path: " LOG_S,
+           gCallbackBackupPath));
+    }
   }
 
   if (!sStagedUpdate && !sReplaceRequest && _wrmdir(gDeleteDirPath)) {
     LOG(("NS_main: unable to remove directory: " LOG_S ", err: %d", DELETE_DIR,
          errno));
     // The directory probably couldn't be removed due to it containing files
     // that are in use and will be removed on OS reboot. The call to remove the
     // directory on OS reboot is done after the calls to remove the files so the
@@ -4189,17 +4210,18 @@ int DoUpdate() {
   // extract the manifest
   int rv = gArchiveReader.ExtractFile("updatev3.manifest", manifest);
   if (rv) {
     LOG(("DoUpdate: error extracting manifest file"));
     return rv;
   }
 
   NS_tchar *buf = GetManifestContents(manifest);
-  NS_tremove(manifest);
+  // The manifest is located in the <working_dir>/updating directory which is
+  // removed after the update has finished so don't delete it here.
   if (!buf) {
     LOG(("DoUpdate: error opening manifest file: " LOG_S, manifest));
     return READ_ERROR;
   }
   NS_tchar *rb = buf;
 
   ActionList list;
   NS_tchar *line;