Patchwork D7603: cext-revlog: fixed __delitem__ for uninitialized nodetree

login
register
mail settings
Submitter phabricator
Date Dec. 11, 2019, 11:52 a.m.
Message ID <differential-rev-PHID-DREV-yx5epo5soqzlsbdeylsy-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43701/
State Superseded
Headers show

Comments

phabricator - Dec. 11, 2019, 11:52 a.m.
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
phabricator - Dec. 12, 2019, 4:49 p.m.
martinvonz added a comment.


  This broke `./run-tests.py --pure test-parseindex2.py`

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7603/new/

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

To: gracinet, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Dec. 12, 2019, 5:32 p.m.
gracinet added a comment.


  oops, sorry, had no idea the exact same test was running for the pure imp (although it makes sense). I'll check into it, it's not obvious at first sight why it should not work.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7603/new/

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

To: gracinet, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Dec. 12, 2019, 5:38 p.m.
gracinet added a comment.


  Here's a quickfix that works for me for both pure and C
  
    -- a/tests/test-parseindex2.py Thu Dec 05 20:41:23 2019 +0100
    +++ b/tests/test-parseindex2.py Thu Dec 12 18:34:28 2019 +0100
    @@ -18,6 +18,7 @@
         node as nodemod,
         policy,
         pycompat,
    +    util,
     )
     
     parsers = policy.importmod('parsers')
    @@ -267,11 +268,12 @@
             appendrev(6)
             self.assertEqual(len(index), 7)
     
    -        del index[1:7]
    +        del index[1:-1]
     
             # assertions that failed before correction
             self.assertEqual(len(index), 1)  # was 4
    -        self.assertEqual(index.headrevs(), [0])  # gave ValueError
    +        if util.safehasattr(index, 'headrevs'):  # not implemented in pure
    +            self.assertEqual(index.headrevs(), [0])  # gave ValueError
  
  Explanation: the C version would allow either -1 or actual length, the pure version just allows -1. The pure version does not have algorithms such as `headrevs`, they are implemented in revlog.py
  
  I can redo that cleanly, just tell me the expected way.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7603/new/

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

To: gracinet, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Dec. 12, 2019, 6:14 p.m.
martinvonz added a comment.


  In D7603#112088 <https://phab.mercurial-scm.org/D7603#112088>, @gracinet wrote:
  
  > Here's a quickfix that works for me for both pure and C
  >
  >   -- a/tests/test-parseindex2.py Thu Dec 05 20:41:23 2019 +0100
  >   +++ b/tests/test-parseindex2.py Thu Dec 12 18:34:28 2019 +0100
  >   @@ -18,6 +18,7 @@
  >        node as nodemod,
  >        policy,
  >        pycompat,
  >   +    util,
  >    )
  >    parsers = policy.importmod('parsers')
  >   @@ -267,11 +268,12 @@
  >            appendrev(6)
  >            self.assertEqual(len(index), 7)
  >   -        del index[1:7]
  >   +        del index[1:-1]
  >            # assertions that failed before correction
  >            self.assertEqual(len(index), 1)  # was 4
  >   -        self.assertEqual(index.headrevs(), [0])  # gave ValueError
  >   +        if util.safehasattr(index, 'headrevs'):  # not implemented in pure
  >   +            self.assertEqual(index.headrevs(), [0])  # gave ValueError
  >
  > Explanation: the C version would allow either -1 or actual length, the pure version just allows -1. The pure version does not have algorithms such as `headrevs`, they are implemented in revlog.py
  > I can redo that cleanly, just tell me the expected way.
  
  Seems fine to just send that as a regular patch. It's too late to amend this one now. Thanks.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7603/new/

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

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

Patch

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