This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4867

revlog: clear revision cache on hash verification failure
ClosedPublic

Authored by indygreg on Oct 3 2018, 2:13 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG801ccd8e67c0: revlog: clear revision cache on hash verification failure
rHG731ad7503496: revlog: clear revision cache on hash verification failure
Summary

The revision cache is populated after raw revision fulltext is
retrieved but before hash verification. If hash verification
fails, the revision cache will be populated and subsequent
operations to retrieve the invalid fulltext may return the cached
fulltext instead of raising.

This commit changes hash verification so it will invalidate the
revision cache if the cached node fails hash verification. The
side-effect is that subsequent operations to request the revision
text - even the raw revision text - will always fail.

The new behavior is consistent and is definitely less wrong. There
is an open question of whether revision(raw=True) should validate
hashes. But I'm going to punt on this problem. We can always change
behavior later. And to be honest, I'm not sure we should expose
raw=True on the storage interface at all. Another day...

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

indygreg created this revision.Oct 3 2018, 2:13 PM
Closed by commit rHG731ad7503496: revlog: clear revision cache on hash verification failure (authored by indygreg). · Explain WhyOct 4 2018, 6:52 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/revlog.py (9 lines)
Mmercurial/testing/storage.py (18 lines)

Diff 11687

mercurial/revlog.py

Available as a function so that subclasses can extend hash mismatch Available as a function so that subclasses can extend hash mismatch
behaviors as needed. behaviors as needed.
""" """
try: try:
if p1 is None and p2 is None: if p1 is None and p2 is None:
p1, p2 = self.parents(node) p1, p2 = self.parents(node)
if node != self.hash(text, p1, p2): if node != self.hash(text, p1, p2):
# Clear the revision cache on hash failure. The revision cache
# only stores the raw revision and clearing the cache does have
# the side-effect that we won't have a cache hit when the raw
# revision data is accessed. But this case should be rare and
# it is extra work to teach the cache about the hash
# verification state.
if self._revisioncache and self._revisioncache[0] == node:
self._revisioncache = None
revornode = rev revornode = rev
if revornode is None: if revornode is None:
revornode = templatefilters.short(hex(node)) revornode = templatefilters.short(hex(node))
raise error.RevlogError(_("integrity check failed on %s:%s") raise error.RevlogError(_("integrity check failed on %s:%s")
% (self.indexfile, pycompat.bytestr(revornode))) % (self.indexfile, pycompat.bytestr(revornode)))
except error.RevlogError: except error.RevlogError:
if self._censorable and storageutil.iscensoredtext(text): if self._censorable and storageutil.iscensoredtext(text):
raise error.CensoredNodeError(self.indexfile, node, text) raise error.CensoredNodeError(self.indexfile, node, text)

mercurial/testing/storage.py

self.assertEqual(len(f), 2) self.assertEqual(len(f), 2)
self.assertEqual(f.parents(node1), (node0, nullid)) self.assertEqual(f.parents(node1), (node0, nullid))
# revision() raises since it performs hash verification. # revision() raises since it performs hash verification.
with self.assertRaises(error.StorageError): with self.assertRaises(error.StorageError):
f.revision(node1) f.revision(node1)
# revision(raw=True) still verifies hashes. # raw=True still verifies because there are no special storage
# TODO this is buggy because of cache interaction. # settings.
self.assertEqual(f.revision(node1, raw=True), fulltext1) with self.assertRaises(error.StorageError):
f.revision(node1, raw=True)
# read() behaves like revision(). # read() behaves like revision().
# TODO this is buggy because of cache interaction. with self.assertRaises(error.StorageError):
f.read(node1) f.read(node1)
# We can't test renamed() here because some backends may not require # We can't test renamed() here because some backends may not require
# reading/validating the fulltext to return rename metadata. # reading/validating the fulltext to return rename metadata.
def testbadnoderevisionraw(self): def testbadnoderevisionraw(self):
# Like above except we test revision(raw=True) first to isolate # Like above except we test revision(raw=True) first to isolate
# revision caching behavior. # revision caching behavior.
f = self._makefilefn() f = self._makefilefn()
node1 = b'\xaa' * 20 node1 = b'\xaa' * 20
self._addrawrevisionfn(f, tr, node1, node0, nullid, 1, self._addrawrevisionfn(f, tr, node1, node0, nullid, 1,
rawtext=fulltext1) rawtext=fulltext1)
with self.assertRaises(error.StorageError): with self.assertRaises(error.StorageError):
f.read(node1) f.read(node1)
# TODO this should raise error.StorageError. with self.assertRaises(error.StorageError):
f.read(node1) f.read(node1)
def testbadnodedelta(self): def testbadnodedelta(self):
f = self._makefilefn() f = self._makefilefn()
fulltext0 = b'foo\n' * 31 fulltext0 = b'foo\n' * 31
fulltext1 = fulltext0 + b'bar\n' fulltext1 = fulltext0 + b'bar\n'
fulltext2 = fulltext1 + b'baz\n' fulltext2 = fulltext1 + b'baz\n'
self._addrawrevisionfn(f, tr, node1, node0, nullid, 1, stored1, self._addrawrevisionfn(f, tr, node1, node0, nullid, 1, stored1,
censored=True) censored=True)
self.assertTrue(f.iscensored(1)) self.assertTrue(f.iscensored(1))
with self.assertRaises(error.CensoredNodeError): with self.assertRaises(error.CensoredNodeError):
f.revision(1) f.revision(1)
self.assertEqual(f.revision(1, raw=True), stored1) with self.assertRaises(error.CensoredNodeError):
f.revision(1, raw=True)
with self.assertRaises(error.CensoredNodeError): with self.assertRaises(error.CensoredNodeError):
f.read(1) f.read(1)
def testcensoredrawrevision(self): def testcensoredrawrevision(self):
# Like above, except we do the revision(raw=True) request first to # Like above, except we do the revision(raw=True) request first to
# isolate revision caching behavior. # isolate revision caching behavior.