This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6939

sidedata: apply basic but tight security around exchange
ClosedPublic

Authored by marmoute on Oct 1 2019, 12:07 PM.

Download Raw Diff

Details

Reviewers

None

Group Reviewers

hg-reviewers

Commits

rHGc17a63eb5d4c: sidedata: apply basic but tight security around exchange

Summary

We don't currently have code to deal with exchange between repository using
sidedata and repository not using sidedata. Until we implement such code (eg:
dropping side data when pushing to a non-sidedata repo) we prevent the two kind
of repo to speak to each other. This is somewhere similar to what 'treemanifest'
does.

Note that sidedata exchange is broken unless one use changegroup v3 anyway. See
next changeset for details.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

marmoute created this revision.Oct 1 2019, 12:07 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptOct 1 2019, 12:07 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

marmoute added a child revision: D6940: sidedata: use only changegroup3 if sidedata is in use.Oct 1 2019, 12:07 PM

marmoute updated this revision to Diff 16771.Oct 2 2019, 12:08 AM

martinvonz added a subscriber: martinvonz.Oct 4 2019, 11:55 PM

martinvonz added inline comments.

mercurial/bundle2.py
1837–1838	Isn't `bool()` redundant here?
1840–1841	Need `b''` prefix, I think (also pick one type of quotes maybe? but maybe Black will soon change this anyway...)

martinvonz added inline comments.Oct 4 2019, 11:59 PM

mercurial/bundle2.py
1840–1841	Never mind... I've just gotten used to looking at extension code where the source transformer is not run. It is run on this code, of course, so this is correct as is.

indygreg added a subscriber: indygreg.Oct 6 2019, 9:49 AM

indygreg added inline comments.

mercurial/bundle2.py
2343	Shouldn't this by `exp-sidedata`?

marmoute updated this revision to Diff 16943.Oct 7 2019, 8:36 PM

marmoute marked an inline comment as done.Oct 7 2019, 8:51 PM

marmoute added inline comments.

mercurial/bundle2.py
1837–1838	It is redundant, but helping Raphaël work on Rust got me scared about boolean. I'll clean it up (either resend or followup)
2343	Hum, yeah. That would be safer. Updated code coming

marmoute marked an inline comment as done.Oct 9 2019, 12:45 PM

marmoute added a commit: rHGc17a63eb5d4c: sidedata: apply basic but tight security around exchange.

This revision was not accepted when it landed; it landed in state Needs Review.

Closed by commit rHGc17a63eb5d4c: sidedata: apply basic but tight security around exchange (authored by marmoute). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/bundle2.py (16 lines)
M			mercurial/exchange.py (5 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	16742		Oct 1 2019, 12:07 PM	★	★
Diff 2	16771		Oct 2 2019, 12:08 AM	★	★
Diff 3	16943		Oct 7 2019, 8:35 PM	★	★
Diff 4	16993	rHGc17a63eb5d4c6d39567ea2aadd7687af9213526f	Oct 6 2019, 11:36 PM	★	★

Commit	Parents	Author	Summary	Date
ca12d5d009a4	065f81f23f4b	Pierre-Yves David		Sep 30 2019, 1:37 PM

Status	Author	Revision
Abandoned	marmoute	D6952 sidedatacopies: preindent some copies related code
Closed	marmoute	D6951 sidedatacopies: get and store sidedata in the changelogrevision object
Closed	marmoute	D6950 sidedatacopies: write copies information in sidedata when applicable
Closed	marmoute	D6949 sidedatacopies: introduce a sidedata testcase for test-copies-in-changeset.t
Closed	marmoute	D6948 sidedatacopies: introduce a sidedata testcase for test-copies-unrelated.t
Closed	marmoute	D6947 sidedatacopies: introduce a sidedata testcase for test-copies.t
Closed	marmoute	D6946 sidedatacopies: teach upgrade about the new requirement
Closed	marmoute	D6945 sidedatacopies: add a new requirement for storing copies into sidedata
Closed	marmoute	D6944 debugsidedata: small doc improvement
Closed	marmoute	D6943 upgrade: allow for `sidedata` removal
Closed	marmoute	D6942 upgrade: allow upgrade to repository using sidedata
Closed	marmoute	D6941 revlog: add a way to control sidedata changes during revlog.clone
Closed	marmoute	D6940 sidedata: use only changegroup3 if sidedata is in use
Closed	marmoute	D6939 sidedata: apply basic but tight security around exchange
Closed	marmoute	D6938 changegroup: use positive logic for treemanifest changegroup3 logic
Closed	marmoute	D6995 debugsidedata: fix verbose mode on python3

Diff 16771

mercurial/bundle2.py

	part = bundler.newpart('changegroup', data=cg.getchunks())			part = bundler.newpart('changegroup', data=cg.getchunks())
	part.addparam('version', cg.version)			part.addparam('version', cg.version)
	if 'clcount' in cg.extras:			if 'clcount' in cg.extras:
	part.addparam('nbchanges', '%d' % cg.extras['clcount'],			part.addparam('nbchanges', '%d' % cg.extras['clcount'],
	mandatory=False)			mandatory=False)
	if opts.get('phases') and repo.revs('%ln and secret()',			if opts.get('phases') and repo.revs('%ln and secret()',
	outgoing.missingheads):			outgoing.missingheads):
	part.addparam('targetphase', '%d' % phases.secret, mandatory=False)			part.addparam('targetphase', '%d' % phases.secret, mandatory=False)
				if 'exp-sidedata-flag' in repo.requirements:
				part.addparam('exp-sidedata', '1')

	if opts.get('streamv2', False):			if opts.get('streamv2', False):
	addpartbundlestream2(bundler, repo, stream=True)			addpartbundlestream2(bundler, repo, stream=True)

	if opts.get('tagsfnodescache', True):			if opts.get('tagsfnodescache', True):
	addparttagsfnodescache(repo, bundler, outgoing)			addparttagsfnodescache(repo, bundler, outgoing)

	if opts.get('revbranchcache', True):			if opts.get('revbranchcache', True):
	elif ret > 1:			elif ret > 1:
	changedheads += ret - 1			changedheads += ret - 1
	if changedheads > 0:			if changedheads > 0:
	result = 1 + changedheads			result = 1 + changedheads
	elif changedheads < 0:			elif changedheads < 0:
	result = -1 + changedheads			result = -1 + changedheads
	return result			return result

	@parthandler('changegroup', ('version', 'nbchanges', 'treemanifest',			@parthandler('changegroup', ('version', 'nbchanges', 'exp-sidedata',
	'targetphase'))			'treemanifest', 'targetphase'))
	def handlechangegroup(op, inpart):			def handlechangegroup(op, inpart):
	"""apply a changegroup part on the repo			"""apply a changegroup part on the repo

	This is a very early implementation that will massive rework before being			This is a very early implementation that will massive rework before being
	inflicted to any end-user.			inflicted to any end-user.
	"""			"""
	from . import localrepo			from . import localrepo

	if len(op.repo.changelog) != 0:			if len(op.repo.changelog) != 0:
	raise error.Abort(_(			raise error.Abort(_(
	"bundle contains tree manifests, but local repo is "			"bundle contains tree manifests, but local repo is "
	"non-empty and does not use tree manifests"))			"non-empty and does not use tree manifests"))
	op.repo.requirements.add('treemanifest')			op.repo.requirements.add('treemanifest')
	op.repo.svfs.options = localrepo.resolvestorevfsoptions(			op.repo.svfs.options = localrepo.resolvestorevfsoptions(
	op.repo.ui, op.repo.requirements, op.repo.features)			op.repo.ui, op.repo.requirements, op.repo.features)
	op.repo._writerequirements()			op.repo._writerequirements()

				bundlesidedata = bool('exp-sidedata' in inpart.params)
				reposidedata = bool('exp-sidedata-flag' in op.repo.requirements)
				martinvonzUnsubmitted Not Done Isn't `bool()` redundant here? martinvonz: Isn't `bool()` redundant here?
				marmouteAuthorUnsubmitted Done It is redundant, but helping Raphaël work on Rust got me scared about boolean. I'll clean it up (either resend or followup) marmoute: It is redundant, but helping Raphaël work on Rust got me scared about boolean. I'll clean it up…
				if reposidedata and not bundlesidedata:
				msg = "repository is using sidedata but the bundle source do not"
				hint = 'this is currently unsupported'
				martinvonzUnsubmitted Not Done Need `b''` prefix, I think (also pick one type of quotes maybe? but maybe Black will soon change this anyway...) martinvonz: Need `b''` prefix, I think (also pick one type of quotes maybe? but maybe Black will soon…
				martinvonzUnsubmitted Not Done Never mind... I've just gotten used to looking at extension code where the source transformer is not run. It is run on this code, of course, so this is correct as is. martinvonz: Never mind... I've just gotten used to looking at extension code where the source transformer…
				raise error.Abort(msg, hint=hint)

	extrakwargs = {}			extrakwargs = {}
	targetphase = inpart.params.get('targetphase')			targetphase = inpart.params.get('targetphase')
	if targetphase is not None:			if targetphase is not None:
	extrakwargs[r'targetphase'] = int(targetphase)			extrakwargs[r'targetphase'] = int(targetphase)
	ret = _processchangegroup(op, cg, tr, 'bundle2', 'bundle2',			ret = _processchangegroup(op, cg, tr, 'bundle2', 'bundle2',
	expectedtotal=nbchangesets, **extrakwargs)			expectedtotal=nbchangesets, **extrakwargs)
	if op.reply is not None:			if op.reply is not None:
	# This is definitely not the final form of this			# This is definitely not the final form of this
	fullnodes=commonnodes)			fullnodes=commonnodes)
	cgdata = packer.generate({nodemod.nullid}, list(commonnodes),			cgdata = packer.generate({nodemod.nullid}, list(commonnodes),
	False, 'narrow_widen', changelog=False)			False, 'narrow_widen', changelog=False)

	part = bundler.newpart('changegroup', data=cgdata)			part = bundler.newpart('changegroup', data=cgdata)
	part.addparam('version', cgversion)			part.addparam('version', cgversion)
	if 'treemanifest' in repo.requirements:			if 'treemanifest' in repo.requirements:
	part.addparam('treemanifest', '1')			part.addparam('treemanifest', '1')
				if 'exp-sidedata-flag' in repo.requirements:
				part.addparam('sidedata', '1')
				indygregUnsubmitted Done Shouldn't this by `exp-sidedata`? indygreg: Shouldn't this by `exp-sidedata`?
				marmouteAuthorUnsubmitted Done Hum, yeah. That would be safer. Updated code coming marmoute: Hum, yeah. That would be safer. Updated code coming

	return bundler			return bundler

mercurial/exchange.py

	version = max(cgversions)			version = max(cgversions)
	cgstream = changegroup.makestream(pushop.repo, pushop.outgoing, version,			cgstream = changegroup.makestream(pushop.repo, pushop.outgoing, version,
	'push')			'push')
	cgpart = bundler.newpart('changegroup', data=cgstream)			cgpart = bundler.newpart('changegroup', data=cgstream)
	if cgversions:			if cgversions:
	cgpart.addparam('version', version)			cgpart.addparam('version', version)
	if 'treemanifest' in pushop.repo.requirements:			if 'treemanifest' in pushop.repo.requirements:
	cgpart.addparam('treemanifest', '1')			cgpart.addparam('treemanifest', '1')
				if 'exp-sidedata-flag' in pushop.repo.requirements:
				cgpart.addparam('exp-sidedata', '1')
	def handlereply(op):			def handlereply(op):
	"""extract addchangegroup returns from server reply"""			"""extract addchangegroup returns from server reply"""
	cgreplies = op.records.getreplies(cgpart.id)			cgreplies = op.records.getreplies(cgpart.id)
	assert len(cgreplies['changegroup']) == 1			assert len(cgreplies['changegroup']) == 1
	pushop.cgresult = cgreplies['changegroup'][0]['return']			pushop.cgresult = cgreplies['changegroup'][0]['return']
	return handlereply			return handlereply

	@b2partsgenerator('phase')			@b2partsgenerator('phase')
	part.addparam('version', version)			part.addparam('version', version)

	part.addparam('nbchanges', '%d' % len(outgoing.missing),			part.addparam('nbchanges', '%d' % len(outgoing.missing),
	mandatory=False)			mandatory=False)

	if 'treemanifest' in repo.requirements:			if 'treemanifest' in repo.requirements:
	part.addparam('treemanifest', '1')			part.addparam('treemanifest', '1')

				if 'exp-sidedata-flag' in repo.requirements:
				part.addparam('exp-sidedata', '1')

	if (kwargs.get(r'narrow', False) and kwargs.get(r'narrow_acl', False)			if (kwargs.get(r'narrow', False) and kwargs.get(r'narrow_acl', False)
	and (include or exclude)):			and (include or exclude)):
	# this is mandatory because otherwise ACL clients won't work			# this is mandatory because otherwise ACL clients won't work
	narrowspecpart = bundler.newpart('Narrow:responsespec')			narrowspecpart = bundler.newpart('Narrow:responsespec')
	narrowspecpart.data = '%s\0%s' % ('\n'.join(include),			narrowspecpart.data = '%s\0%s' % ('\n'.join(include),
	'\n'.join(exclude))			'\n'.join(exclude))

	@getbundle2partsgenerator('bookmarks')			@getbundle2partsgenerator('bookmarks')