This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7603

cext-revlog: fixed __delitem__ for uninitialized nodetree
ClosedPublic

Authored by gracinet on Dec 11 2019, 6:52 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG49fa0b31ee1d: cext-revlog: fixed __delitem__ for uninitialized nodetree
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.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gracinet created this revision.Dec 11 2019, 6:52 AM
gracinet added a commit: rHG49fa0b31ee1d: cext-revlog: fixed __delitem__ for uninitialized nodetree.Dec 11 2019, 2:06 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG49fa0b31ee1d: cext-revlog: fixed __delitem__ for uninitialized nodetree (authored by gracinet). · Explain Why
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.Dec 12 2019, 11:49 AM

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

gracinet added a comment.Dec 12 2019, 12:32 PM

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.

gracinet added a comment.Dec 12 2019, 12:38 PM

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.

martinvonz added a comment.Dec 12 2019, 1:14 PM
In 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.

Revision Contents
Changeset List

PathPackages
Mmercurial/cext/revlog.c (3 lines)
Mtests/test-parseindex2.py (26 lines)

Diff 18621

mercurial/cext/revlog.c

return -1; return -1;
nt_delete_node(&self->nt, node); nt_delete_node(&self->nt, node);
} }
if (self->added) if (self->added)
index_invalidate_added(self, 0); index_invalidate_added(self, 0);
if (self->ntrev > start) if (self->ntrev > start)
self->ntrev = (int)start; self->ntrev = (int)start;
} else if (self->added) {
Py_CLEAR(self->added);
} }
self->length = start; self->length = start;
if (start < self->raw_length) { if (start < self->raw_length) {
if (self->cache) { if (self->cache) {
Py_ssize_t i; Py_ssize_t i;
for (i = start; i < self->raw_length; i++) for (i = start; i < self->raw_length; i++)
Py_CLEAR(self->cache[i]); Py_CLEAR(self->cache[i]);
} }
self->raw_length = start; self->raw_length = start;

tests/test-parseindex2.py

index, junk = parsers.parse_index2(data_inlined, True) index, junk = parsers.parse_index2(data_inlined, True)
got = index[-1] got = index[-1]
self.assertEqual(want, got) # inline data self.assertEqual(want, got) # inline data
index, junk = parsers.parse_index2(data_non_inlined, False) index, junk = parsers.parse_index2(data_non_inlined, False)
got = index[-1] got = index[-1]
self.assertEqual(want, got) # no inline data 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__': if __name__ == '__main__':
import silenttestrunner import silenttestrunner
silenttestrunner.main(__name__) silenttestrunner.main(__name__)