D7603: cext-revlog: fixed __delitem__ for uninitialized nodetree

gracinet (Georges Racinet) phabricator at mercurial-scm.org
Wed Dec 11 06:52:30 EST 2019


gracinet created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This is a bug in a code path that's seldom used, because in practice
  (at least in the whole test suite), calls to `del index[i:j]` currently
  just don't happen before the nodetree has been initialized.
  However, in our current work to replace the nodetree by a Rust implementation,
  this is of course systematic.
  
  In `index_slice_del()`, if the slice start is smaller than `self->length`,
  the whole of `self->added` has to be cleared.
  
  Before this change, the clearing was done only by the call to
  `index_invalidate_added(self, 0)`, that happens only for initialized
  nodetrees. Hence the removal was effective only from `start` to `self->length`.
  
  The consequence is index corruption, with bogus results in subsequent calls,
  and in particular errors such as `ValueError("parent out of range")`, due to
  the fact that parents of entries in `self->added` are now just invalid.
  
  This is detected by the rebase tests, under conditions that the nodetree
  of revlog.c is never initialized. The provided specific test is more direct.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D7603

AFFECTED FILES
  mercurial/cext/revlog.c
  tests/test-parseindex2.py

CHANGE DETAILS

diff --git a/tests/test-parseindex2.py b/tests/test-parseindex2.py
--- a/tests/test-parseindex2.py
+++ b/tests/test-parseindex2.py
@@ -247,6 +247,32 @@
         got = index[-1]
         self.assertEqual(want, got)  # no inline data
 
+    def testdelitemwithoutnodetree(self):
+        index, _junk = parsers.parse_index2(data_non_inlined, False)
+
+        def hexrev(rev):
+            if rev == nullrev:
+                return b'\xff\xff\xff\xff'
+            else:
+                return nodemod.bin('%08x' % rev)
+
+        def appendrev(p1, p2=nullrev):
+            # node won't matter for this test, let's just make sure
+            # they don't collide. Other data don't matter either.
+            node = hexrev(p1) + hexrev(p2) + b'.' * 12
+            index.append((0, 0, 12, 1, 34, p1, p2, node))
+
+        appendrev(4)
+        appendrev(5)
+        appendrev(6)
+        self.assertEqual(len(index), 7)
+
+        del index[1:7]
+
+        # assertions that failed before correction
+        self.assertEqual(len(index), 1)  # was 4
+        self.assertEqual(index.headrevs(), [0])  # gave ValueError
+
 
 if __name__ == '__main__':
     import silenttestrunner
diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
--- a/mercurial/cext/revlog.c
+++ b/mercurial/cext/revlog.c
@@ -2522,7 +2522,10 @@
 				index_invalidate_added(self, 0);
 			if (self->ntrev > start)
 				self->ntrev = (int)start;
+		} else if (self->added) {
+			Py_CLEAR(self->added);
 		}
+
 		self->length = start;
 		if (start < self->raw_length) {
 			if (self->cache) {



To: gracinet, #hg-reviewers
Cc: mercurial-devel


More information about the Mercurial-devel mailing list