From e5d5ee119e6410467e9f3256465f4c60fa328e11 Mon Sep 17 00:00:00 2001 From: Sean Budd Date: Thu, 15 Feb 2024 15:22:41 +1100 Subject: [PATCH] Attempt to remove exes in a batch --- appx/sconscript | 2 - source/buildVersion.py | 2 +- source/installer.py | 94 +++++++++++++++++++++++++++++------- tests/unit/test_installer.py | 46 ++++++++++++++++++ user_docs/en/changes.t2t | 7 +++ 5 files changed, 130 insertions(+), 21 deletions(-) create mode 100644 tests/unit/test_installer.py diff --git a/appx/sconscript b/appx/sconscript index d4ae2e8efd9..e2f5963cd33 100644 --- a/appx/sconscript +++ b/appx/sconscript @@ -63,7 +63,6 @@ signExec=env['signExec'] if env['certFile'] else None # Files from NVDA's distribution that cannot be included in the appx due to policy or security restrictions excludedDistFiles=[ 'nvda_eoaProxy.exe', - 'nvda_service.exe', 'nvda_slave.exe', 'nvda_noUIAccess.exe', 'lib/IAccessible2Proxy.dll', @@ -107,4 +106,3 @@ if signExec and not isStoreSubmission: env.AddPostAction(appx,signExec) Return(['appx']) - diff --git a/source/buildVersion.py b/source/buildVersion.py index ed4f8e36219..ec823123174 100644 --- a/source/buildVersion.py +++ b/source/buildVersion.py @@ -67,7 +67,7 @@ def formatVersionForGUI(year, major, minor): name = "NVDA" version_year = 2023 version_major = 3 -version_minor = 3 +version_minor = 4 version_build = 0 # Should not be set manually. Set in 'sconscript' provided by 'appVeyor.yml' version=_formatDevVersionString() publisher="unknown" diff --git a/source/installer.py b/source/installer.py index f809bf2b3e8..7797c22fdbd 100644 --- a/source/installer.py +++ b/source/installer.py @@ -2,9 +2,10 @@ # A part of NonVisual Desktop Access (NVDA) # This file is covered by the GNU General Public License. # See the file COPYING for more details. -# Copyright (C) 2011-2023 NV Access Limited, Joseph Lee, Babbage B.V., Łukasz Golonka +# Copyright (C) 2011-2024 NV Access Limited, Joseph Lee, Babbage B.V., Łukasz Golonka import ctypes +import pathlib import winreg import time import os @@ -22,8 +23,8 @@ import COMRegistrationFixes import winKernel from typing import ( - Any, Dict, + Iterable, Union, ) import NVDAState @@ -205,6 +206,7 @@ def removeOldProgramFiles(destPath): for fn in files: fn = os.path.join(root, fn) # No need to use tryRemoveFile here because these files should never be locked. + # TODO: should we use tryRemoveFile anyway here? if os.path.isdir(fn): shutil.rmtree(fn) else: @@ -566,7 +568,13 @@ def _deleteKeyAndSubkeys(key, subkey): class RetriableFailure(Exception): pass -def tryRemoveFile(path,numRetries=6,retryInterval=0.5,rebootOK=False): + +def tryRemoveFile( + path: str, + numRetries: int = 6, + retryInterval: float = 0.5, + rebootOK: bool = False +): dirPath=os.path.dirname(path) tempPath=tempfile.mktemp(dir=dirPath) try: @@ -581,7 +589,7 @@ def tryRemoveFile(path,numRetries=6,retryInterval=0.5,rebootOK=False): os.remove(tempPath) return except OSError: - pass + log.debugWarning(f"Failed to delete file {tempPath}, attempt {count}/{numRetries}", exc_info=True) time.sleep(retryInterval) if rebootOK: log.debugWarning("Failed to delete file %s, marking for delete on reboot"%tempPath) @@ -592,12 +600,13 @@ def tryRemoveFile(path,numRetries=6,retryInterval=0.5,rebootOK=False): # #9847: Move file to None to delete it. winKernel.moveFileEx(pathQualifier+tempPath,None,winKernel.MOVEFILE_DELAY_UNTIL_REBOOT) except WindowsError: - log.debugWarning("Failed to delete file %s, marking for delete on reboot"%tempPath, exc_info=True) - return + log.debugWarning(f"Failed to mark file {tempPath} for delete on reboot", exc_info=True) + else: + return try: os.rename(tempPath,path) - except: - log.error("Unable to rename back to %s before retriable failier"%path) + except Exception: + log.exception(f"Unable to rename back to {path} before retriable failure") raise RetriableFailure("File %s could not be removed"%path) def tryCopyFile(sourceFilePath,destFilePath): @@ -622,9 +631,60 @@ def tryCopyFile(sourceFilePath,destFilePath): raise OSError("Unable to copy file %s to %s, error %d"%(sourceFilePath,destFilePath,errorCode)) +_nvdaExes = { + "nvda.exe", + "nvda_noUIAccess.exe", + "nvda_uiAccess.exe", + "nvda_dmp.exe", + "nvda_slave.exe" +} + + +def _revertGroupDelete(tempDir: str, installDir: str): + """Move all files in tempDir back to installDir, retaining the same relative path""" + for tempFile in pathlib.Path(tempDir).rglob("*"): + relativePath = tempFile.relative_to(tempDir) + originalPath = os.path.join(installDir, relativePath.as_posix()) + try: + os.rename(tempFile.absolute(), originalPath) + except OSError: + log.exception(f"Failed to rename {tempFile} back to {originalPath}") + + +def _deleteInstallerFileGroupOrFail(installDir: str, relativeFilepaths: Iterable[str]): + """ + Delete a group of files in the installer folder. + If any file fails to be deleted, revert the deletion of all other files. + + :param installDir: an iterable of file paths relative to installDir + :param relativeFilepaths: an iterable of file paths relative to installDir + + :raises RetriableFailure: if the files fail to be deleted. + """ + tempDir = tempfile.mkdtemp() + for filepath in relativeFilepaths: + originalPath = os.path.join(installDir, filepath) + if not pathlib.Path(originalPath).exists(): + log.debug(f"Skipping remove for non-existent file: {originalPath}") + continue + tempPath = os.path.join(tempDir, filepath) + pathlib.Path(tempPath).parent.mkdir(parents=True, exist_ok=True) + try: + os.rename(originalPath, tempPath) + except OSError: + log.exception(f"Failed to move {originalPath} to {tempPath}") + _revertGroupDelete(tempDir, installDir) + raise RetriableFailure("Failed to move files to temp directory for deletion") + + try: + shutil.rmtree(tempDir) + except OSError: + # Ignore this failure, as the temp directory should get deleted eventually + log.debugWarning(f"Failed to remove temp directory {tempDir}", exc_info=True) + + def install(shouldCreateDesktopShortcut: bool = True, shouldRunAtLogon: bool = True): prevInstallPath=getInstallPath(noDefault=True) - unregisterInstallation(keepDesktopShortcut=shouldCreateDesktopShortcut) installDir=defaultInstallPath startMenuFolder=defaultStartMenuFolder # Remove all the main executables always. @@ -633,10 +693,11 @@ def install(shouldCreateDesktopShortcut: bool = True, shouldRunAtLogon: bool = T # so we shouldn't proceed. # 2. The appropriate executable for nvda.exe will be determined by # which executables exist after copying program files. - for f in ("nvda.exe","nvda_noUIAccess.exe","nvda_UIAccess.exe","nvda_service.exe","nvda_slave.exe"): - f=os.path.join(installDir,f) - if os.path.isfile(f): - tryRemoveFile(f) + # Some exes are no longer used, but we remove them anyway from legacy copies. + # nvda_service.exe was removed in 2017.4 (#7625). + # TODO: nvda_eoaProxy.exe should be added to this list in 2024.1 (#15544). + _deleteInstallerFileGroupOrFail(prevInstallPath, _nvdaExes.union({"nvda_service.exe"})) + unregisterInstallation(keepDesktopShortcut=shouldCreateDesktopShortcut) if prevInstallPath: removeOldLoggedFiles(prevInstallPath) removeOldProgramFiles(installDir) @@ -675,11 +736,8 @@ def removeOldLoggedFiles(installPath): def createPortableCopy(destPath,shouldCopyUserConfig=True): assert os.path.isabs(destPath), f"Destination path {destPath} is not absolute" - #Remove all the main executables always - for f in ("nvda.exe","nvda_noUIAccess.exe","nvda_UIAccess.exe"): - f=os.path.join(destPath,f) - if os.path.isfile(f): - tryRemoveFile(f) + # Remove all the main executables always + _deleteInstallerFileGroupOrFail(destPath, {"nvda.exe", "nvda_noUIAccess.exe", "nvda_UIAccess.exe"}) removeOldProgramFiles(destPath) copyProgramFiles(destPath) tryCopyFile(os.path.join(destPath,"nvda_noUIAccess.exe"),os.path.join(destPath,"nvda.exe")) diff --git a/tests/unit/test_installer.py b/tests/unit/test_installer.py new file mode 100644 index 00000000000..24f1a26d167 --- /dev/null +++ b/tests/unit/test_installer.py @@ -0,0 +1,46 @@ +# A part of NonVisual Desktop Access (NVDA) +# This file is covered by the GNU General Public License. +# See the file COPYING for more details. +# Copyright (C) 2024 NV Access Limited + +"""Unit tests for the installer module. +""" + +import pathlib +import tempfile +import unittest + +import installer + +class Test_BatchDeletion(unittest.TestCase): + """A test for the extension points on the input manager.""" + + def setUp(self) -> None: + self._originalTempDir = tempfile.mkdtemp() + self._sampleFiles = [ + "file1.txt", + "file2.txt", + "file3.txt", + ] + for file in self._sampleFiles: + pathlib.Path(self._originalTempDir, file).touch(exist_ok=False) + + def tearDown(self) -> None: + for file in self._sampleFiles: + f = pathlib.Path(self._originalTempDir, file) + if f.exists(): + f.unlink() + + def test_deleteFilesSuccess(self): + installer._deleteInstallerFileGroupOrFail(self._originalTempDir, self._sampleFiles) + for file in self._sampleFiles: + self.assertFalse(pathlib.Path(self._originalTempDir, file).exists()) + + def test_deleteFilesFailure(self): + with self.assertRaises(installer.RetriableFailure): + with open(pathlib.Path(self._originalTempDir, self._sampleFiles[1]), "r"): + installer._deleteInstallerFileGroupOrFail(self._originalTempDir, self._sampleFiles) + + # Assert files are back where they started + for file in self._sampleFiles: + self.assertTrue(pathlib.Path(self._originalTempDir, file).exists()) diff --git a/user_docs/en/changes.t2t b/user_docs/en/changes.t2t index e1cd81c612a..9b5814150eb 100644 --- a/user_docs/en/changes.t2t +++ b/user_docs/en/changes.t2t @@ -4,6 +4,13 @@ What's New in NVDA %!includeconf: ../changes.t2tconf %!includeconf: ./locale.t2tconf += 2023.3.4 = +This is a patch release to fix an installer issue. + +== Bug Fixes == +- Fixes bug which caused NVDA installation to fail to an unrecoverable state. (#16122) +- + = 2023.3.3 = This is a patch release to fix a security issue. Please responsibly disclose security issues following NVDA's [security policy https://github.com/nvaccess/nvda/blob/master/security.md].