This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4869

revlog: rewrite censoring logic
ClosedPublic

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

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG324b4b10351e: revlog: rewrite censoring logic
rHGdf59040a0fbb: revlog: rewrite censoring logic
Summary

I was able to corrupt a revlog relatively easily with the existing
censoring code. The underlying problem is that the existing code
doesn't fully take delta chains into account. When copying revisions
that occur after the censored revision, the delta base can refer
to a censored revision. Then at read time, things blow up due to the
revision data not being a compressed delta.

This commit rewrites the revlog censoring code to take a higher-level
approach. We now create a new revlog instance pointing at temp files.
We iterate through each revision in the source revlog and insert
those revisions into the new revlog, replacing the censored revision's
data along the way.

The new implementation isn't as efficient as the old one. This is
because it will fully engage delta computation on insertion. But I
don't think it matters.

The new implementation is a bit hacky because it attempts to reload
the revlog instance with a new revlog index/data file. This is fragile.
But this is needed because the index (which could be backed by C) would
have a cached copy of the old, possibly changed data and that could
lead to problems accessing index or revision data later.

One benefit of the new approach is that we integrate with the
transaction. The old revlog is backed up and if the transaction is
rolled back, the original revlog is restored.

As part of this, we had to teach the transaction about the store
vfs. I'm not super keen about this. But this was the easiest way
to hook things up to the transaction. We /could/ just ignore the
transaction like we were doing before. But any file mutation should
be governed by transaction semantics, including undo during rollback.

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 rHGdf59040a0fbb: revlog: rewrite censoring logic (authored by indygreg). · Explain WhyOct 4 2018, 6:52 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 11689

mercurial/filelog.py

def getstrippoint(self, minlink): def getstrippoint(self, minlink):
return self._revlog.getstrippoint(minlink) return self._revlog.getstrippoint(minlink)
def strip(self, minlink, transaction): def strip(self, minlink, transaction):
return self._revlog.strip(minlink, transaction) return self._revlog.strip(minlink, transaction)
def censorrevision(self, tr, node, tombstone=b''): def censorrevision(self, tr, node, tombstone=b''):
return self._revlog.censorrevision(node, tombstone=tombstone) return self._revlog.censorrevision(tr, node, tombstone=tombstone)
def files(self): def files(self):
return self._revlog.files() return self._revlog.files()
def read(self, node): def read(self, node):
return storageutil.filtermetadata(self.revision(node)) return storageutil.filtermetadata(self.revision(node))
def add(self, text, meta, transaction, link, p1=None, p2=None): def add(self, text, meta, transaction, link, p1=None, p2=None):

mercurial/localrepo.py

self.hook('pretxnopen', throw=True, txnname=desc, txnid=txnid) self.hook('pretxnopen', throw=True, txnname=desc, txnid=txnid)
self._writejournal(desc) self._writejournal(desc)
renames = [(vfs, x, undoname(x)) for vfs, x in self._journalfiles()] renames = [(vfs, x, undoname(x)) for vfs, x in self._journalfiles()]
if report: if report:
rp = report rp = report
else: else:
rp = self.ui.warn rp = self.ui.warn
vfsmap = {'plain': self.vfs} # root of .hg/ vfsmap = {'plain': self.vfs, 'store': self.svfs} # root of .hg/
# we must avoid cyclic reference between repo and transaction. # we must avoid cyclic reference between repo and transaction.
reporef = weakref.ref(self) reporef = weakref.ref(self)
# Code to track tag movement # Code to track tag movement
# #
# Since tags are all handled as file content, it is actually quite hard # Since tags are all handled as file content, it is actually quite hard
# to track these movement from a code perspective. So we fallback to a # to track these movement from a code perspective. So we fallback to a
# tracking at the repository level. One could envision to track changes # tracking at the repository level. One could envision to track changes
# to the '.hgtags' file through changegroup apply but that fails to # to the '.hgtags' file through changegroup apply but that fails to

mercurial/revlog.py

ifh.close() ifh.close()
if addrevisioncb: if addrevisioncb:
addrevisioncb(self, rev, node) addrevisioncb(self, rev, node)
finally: finally:
destrevlog._lazydeltabase = oldlazydeltabase destrevlog._lazydeltabase = oldlazydeltabase
destrevlog._deltabothparents = oldamd destrevlog._deltabothparents = oldamd
def censorrevision(self, node, tombstone=b''): def censorrevision(self, tr, censornode, tombstone=b''):
if (self.version & 0xFFFF) == REVLOGV0: if (self.version & 0xFFFF) == REVLOGV0:
raise error.RevlogError(_('cannot censor with version %d revlogs') % raise error.RevlogError(_('cannot censor with version %d revlogs') %
self.version) self.version)
rev = self.rev(node) censorrev = self.rev(censornode)
tombstone = storageutil.packmeta({b'censored': tombstone}, b'') tombstone = storageutil.packmeta({b'censored': tombstone}, b'')
if len(tombstone) > self.rawsize(rev): if len(tombstone) > self.rawsize(censorrev):
raise error.Abort(_('censor tombstone must be no longer than ' raise error.Abort(_('censor tombstone must be no longer than '
'censored data')) 'censored data'))
# Using two files instead of one makes it easy to rewrite entry-by-entry # Rewriting the revlog in place is hard. Our strategy for censoring is
idxread = self.opener(self.indexfile, 'r') # to create a new revlog, copy all revisions to it, then replace the
idxwrite = self.opener(self.indexfile, 'wb', atomictemp=True) # revlogs on transaction close.
if self.version & FLAG_INLINE_DATA:
dataread, datawrite = idxread, idxwrite newindexfile = self.indexfile + b'.tmpcensored'
else: newdatafile = self.datafile + b'.tmpcensored'
dataread = self.opener(self.datafile, 'r')
datawrite = self.opener(self.datafile, 'wb', atomictemp=True) # This is a bit dangerous. We could easily have a mismatch of state.
newrl = revlog(self.opener, newindexfile, newdatafile,
# Copy all revlog data up to the entry to be censored. censorable=True)
offset = self.start(rev) newrl.version = self.version
newrl._generaldelta = self._generaldelta
for chunk in util.filechunkiter(idxread, limit=rev * self._io.size): newrl._io = self._io
idxwrite.write(chunk)
for chunk in util.filechunkiter(dataread, limit=offset): for rev in self.revs():
datawrite.write(chunk) node = self.node(rev)
p1, p2 = self.parents(node)
def rewriteindex(r, newoffs, newdata=None):
"""Rewrite the index entry with a new data offset and new data. if rev == censorrev:
newrl.addrawrevision(tombstone, tr, self.linkrev(censorrev),
The newdata argument, if given, is a tuple of three positive p1, p2, censornode, REVIDX_ISCENSORED)
integers: (new compressed, new uncompressed, added flag bits).
""" if newrl.deltaparent(rev) != nullrev:
offlags, comp, uncomp, base, link, p1, p2, nodeid = self.index[r] raise error.Abort(_('censored revision stored as delta; '
flags = gettype(offlags) 'cannot censor'),
if newdata: hint=_('censoring of revlogs is not '
comp, uncomp, nflags = newdata 'fully implemented; please report '
flags |= nflags 'this bug'))
offlags = offset_type(newoffs, flags) continue
e = (offlags, comp, uncomp, r, link, p1, p2, nodeid)
idxwrite.write(self._io.packentry(e, None, self.version, r)) if self.iscensored(rev):
idxread.seek(self._io.size, 1) if self.deltaparent(rev) != nullrev:
raise error.Abort(_('cannot censor due to censored '
def rewrite(r, offs, data, nflags=REVIDX_DEFAULT_FLAGS): 'revision having delta stored'))
"""Write the given fulltext with the given data offset. rawtext = self._chunk(rev)
else:
Returns: rawtext = self.revision(rev, raw=True)
The integer number of data bytes written, for tracking data
offsets. newrl.addrawrevision(rawtext, tr, self.linkrev(rev), p1, p2, node,
""" self.flags(rev))
flag, compdata = self.compress(data)
newcomp = len(flag) + len(compdata) tr.addbackup(self.indexfile, location='store')
rewriteindex(r, offs, (newcomp, len(data), nflags)) if not self._inline:
datawrite.write(flag) tr.addbackup(self.datafile, location='store')
datawrite.write(compdata)
dataread.seek(self.length(r), 1) self.opener.rename(newrl.indexfile, self.indexfile)
return newcomp if not self._inline:
self.opener.rename(newrl.datafile, self.datafile)
# Rewrite censored entry with (padded) tombstone data.
pad = ' ' * (self.rawsize(rev) - len(tombstone)) self.clearcaches()
offset += rewrite(rev, offset, tombstone + pad, REVIDX_ISCENSORED) self._loadindex(self.version, None)
# Rewrite all following filelog revisions fixing up offsets and deltas.
for srev in pycompat.xrange(rev + 1, len(self)):
if rev in self.parentrevs(srev):
# Immediate children of censored node must be re-added as
# fulltext.
try:
revdata = self.revision(srev)
except error.CensoredNodeError as e:
revdata = e.tombstone
dlen = rewrite(srev, offset, revdata)
else:
# Copy any other revision data verbatim after fixing up the
# offset.
rewriteindex(srev, offset)
dlen = self.length(srev)
for chunk in util.filechunkiter(dataread, limit=dlen):
datawrite.write(chunk)
offset += dlen
idxread.close()
idxwrite.close()
if dataread is not idxread:
dataread.close()
datawrite.close()
def verifyintegrity(self, state): def verifyintegrity(self, state):
"""Verifies the integrity of the revlog. """Verifies the integrity of the revlog.
Yields ``revlogproblem`` instances describing problems that are Yields ``revlogproblem`` instances describing problems that are
found. found.
""" """
dd, di = self.checksize() dd, di = self.checksize()

mercurial/testing/storage.py

with self._maketransactionfn() as tr: with self._maketransactionfn() as tr:
f.censorrevision(tr, node1) f.censorrevision(tr, node1)
self.assertEqual(len(f), 3) self.assertEqual(len(f), 3)
self.assertEqual(list(f.revs()), [0, 1, 2]) self.assertEqual(list(f.revs()), [0, 1, 2])
self.assertEqual(f.read(node0), b'foo\n' * 30) self.assertEqual(f.read(node0), b'foo\n' * 30)
# TODO revlog can't resolve revision after censor. Probably due to a
# cache on the revlog instance.
with self.assertRaises(error.StorageError):
self.assertEqual(f.read(node2), b'foo\n' * 32) self.assertEqual(f.read(node2), b'foo\n' * 32)
# TODO should raise CensoredNodeError, but fallout from above prevents. with self.assertRaises(error.CensoredNodeError):
with self.assertRaises(error.StorageError):
f.read(node1) f.read(node1)
def testgetstrippointnoparents(self): def testgetstrippointnoparents(self):
# N revisions where none have parents. # N revisions where none have parents.
f = self._makefilefn() f = self._makefilefn()
with self._maketransactionfn() as tr: with self._maketransactionfn() as tr:
for rev in range(10): for rev in range(10):

tests/test-storage.py

def makefilefn(self): def makefilefn(self):
"""Factory for filelog instances.""" """Factory for filelog instances."""
fl = filelog.filelog(STATE['vfs'], b'filelog-%d' % STATE['lastindex']) fl = filelog.filelog(STATE['vfs'], b'filelog-%d' % STATE['lastindex'])
STATE['lastindex'] += 1 STATE['lastindex'] += 1
return fl return fl
def maketransaction(self): def maketransaction(self):
vfsmap = {'plain': STATE['vfs']} vfsmap = {'plain': STATE['vfs'], 'store': STATE['vfs']}
return transaction.transaction(STATE['ui'].warn, STATE['vfs'], vfsmap, return transaction.transaction(STATE['ui'].warn, STATE['vfs'], vfsmap,
b'journal', b'undo') b'journal', b'undo')
def addrawrevision(self, fl, tr, node, p1, p2, linkrev, rawtext=None, def addrawrevision(self, fl, tr, node, p1, p2, linkrev, rawtext=None,
delta=None, censored=False, ellipsis=False, extstored=False): delta=None, censored=False, ellipsis=False, extstored=False):
flags = 0 flags = 0