Windows: Fix a regression in the previous release that could cause files to be deleted if one of the files/folders was locked ina nother program while changing title/author in calibre

src/calibre/utils/copy_files.py

@@ -90,11 +90,12 @@ class WindowsFileCopier:
 
     def _open_file(self, path: str, retry_on_sharing_violation: bool = True, is_folder: bool = False) -> 'winutil.Handle':
         flags = winutil.FILE_FLAG_BACKUP_SEMANTICS if is_folder else winutil.FILE_FLAG_SEQUENTIAL_SCAN
+        access_flags = winutil.GENERIC_READ
         if self.delete_all:
-            flags |= winutil.FILE_FLAG_DELETE_ON_CLOSE
+            access_flags |= winutil.DELETE
+        share_flags = winutil.FILE_SHARE_DELETE if self.allow_move else 0
         try:
-            return winutil.create_file(make_long_path_useable(path), winutil.GENERIC_READ,
+            return winutil.create_file(make_long_path_useable(path), access_flags, share_flags, winutil.OPEN_EXISTING, flags)
-                                       winutil.FILE_SHARE_DELETE if self.allow_move else 0, winutil.OPEN_EXISTING, flags)
         except OSError as e:
             if e.winerror == winutil.ERROR_SHARING_VIOLATION:
                 # The file could be a hardlink to an already opened file,
@@ -106,13 +107,12 @@ class WindowsFileCopier:
                 if retry_on_sharing_violation:
                     time.sleep(WINDOWS_SLEEP_FOR_RETRY_TIME)
                     return self._open_file(path, False, is_folder)
-                err = IOError(errno.EACCES,
+                err = IOError(errno.EACCES, _('File {} is open in another program').format(path))
-                        _('File is open in another program'))
                 err.filename = path
                 raise err from e
             raise
 
-    def __enter__(self) -> None:
+    def open_all_handles(self) -> None:
         for path, file_id in self.path_to_fileid_map.items():
             self.fileid_to_paths_map[file_id].add(path)
         for src in self.copy_map:
@@ -120,11 +120,36 @@ class WindowsFileCopier:
         for path in self.folders:
             self.folder_to_handle_map[path] = self._open_file(path, is_folder=True)
 
+    def __enter__(self) -> None:
+        try:
+            self.open_all_handles()
+        except OSError:
+            self.close_all_handles()
+            raise
+
+    def close_all_handles(self, delete_on_close: bool = False) -> None:
+        while self.path_to_handle_map:
+            path, h = next(iter(self.path_to_handle_map.items()))
+            if delete_on_close:
+                winutil.set_file_handle_delete_on_close(h, True)
+            h.close()
+            self.path_to_handle_map.pop(path)
+        while self.folder_to_handle_map:
+            path, h = next(reversed(self.folder_to_handle_map.items()))
+            if delete_on_close:
+                try:
+                    winutil.set_file_handle_delete_on_close(h, True)
+                except OSError as err:
+                    # Ignore dir not empty errors. Should never happen but we
+                    # ignore it as the UNIX semantics are to no delete folders
+                    # during __exit__ anyway and we dont want to leak the handle.
+                    if err.winerror != winutil.ERROR_DIR_NOT_EMPTY:
+                        raise
+            h.close()
+            self.folder_to_handle_map.pop(path)
+
     def __exit__(self, exc_type, exc_val, exc_tb) -> None:
-        for h in self.path_to_handle_map.values():
+        self.close_all_handles(delete_on_close=self.delete_all and exc_val is None)
-            h.close()
-        for h in reversed(self.folder_to_handle_map.values()):
-            h.close()
 
     def copy_all(self) -> None:
         for src_path, dest_path in self.copy_map.items():

src/calibre/utils/copy_files_test.py

@@ -95,16 +95,27 @@ class TestCopyFiles(unittest.TestCase):
         self.reset()
         src, dest = self.s(), self.d()
         if iswindows:
-            with open(self.s('sub/a')) as locked:
+            os.mkdir(self.s('lockdir'))
+            open(self.s('lockdir/lockfile'), 'w').close()
+            before = frozenset(walk(src))
+            with open(self.s('lockdir/lockfile')) as locked:
                 locked
-                self.assertRaises(IOError, copy_tree, src, dest)
+                self.assertRaises(IOError, copy_tree, src, dest, delete_source=True)
-                self.ae(os.listdir(self.d()), ['sub'])
+                self.ae(set(os.listdir(self.d())), {'sub', 'lockdir'})
                 self.assertFalse(tuple(walk(self.d())))
-            h = winutil.create_file(self.s('sub'), winutil.GENERIC_READ, 0, winutil.OPEN_EXISTING, winutil.FILE_FLAG_BACKUP_SEMANTICS)
+            self.ae(before, frozenset(walk(src)), 'Source files were deleted despite there being an error')
+
+            shutil.rmtree(dest)
+            os.mkdir(dest)
+            h = winutil.create_file(
+                self.s('lockdir'), winutil.GENERIC_READ|winutil.GENERIC_WRITE|winutil.DELETE,
+                winutil.FILE_SHARE_READ|winutil.FILE_SHARE_WRITE|winutil.FILE_SHARE_DELETE, winutil.OPEN_EXISTING,
+                winutil.FILE_FLAG_BACKUP_SEMANTICS)
             with closing(h):
-                self.assertRaises(IOError, copy_tree, src, dest)
+                self.assertRaises(IOError, copy_tree, src, dest, delete_source=True)
-                self.ae(os.listdir(self.d()), ['sub'])
+                self.ae(set(os.listdir(self.d())), {'sub', 'lockdir'})
                 self.assertFalse(tuple(walk(self.d())))
+            self.ae(before, frozenset(walk(src)), 'Source files were deleted despite there being an error')
 
 def find_tests():
     return unittest.defaultTestLoader.loadTestsFromTestCase(TestCopyFiles)

src/calibre/utils/windows/winutil.cpp

@@ -490,6 +490,17 @@ winutil_set_file_pointer(PyObject *self, PyObject *args) {
     return PyLong_FromLongLong(ans.QuadPart);
 }
 
+static PyObject*
+winutil_set_file_handle_delete_on_close(PyObject *self, PyObject *args) {
+    HANDLE handle;
+    int delete_on_close;
+    if (!PyArg_ParseTuple(args, "O&p", convert_handle, &handle, &delete_on_close)) return NULL;
+    FILE_DISPOSITION_INFO di;
+    di.DeleteFile = delete_on_close;
+    if (!SetFileInformationByHandle(handle, FileDispositionInfo, &di, sizeof(FILE_DISPOSITION_INFO))) return set_error_from_handle(args);
+    Py_RETURN_NONE;
+}
+
 static PyObject*
 winutil_create_file(PyObject *self, PyObject *args) {
 	wchar_raii path;
@@ -1255,6 +1266,10 @@ static PyMethodDef winutil_methods[] = {
         "set_file_pointer(handle, pos, method=FILE_BEGIN)\n\nWrapper for SetFilePointer"
     },
 
+    {"set_file_handle_delete_on_close", (PyCFunction)winutil_set_file_handle_delete_on_close, METH_VARARGS,
+        "set_file_handle_delete_on_close(handle, delete_on_close)\n\nSet the delete on close flag on the specified handle. Note only works if CreateFile() is called with DELETE generic access, and without FILE_FLAG_DELETE_ON_CLOSE."
+    },
+
     {"read_file", (PyCFunction)winutil_read_file, METH_VARARGS,
         "read_file(handle, chunk_size=16KB)\n\nWrapper for ReadFile"
     },
@@ -1467,6 +1482,7 @@ exec_module(PyObject *m) {
     A(ERROR_ALREADY_EXISTS);
     A(ERROR_BROKEN_PIPE);
     A(ERROR_PIPE_BUSY);
+    A(ERROR_DIR_NOT_EMPTY);
     A(NormalHandle);
     A(ModuleHandle);
     A(IconHandle);