From 7bef1b6e0ddc7bdfe4c1e680d099e8203f95956f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= Date: Wed, 14 Feb 2024 14:32:43 +0000 Subject: [PATCH 1/2] FileStorage: support restore with a prev_txn not existing in storage According to restore comment: > the prev_txn should be considered just a hint, and is ignored if the > transaction doesn't exist. instead, this was raising an UndoError. --- src/ZODB/FileStorage/FileStorage.py | 4 +++- src/ZODB/tests/testFileStorage.py | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/ZODB/FileStorage/FileStorage.py b/src/ZODB/FileStorage/FileStorage.py index a3afec596..80842c9bd 100644 --- a/src/ZODB/FileStorage/FileStorage.py +++ b/src/ZODB/FileStorage/FileStorage.py @@ -1048,6 +1048,8 @@ def undo(self, transaction_id, transaction): tid = decodebytes(transaction_id + b'\n') assert len(tid) == 8 tpos = self._txn_find(tid, 1) + if not tpos: + raise UndoError("Invalid transaction id") tindex = self._txn_undo_write(tpos) self._tindex.update(tindex) return self._tid, tindex.keys() @@ -1066,7 +1068,7 @@ def _txn_find(self, tid, stop_at_pack): # check the status field of the transaction header if h[16] == b'p': break - raise UndoError("Invalid transaction id") + return None def _txn_undo_write(self, tpos): # a helper function to write the data records for transactional undo diff --git a/src/ZODB/tests/testFileStorage.py b/src/ZODB/tests/testFileStorage.py index 9d1fa1010..bdb5cfdd5 100644 --- a/src/ZODB/tests/testFileStorage.py +++ b/src/ZODB/tests/testFileStorage.py @@ -242,6 +242,23 @@ def testRestoreBumpsOid(self): # Before ZODB 3.2.6, this failed, with ._oid == z64. self.assertEqual(self._storage._oid, giant_oid) + def testRestorePrevTxnNotExists(self): + t = TransactionMetaData() + oid = b'\1' * 8 + self._storage.tpc_begin(t) + self._storage.store(oid, b'\0' * 8, b'data1a', b'', t) + self._storage.tpc_vote(t) + self._storage.tpc_finish(t) + + # restore with a transaction not existing in the storage + tid_not_exist = b'\0\0\0\0\0\1\2\3' + t = TransactionMetaData() + self._storage.tpc_begin(t) + self._storage.restore(oid, b'\0' * 8, b'data1b', b'', tid_not_exist, t) + self._storage.tpc_vote(t) + self._storage.tpc_finish(t) + self.assertEqual(self._storage.load(oid)[0], b'data1b') + def testCorruptionInPack(self): # This sets up a corrupt .fs file, with a redundant transaction # length mismatch. The implementation of pack in many releases of From ee1b0b9283b4f0efb11805d480388f93e56e6013 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= Date: Wed, 14 Feb 2024 14:41:42 +0000 Subject: [PATCH 2/2] FileStorage: fix a potential error when restore is called with wrong txn_prev if restore is called with an existing txn_prev that does not contain the oid, the "8-byte redundant transaction length -8" and the beginning of the next transaction was read as a data record. The condition to stop was incorrect as discussed in [1], in practice this error would only happens when restore is passed a bad txn_prev so this should not be a problem in normal conditions because we are supposed to find the data record for that oid. [1] https://github.com/zopefoundation/ZODB/pull/395#discussion_r1474127300 --- CHANGES.rst | 3 +++ src/ZODB/FileStorage/FileStorage.py | 2 +- src/ZODB/tests/testFileStorage.py | 24 ++++++++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index 3c8689abe..4bf1f939e 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -11,6 +11,9 @@ - Fix sorting issue in ``scripts/space.py``. +- Fix behavior of ``FileStorage.restore`` when provided wrong ``prev_txn``. + For details see `#397 `_. + 5.8.1 (2023-07-18) ================== diff --git a/src/ZODB/FileStorage/FileStorage.py b/src/ZODB/FileStorage/FileStorage.py index 80842c9bd..5c76000a7 100644 --- a/src/ZODB/FileStorage/FileStorage.py +++ b/src/ZODB/FileStorage/FileStorage.py @@ -672,7 +672,7 @@ def _data_find(self, tpos, oid, data): tid, tl, status, ul, dl, el = unpack(TRANS_HDR, h) status = as_text(status) self._file.read(ul + dl + el) - tend = tpos + tl + 8 + tend = tpos + tl pos = self._file.tell() while pos < tend: h = self._read_data_header(pos) diff --git a/src/ZODB/tests/testFileStorage.py b/src/ZODB/tests/testFileStorage.py index bdb5cfdd5..5f545e529 100644 --- a/src/ZODB/tests/testFileStorage.py +++ b/src/ZODB/tests/testFileStorage.py @@ -259,6 +259,30 @@ def testRestorePrevTxnNotExists(self): self._storage.tpc_finish(t) self.assertEqual(self._storage.load(oid)[0], b'data1b') + def testRestorePrevTxnWithoutOid(self): + t = TransactionMetaData() + self._storage.tpc_begin(t) + self._storage.store(b'\1' * 8, b'\0' * 8, b'data1', b'', t) + self._storage.tpc_vote(t) + tid1 = self._storage.tpc_finish(t) + + # this transaction is also here to detect problems if restore reads + # below the end of tid1 + t = TransactionMetaData() + self._storage.tpc_begin(t) + self._storage.store(b'\2' * 8, b'\0' * 8, b'data2a', b'', t) + self._storage.tpc_vote(t) + self._storage.tpc_finish(t) + + # restore with a transaction not containing the oid + t = TransactionMetaData() + self._storage.tpc_begin(t) + self._storage.restore(b'\2' * 8, b'\0' * 8, b'data2b', b'', tid1, t) + self._storage.tpc_vote(t) + self._storage.tpc_finish(t) + self.assertEqual(self._storage.load(b'\1' * 8)[0], b'data1') + self.assertEqual(self._storage.load(b'\2' * 8)[0], b'data2b') + def testCorruptionInPack(self): # This sets up a corrupt .fs file, with a redundant transaction # length mismatch. The implementation of pack in many releases of