[RFC/PATCH 0/3] limit the size of the packs we receive

In https://public-inbox.org/git/20150612182045.GA23698%40peff.net/,
Peff sent a patch that is used by GitHub to abort `git receive-pack`
when the size of the pack we receive is bigger than a configured
limit.

GitLab is interested in using the same approach and in standardizing
the error messages the user could get back.

So I rebased Peff's patch to the current master, refreshed it a bit,
split it, and added the missing --max-input-size=<size> option to
`git unpack-objects` - to make it work for all `transfer.unpacklimit`
values - in a new patch.

There is no documentation yet for the `--max-input-size=<size>`
options added to `git index-pack` and `git unpack-objects`, nor for
the new `receive.maxsize` config option.

I kept Peff as the author of the patches that are made mostly from his
patch, but I added my Signed-off-by to them.

Christian Couder (1):
  unpack-objects: add --max-input-size=<size> option

Jeff King (2):
  index-pack: add --max-input-size=<size> option
  receive-pack: allow a maximum input size to be specified

 builtin/index-pack.c     |  5 +++++
 builtin/receive-pack.c   | 12 ++++++++++++
 builtin/unpack-objects.c |  7 +++++++
 t/t5546-push-limits.sh   | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 71 insertions(+)
 create mode 100755 t/t5546-push-limits.sh

-- 
2.10.0.rc0.4.g229e32c.dirty

From: Jeff King <redacted>

When receiving a pack-file, it can be useful to abort the
`git index-pack`, if the pack-file is too big.

Signed-off-by: Jeff King <redacted>
Signed-off-by: Christian Couder <redacted>
---
 builtin/index-pack.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 1d2ea58..1fd60bd 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c

@@ -87,6 +87,7 @@ static struct progress *progress;
 static unsigned char input_buffer[4096];
 static unsigned int input_offset, input_len;
 static off_t consumed_bytes;
+static off_t max_input_size;
 static unsigned deepest_delta;
 static git_SHA_CTX input_ctx;
 static uint32_t input_crc32;

@@ -297,6 +298,8 @@ static void use(int bytes)
 	if (signed_add_overflows(consumed_bytes, bytes))
 		die(_("pack too large for current definition of off_t"));
 	consumed_bytes += bytes;
+	if (max_input_size && consumed_bytes > max_input_size)
+		die(_("pack exceeds maximum allowed size"));
 }
 
 static const char *open_pack_file(const char *pack_name)

@@ -1714,6 +1717,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 					opts.off32_limit = strtoul(c+1, &c, 0);
 				if (*c || opts.off32_limit & 0x80000000)
 					die(_("bad %s"), arg);
+			} else if (skip_prefix(arg, "--max-input-size=", &arg)) {
+				max_input_size = strtoul(arg, NULL, 10);
 			} else
 				usage(index_pack_usage);
 			continue;

-- 
2.10.0.rc0.4.g229e32c.dirty

When receiving a pack-file, it can be useful to abort the
`git unpack-objects`, if the pack-file is too big.

Signed-off-by: Christian Couder <redacted>
---
 builtin/unpack-objects.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 172470b..59b1f39 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c

@@ -19,6 +19,7 @@ static const char unpack_usage[] = "git unpack-objects [-n] [-q] [-r] [--strict]
 static unsigned char buffer[4096];
 static unsigned int offset, len;
 static off_t consumed_bytes;
+static off_t max_input_size;
 static git_SHA_CTX ctx;
 static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;
 

@@ -87,6 +88,8 @@ static void use(int bytes)
 	if (signed_add_overflows(consumed_bytes, bytes))
 		die("pack too large for current definition of off_t");
 	consumed_bytes += bytes;
+	if (max_input_size && consumed_bytes > max_input_size)
+		die(_("pack exceeds maximum allowed size"));
 }
 
 static void *get_data(unsigned long size)

@@ -550,6 +553,10 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)
 				len = sizeof(*hdr);
 				continue;
 			}
+			if (skip_prefix(arg, "--max-input-size=", &arg)) {
+				max_input_size = strtoul(arg, NULL, 10);
+				continue;
+			}
 			usage(unpack_usage);
 		}
 

-- 
2.10.0.rc0.4.g229e32c.dirty

From: Jeff King <redacted>

Receive-pack feeds its input to either index-pack or
unpack-objects, which will happily accept as many bytes as
a sender is willing to provide. Let's allow an arbitrary
cutoff point where we will stop writing bytes to disk.

What has already been written to disk can be cleaned
outside of receive-pack.

Signed-off-by: Jeff King <redacted>
Signed-off-by: Christian Couder <redacted>
---
 builtin/receive-pack.c | 12 ++++++++++++
 t/t5546-push-limits.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)
 create mode 100755 t/t5546-push-limits.sh

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 92e1213..7627f7f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c

@@ -46,6 +46,7 @@ static int transfer_unpack_limit = -1;
 static int advertise_atomic_push = 1;
 static int advertise_push_options;
 static int unpack_limit = 100;
+static off_t max_input_size;
 static int report_status;
 static int use_sideband;
 static int use_atomic;

@@ -212,6 +213,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "receive.maxsize") == 0) {
+		max_input_size = git_config_ulong(var, value);
+		return 0;
+	}
+
 	return git_default_config(var, value, cb);
 }
 

@@ -1650,6 +1656,9 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 		if (fsck_objects)
 			argv_array_pushf(&child.args, "--strict%s",
 				fsck_msg_types.buf);
+		if (max_input_size)
+			argv_array_pushf(&child.args, "--max-input-size=%lu",
+				max_input_size);
 		child.no_stdout = 1;
 		child.err = err_fd;
 		child.git_cmd = 1;

@@ -1678,6 +1687,9 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 				fsck_msg_types.buf);
 		if (!reject_thin)
 			argv_array_push(&child.args, "--fix-thin");
+		if (max_input_size)
+			argv_array_pushf(&child.args, "--max-input-size=%lu",
+				max_input_size);
 		child.out = -1;
 		child.err = err_fd;
 		child.git_cmd = 1;

diff --git a/t/t5546-push-limits.sh b/t/t5546-push-limits.sh
new file mode 100755
index 0000000..d3a4d1a
--- /dev/null
+++ b/t/t5546-push-limits.sh

@@ -0,0 +1,47 @@
+#!/bin/sh
+
+test_description='check input limits for pushing'
+. ./test-lib.sh
+
+test_expect_success 'create known-size commit' '
+	test-genrandom foo 1024 >file &&
+	git add file &&
+	test_commit one-k
+'
+
+test_expect_success 'create remote repository' '
+	git init --bare dest &&
+	git --git-dir=dest config receive.unpacklimit 1
+'
+
+test_expect_success 'receive.maxsize rejects push' '
+	git --git-dir=dest config receive.maxsize 512 &&
+	test_must_fail git push dest HEAD
+'
+
+test_expect_success 'bumping limit allows push' '
+	git --git-dir=dest config receive.maxsize 4k &&
+	git push dest HEAD
+'
+
+test_expect_success 'create another known-size commit' '
+	test-genrandom bar 2048 >file2 &&
+	git add file2 &&
+	test_commit two-k
+'
+
+test_expect_success 'change unpacklimit' '
+	git --git-dir=dest config receive.unpacklimit 10
+'
+
+test_expect_success 'receive.maxsize rejects push' '
+	git --git-dir=dest config receive.maxsize 512 &&
+	test_must_fail git push dest HEAD
+'
+
+test_expect_success 'bumping limit allows push' '
+	git --git-dir=dest config receive.maxsize 4k &&
+	git push dest HEAD
+'
+
+test_done

-- 
2.10.0.rc0.4.g229e32c.dirty

On Mon, Aug 15, 2016 at 09:57:27PM +0200, Christian Couder wrote:

From: Jeff King <redacted>

When receiving a pack-file, it can be useful to abort the
`git index-pack`, if the pack-file is too big.

Not much rationale here. I guess because it is all in the 3rd patch,
which ties it into receive-pack. I'm not sure it's worth repeating. I
guess it could all be squished back into one patch. I'm OK either way.

Looks good. I see you marked it for translation, which makes sense.

On the original, I waffled on whether to share the size with the user in
the message. I didn't want to encourage people with "oh, if it's under
2G it must be OK, then!". Because really 2G was meant to be a "you
really shouldn't get this high, and we will unceremoniously dump your
push if you do".

max_input_size is an off_t, but your parse only up to ULONG_MAX here.
For my purposes in the original patch, this was OK, as we set it at 2GB,
which works everywhere (and also, GitHub systems all have 64-bit "long"
these days). But somebody on a 32-bit system could not set this to 4GB,
even though I think index-pack could otherwise handle it. We seem to use
strtoumax() elsewhere, so that's probably a good match (technically it
can overflow an off_t, but in practice this value comes from the admin
and they will set something sane).

-Peff

On Mon, Aug 15, 2016 at 09:57:28PM +0200, Christian Couder wrote:

When receiving a pack-file, it can be useful to abort the
`git unpack-objects`, if the pack-file is too big.

Signed-off-by: Christian Couder <redacted>

Same remarks here as on the last patch, including strtoumax. :)

-Peff

On Mon, Aug 15, 2016 at 09:57:29PM +0200, Christian Couder wrote:

From: Jeff King <redacted>

Receive-pack feeds its input to either index-pack or
unpack-objects, which will happily accept as many bytes as
a sender is willing to provide. Let's allow an arbitrary
cutoff point where we will stop writing bytes to disk.

What has already been written to disk can be cleaned
outside of receive-pack.

This second paragraph hints at a related problem.

Git is generally happy to leave tmp_pack_* around to be cleaned up later
next time git-gc runs. Including its default 2-week grace time.

So imagine that tries to "git push" in a loop. And each time they push,
you say "nope, that's too big". And each time you acquire a new 2GB
tmp_pack file. If your goal was to prevent somebody from streaming
straight to your filesystem and filling up your disk, then it wasn't
very successful. :)

The simple fix is to call register_tempfile() in open_pack_file(), and
just have index-pack clean up the file on its way out.

But there are harder cases. For instance, imagine somebody pushes a
500MB file, and you have a pre-receive hook that says "too big; I won't
accept this". And then they push in a loop, as before. You've accepted
the incoming pack into the repository by the time the pre-receive runs.
You can't just delete it, because you don't know if other simultaneous
processes have started to depend on the objects.

To solve that, I have patches that put incoming packfiles into a
"quarantine" area, then run the connectivity check and pre-receive hooks
with the quarantine accessible via GIT_ALTERNATE_OBJECT_DIRECTORIES. And
then we either move the quarantine packs into the real repo, or blow
away the tmpdir, depending on whether the hooks said the objects were
OK.

Those are patches I plan to share upstream but just haven't gotten
around to yet.

Another off_t/ulong mismatch. I think you want git_config_int64() here.

And here, PRIuMAX and uintmax_t. Or perhaps simpler, just store the
value as a string here and pass it on to index-pack (which would then
need to learn to handle suffixes like "2g"). We do a similar trick in
repack; see b861e23 (repack: propagate pack-objects options as strings,
2014-01-22).

We're going to do basically the same battery of tests against an
unpacklimit of "1" (to catch index-pack) and of "10" (to catch
unpack-objects). It might be clearer to just have a for-loop like:

  for unpacklimit in 1 100
  do
	test_expect_success 'create remote repository' '
		rm -rf dest &&
		git init --bare dest &&
		git -C dest config receive.unpacklimit $unpacklimit
	'

	test_expect_success 'receive.maxsize rejects push' '
		git -C dest config receive.maxsize 512 &&
		test_must_fail git push dest HEAD &&
	'

	test_expect_success 'bumping limit allows push' '
		git -C dest config receive.maxsize 4k &&
		git push dest HEAD
	'
  done

and it's probably worth a comment at the top of the loop explaining what
the heck those numbers mean. :)

-Peff