Skip to content

Commit

Permalink
FileStorage: fix data corruption when using restore after multiple undos
Browse files Browse the repository at this point in the history
The case of a history like this:
 - T0 initialize an object in state 0
 - T1 modifies object to state 1
 - T2 modifies object to state 2
 - T3 undo T2 and T1, bringing back to state 0
 - T4 modified object to state 3
 - T5 undo T4, bringing back object to state 0

was not correct after `restore`: the state is 1 instead of the expected 0.

This is because T3 contains two data records:
 - an undo of T2, with a backpointer to the data of state 1
 - an undo of T1, with a backpointer to the data of state 0
When restoring T5 (the undo of T4), the transaction is made of one data
record, with a backpointer that is copied from the backpointer from T3,
but this uses backpointer from the first record for this object, which
is incorrect, in such a case where there is more than one backpointer
for the same oid, we need to iterate in all data record to find the
oldest version.
  • Loading branch information
perrinjerome committed Jan 31, 2024
1 parent 891a8a6 commit 44d36b0
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 13 deletions.
3 changes: 3 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@

- Fix sorting issue in ``scripts/space.py``.

- FileStorage: fix a rare data corruption when using restore after multiple undos.
For details see `#395 <https://github.com/zopefoundation/ZODB/pull/395>`_.


5.8.1 (2023-07-18)
==================
Expand Down
31 changes: 18 additions & 13 deletions src/ZODB/FileStorage/FileStorage.py
Original file line number Diff line number Diff line change
Expand Up @@ -672,28 +672,33 @@ 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()
backpointer = None
while pos < tend:
h = self._read_data_header(pos)
if h.oid == oid:
# Make sure this looks like the right data record
if h.plen == 0:
# This is also a backpointer. Gotta trust it.
# This is also a backpointer, remember it and keep
# looking at other records from this transaction,
# if there is multiple undo records, we use the
# oldest.
backpointer = pos
else:
if h.plen != len(data):
# The expected data doesn't match what's in the
# backpointer. Something is wrong.
logger.error("Mismatch between data and"
" backpointer at %d", pos)
return 0
_data = self._file.read(h.plen)
if data != _data:
return 0
return pos
if h.plen != len(data):
# The expected data doesn't match what's in the
# backpointer. Something is wrong.
logger.error("Mismatch between data and"
" backpointer at %d", pos)
return 0
_data = self._file.read(h.plen)
if data != _data:
return 0
return pos
pos += h.recordlen()
self._file.seek(pos)
return 0
return backpointer or 0

def restore(self, oid, serial, data, version, prev_txn, transaction):
# A lot like store() but without all the consistency checks. This
Expand Down
40 changes: 40 additions & 0 deletions src/ZODB/tests/RecoveryStorage.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,43 @@ def testRestoreWithMultipleObjectsInUndoRedo(self):
# part of the test.
self._dst.copyTransactionsFrom(self._storage)
self.compare(self._storage, self._dst)

def testRestoreWithMultipleUndoRedo(self):
db = DB(self._storage)
c = db.open()
r = c.root()

# TO
r["obj"] = MinPO(0)
transaction.commit()
c.close()

# T1: do 1
r = db.open().root()
r["obj"].value = 1
transaction.commit()

# T2: do 2
r = db.open().root()
r["obj"].value = 2
transaction.commit()

# T3: undo T1 and T2
db.undoMultiple([u["id"] for u in db.undoLog(0, 2)])
transaction.commit()
# obj will be a backpointer to T0

# T4: do 3
r = db.open().root()
r["obj"].value = 3
transaction.commit()

# T4: undo
self._undo(self._storage.undoInfo()[0]['id'])
# obj will be a backpointer to T3, which is a backpointer to T0

r = db.open().root()
self.assertEqual(r["obj"].value, 0)

self._dst.copyTransactionsFrom(self._storage)
self.compare(self._storage, self._dst)
4 changes: 4 additions & 0 deletions src/ZODB/tests/testFileStorage.py
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,10 @@ def testRestoreAcrossPack(self):
# Skip this check as it calls restore directly.
pass

def testRestoreWithMultipleUndoRedo(self):
# This case is only supported with restore, not with store "emulation"
pass


class AnalyzeDotPyTest(StorageTestBase.StorageTestBase):

Expand Down

0 comments on commit 44d36b0

Please sign in to comment.