Re: [PATCHv4 06/10] send-pack/receive-pack: Allow server to refuse pushes with too many commits
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:51:18
Johan Herland [off-list ref] writes:
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
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.