From b19816025712671186b00ab95c29b9618e9a40b9 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Sun, 6 Sep 2020 11:57:21 +0200 Subject: [PATCH 1/2] check --repair: fix potential data loss, fixes #5325 We already have used SaveFile context manager since long at other places. By using it, the original segment file stays in place until recovery of it is completed (writing/syncing into *.tmp). On successful completion, .tmp is renamed over original + dir syncing. If aborted by some exception, including Ctrl-C, the original file is unmodified. --- src/borg/repository.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/borg/repository.py b/src/borg/repository.py index b3489c1ae..196916251 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -1486,23 +1486,21 @@ class LoggedIO: logger.info('attempting to recover ' + filename) if segment in self.fds: del self.fds[segment] - backup_filename = filename + '.beforerecover' - os.rename(filename, backup_filename) - if os.path.getsize(backup_filename) < MAGIC_LEN + self.header_fmt.size: + if os.path.getsize(filename) < MAGIC_LEN + self.header_fmt.size: # this is either a zero-byte file (which would crash mmap() below) or otherwise # just too small to be a valid non-empty segment file, so do a shortcut here: - with open(filename, 'wb') as fd: + with SaveFile(filename, binary=True) as fd: fd.write(MAGIC) return - with open(backup_filename, 'rb') as backup_fd: + with open(filename, 'rb') as src_fd: # note: file must not be 0 size or mmap() will crash. - with mmap.mmap(backup_fd.fileno(), 0, access=mmap.ACCESS_READ) as mm: + with mmap.mmap(src_fd.fileno(), 0, access=mmap.ACCESS_READ) as mm: # memoryview context manager is problematic, see https://bugs.python.org/issue35686 data = memoryview(mm) d = data try: - with open(filename, 'wb') as fd: - fd.write(MAGIC) + with SaveFile(filename, binary=True) as dst_fd: + dst_fd.write(MAGIC) while len(d) >= self.header_fmt.size: crc, size, tag = self.header_fmt.unpack(d[:self.header_fmt.size]) if size < self.header_fmt.size or size > len(d): @@ -1511,7 +1509,7 @@ class LoggedIO: if crc32(d[4:size]) & 0xffffffff != crc: d = d[1:] continue - fd.write(d[:size]) + dst_fd.write(d[:size]) d = d[size:] finally: del d From bf8706b741f005a1b807fb1f3cb6bafaf24c59b5 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Sun, 6 Sep 2020 12:33:14 +0200 Subject: [PATCH 2/2] fixup: invert nesting of context managers cleaner teardown of contexts: close mmap, close src_fd (reading), close dst_fd (and rename) maybe it was not a real problem to rename a still open-for-reading / mmapped file, but in any case it is cleaner like now. --- src/borg/repository.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/borg/repository.py b/src/borg/repository.py index 196916251..ddfc2ea48 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -1492,14 +1492,14 @@ class LoggedIO: with SaveFile(filename, binary=True) as fd: fd.write(MAGIC) return - with open(filename, 'rb') as src_fd: - # note: file must not be 0 size or mmap() will crash. - with mmap.mmap(src_fd.fileno(), 0, access=mmap.ACCESS_READ) as mm: - # memoryview context manager is problematic, see https://bugs.python.org/issue35686 - data = memoryview(mm) - d = data - try: - with SaveFile(filename, binary=True) as dst_fd: + with SaveFile(filename, binary=True) as dst_fd: + with open(filename, 'rb') as src_fd: + # note: file must not be 0 size or mmap() will crash. + with mmap.mmap(src_fd.fileno(), 0, access=mmap.ACCESS_READ) as mm: + # memoryview context manager is problematic, see https://bugs.python.org/issue35686 + data = memoryview(mm) + d = data + try: dst_fd.write(MAGIC) while len(d) >= self.header_fmt.size: crc, size, tag = self.header_fmt.unpack(d[:self.header_fmt.size]) @@ -1511,9 +1511,9 @@ class LoggedIO: continue dst_fd.write(d[:size]) d = d[size:] - finally: - del d - data.release() + finally: + del d + data.release() def read(self, segment, offset, id, read_data=True): """