From patchwork Wed Dec 11 11:52:30 2019
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: D7603: cext-revlog: fixed __delitem__ for uninitialized nodetree
From: phabricator <phabricator@mercurial-scm.org>
X-Patchwork-Id: 43701
Message-Id: <differential-rev-PHID-DREV-yx5epo5soqzlsbdeylsy-req@mercurial-scm.org>
To: Phabricator <phabricator@mercurial-scm.org>
Cc: mercurial-devel@mercurial-scm.org
Date: Wed, 11 Dec 2019 11:52:30 +0000

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




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

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) {