[Zodb-checkins] CVS: Packages/ZODB/tests - RecoveryStorage.py:1.8.6.3

[Tim Peters]

Update of /cvs-repository/Packages/ZODB/tests
In directory cvs.zope.org:/tmp/cvs-serv11279/ZODB/tests

Modified Files:
      Tag: Zope-2_7-branch
	RecoveryStorage.py 
Log Message:
Collector #1379: Undo + FileStorage.restore go BOOM.

FileStorage.__data_find():  When marching across the data records in a
transaction, this had an "off-by-8 error" for data records containing a
backpointer.  The result was reading nonsense bytes as if they were a
data record header, which could lead to an exception (bad argument to
file.seek()), or silent failure to find data that exists.

New test checkRestoreWithMultipleObjectsInUndoRedo provokes two
instances of the bug before the patch.

ZODB 3.3 trunk doesn't have this bug.  ZODB 3.1.5 does have this bug,
but I'm not fixing it at this time.  It's a critical bug for ZRS
(secondaries doing recovery indirectly rely on _data_find); I don't
think base Zope uses FileStorage.restore().


=== Packages/ZODB/tests/RecoveryStorage.py 1.8.6.2 => 1.8.6.3 ===
--- Packages/ZODB/tests/RecoveryStorage.py:1.8.6.2	Mon Sep 15 17:26:57 2003
+++ Packages/ZODB/tests/RecoveryStorage.py	Fri Jun 18 10:37:42 2004
@@ -182,3 +182,104 @@
         data, serial = self._dst.load(root._p_oid, '')
         raises(KeyError, self._dst.load, obj1._p_oid, '')
         raises(KeyError, self._dst.load, obj2._p_oid, '')
+
+    def checkRestoreWithMultipleObjectsInUndoRedo(self):
+        # Undo creates backpointers in (at least) FileStorage.  ZODB 3.2.1
+        # FileStorage._data_find() had an off-by-8 error, neglecting to
+        # account for the size of the backpointer when searching a
+        # transaction with multiple data records.  The results were
+        # unpredictable.  For example, it could raise a Python exception
+        # due to passing a negative offset to file.seek(), or could
+        # claim that a transaction didn't have data for an oid despite
+        # that it actually did.
+        #
+        # The former failure mode was seen in real life, in a ZRS secondary
+        # doing recovery.  On my box today, the second failure mode is
+        # what happens in this test (with an unpatched _data_find, of
+        # course).  Note that the error can only "bite" if more than one
+        # data record is in a transaction, and the oid we're looking for
+        # follows at least one data record with a backpointer.
+        #
+        # Unfortunately, _data_find() is a low-level implementation detail,
+        # and this test does some horrid white-box abuse to test it.
+
+        is_filestorage = str(self._storage.__class__).endswith('.FileStorage')
+
+        db = DB(self._storage)
+        c = db.open()
+        r = c.root()
+
+        # Create some objects.
+        r["obj1"] = MinPO(1)
+        r["obj2"] = MinPO(1)
+        get_transaction().commit()
+
+        # Add x attributes to them.
+        r["obj1"].x = 'x1'
+        r["obj2"].x = 'x2'
+        get_transaction().commit()
+
+        r = db.open().root()
+        self.assertEquals(r["obj1"].x, 'x1')
+        self.assertEquals(r["obj2"].x, 'x2')
+
+        # Dirty tricks.
+        if is_filestorage:
+            obj1_oid = r["obj1"]._p_oid
+            obj2_oid = r["obj2"]._p_oid
+            # This will be the offset of the next transaction, which
+            # will contain two backpointers.
+            pos = self._storage.getSize()
+
+        # Undo the attribute creation.
+        info = self._storage.undoInfo()
+        tid = info[0]['id']
+        t = Transaction()
+        self._storage.tpc_begin(t)
+        oids = self._storage.transactionalUndo(tid, t)
+        self._storage.tpc_vote(t)
+        self._storage.tpc_finish(t)
+
+        r = db.open().root()
+        self.assertRaises(AttributeError, getattr, r["obj1"], 'x')
+        self.assertRaises(AttributeError, getattr, r["obj2"], 'x')
+
+        if is_filestorage:
+            # _data_find should find data records for both objects in that
+            # transaction.  Without the patch, the second assert failed
+            # (it claimed it couldn't find a data record for obj2) on my
+            # box, but other failure modes were possible.
+            self.assert_(self._storage._data_find(pos, obj1_oid, '') > 0)
+            self.assert_(self._storage._data_find(pos, obj2_oid, '') > 0)
+
+            # The offset of the next ("redo") transaction.
+            pos = self._storage.getSize()
+
+        # Undo the undo (restore the attributes).
+        info = self._storage.undoInfo()
+        tid = info[0]['id']
+        t = Transaction()
+        self._storage.tpc_begin(t)
+        oids = self._storage.transactionalUndo(tid, t)
+        self._storage.tpc_vote(t)
+        self._storage.tpc_finish(t)
+
+        r = db.open().root()
+        self.assertEquals(r["obj1"].x, 'x1')
+        self.assertEquals(r["obj2"].x, 'x2')
+
+        if is_filestorage:
+            # Again _data_find should find both objects in this txn, and
+            # again the second assert failed on my box.
+            self.assert_(self._storage._data_find(pos, obj1_oid, '') > 0)
+            self.assert_(self._storage._data_find(pos, obj2_oid, '') > 0)
+
+        # Indirectly provoke .restore().  .restore in turn indirectly
+        # provokes _data_find too, but not usefully for the purposes of
+        # the specific bug this test aims at:  copyTransactionsFrom() uses
+        # storage iterators that chase backpointers themselves, and
+        # return the data they point at instead.  The result is that
+        # _data_find didn't actually see anything dangerous in this
+        # part of the test.
+        self._dst.copyTransactionsFrom(self._storage)
+        self.compare(self._storage, self._dst)