Re: [PATCHv4 06/10] send-pack/receive-pack: Allow server to refuse pushes with too many commits

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2011, 23:39 UTC
Message-ID: <7vpqn9rnpd.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1306111923-16859-7-git-send-email-johan@herland.net>

Johan Herland <johan@herland.net> writes:

Show 5 quoted lines

> However, older clients that do not understand the capability will not check
> their pack against the limit, and will end up pushing the pack to the server.
> Currently there is no extra check on the server to detect a push that exceeds
> receive.commitCountLimit. However, such a check could be done in a pre-receive
> or update hook.

I found the above a reasonable thing to do. In other words, this is an advisory configuration at this point (and from a cursory scanning of the rest of the series, throughout the series), and that is OK.

Show 16 quoted lines

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1a060ec..c18faac 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1592,6 +1592,15 @@ receive.unpackLimit::
>  	especially on slow filesystems.  If not set, the value of
>  	`transfer.unpackLimit` is used instead.
>  
> +receive.commitCountLimit::
> +	If the number of commits received in a push exceeds this limit,
> +	then the entire push will be refused. This is meant to prevent
> +	an unintended large push (typically a result of the user not
> +	being aware of exactly what is being pushed, e.g. pushing a
> +	large rewritten history) from entering the repo. If not set,
> +	there is no upper limit on the number of commits transferred
> +	in a single push.

But then it may probably be a good idea to reword this a bit, to clarify the refusal happens voluntarily by the pusher. E.g.

	Tell "git push" not to push more than this many commits at once
        into this repository. This is meant to prevent ... in a single
        push. Note that older versions of "git push" may ignore this
        advisory, so if you really want to refuse such a push, you would
        need to arrange to do so in either the pre-receive hook or the
        update hook.

Show 10 quoted lines

> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
> index 11849a3..0240967 100644
> --- a/Documentation/technical/protocol-capabilities.txt
> +++ b/Documentation/technical/protocol-capabilities.txt
> @@ -205,5 +205,7 @@ the server. If the check fails, the client must abort the upload, and
>  report the reason for the aborted push back to the user.
>  The following "limit-*" capabilites are recognized:
>  
> + - limit-commit-count=<num> (Maximum number of commits in a pack)
> +

I think s/in a pack/to transfer/ is more appropriate.

It is a non-essential detail that the current implementation carries only one pack in a single session between send-pack and receive-pack. When we update the protocol (with another capability) so that we can send more than one packs in a single session, we would want the maximum number of commits to be honored.

Come to think of it, I do not necessarily agree with the earlier "max commit count can only be used with max pack size"; I can accept it if the statement is qualified with "for now", though.

It is entirely reasonable to say that I want to split packs in 2GB chunks, and I want to keep the number of commits in the resulting packs (notice the plural) under this fixed ceiling to avoid mistakes, no?

Show 8 quoted lines

> @@ -112,6 +118,9 @@ static const char *capabilities()
>  	int ret = snprintf(buf, sizeof(buf),
>  			   " report-status delete-refs side-band-64k%s",
>  			   prefer_ofs_delta ? " ofs-delta" : "");
> +	if (limit_commit_count > 0)
> +		ret += snprintf(buf + ret, sizeof(buf) - ret,
> +				" limit-commit-count=%lu", limit_commit_count);
>  	assert(ret < sizeof(buf));

Hmm, at this point wouldn't it become attractive to stop using the static fixed sized buffer and instead start using a strbuf or something?

Show 13 quoted lines

> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 5ba5262..f91924f 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -49,9 +49,11 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
>  		NULL,
>  		NULL,
>  		NULL,
> +		NULL,
>  	};
>  	struct child_process po;
>  	int i;
> +	char buf[40];

40 is 19 plus terminating NUL plus 20-decimal digits to hold the count?

Show 28 quoted lines

>  	i = 4;
>  	if (args->use_thin_pack)
> @@ -62,6 +64,11 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
>  		argv[i++] = "-q";
>  	if (args->progress)
>  		argv[i++] = "--progress";
> +	if (args->max_commit_count > 0) {
> +		snprintf(buf, sizeof(buf), "--max-commit-count=%lu",
> +			 args->max_commit_count);
> +		argv[i++] = buf;
> +	}
>  	memset(&po, 0, sizeof(po));
>  	po.argv = argv;
>  	po.in = -1;
> @@ -253,6 +260,7 @@ int send_pack(struct send_pack_args *args,
>  	unsigned cmds_sent = 0;
>  	int ret = 0;
>  	struct async demux;
> +	const char *p;
>  
>  	/* Does the other end support the reporting? */
>  	if (server_supports("report-status"))
> @@ -263,6 +271,8 @@ int send_pack(struct send_pack_args *args,
>  		args->use_ofs_delta = 1;
>  	if (server_supports("side-band-64k"))
>  		use_sideband = 1;
> +	if ((p = server_supports("limit-commit-count=")))
> +		args->max_commit_count = strtoul(p, NULL, 10);

If we find garbage in *p, we would just run with a random limit, which may cause the pack-objects to abort, but that still is a controlled failure and is acceptable.