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

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

[Christian Couder]

Goal
~~~~

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.

Comments
~~~~~~~~

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.

Changes from previous v1 version
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  - removed last sentences of the commit message in patch 3/3, as
    suggested by Peff,

  - improved the tests in the last patch, as suggested by Peff

Links
~~~~~

This patch series is available here:

https://github.com/chriscool/git/commits/max-receive

The previous versions are here on GitHub:

RFC: https://github.com/chriscool/git/commits/max-receive2
v1: https://github.com/chriscool/git/commits/max-receive6

and here on the list:

RFC: https://public-inbox.org/git/20160815195729.16826-1-chriscool@tuxfamily.org/
v1: https://public-inbox.org/git/20160816081701.29949-1-chriscool@tuxfamily.org/

Peff's initial patch is:

https://public-inbox.org/git/20150612182045.GA23698%40peff.net/

Diff with previous v1 version
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

diff --git a/t/t5546-push-limits.sh b/t/t5546-push-limits.sh
index b38d508..09e958f 100755
--- a/t/t5546-push-limits.sh
+++ b/t/t5546-push-limits.sh
@@ -11,11 +11,11 @@ test_expect_success 'create remote repository' '
 # When the limit is 1, `git receive-pack` will call `git index-pack`.
 # When the limit is 10, `git receive-pack` will call `git unpack-objects`.
 
-while read unpacklimit filesize filename
+while read unpacklimit filesize filename seed
 do
 
 	test_expect_success "create known-size ($filesize bytes) commit '$filename'" '
-		test-genrandom foo "$filesize" >"$filename" &&
+		test-genrandom "$seed" "$filesize" >"$filename" &&
 		git add "$filename" &&
 		test_commit "$filename"
 	'
@@ -35,8 +35,8 @@ do
 	'
 
 done <<\EOF
-1 1024 one-k-file
-10 2048 two-k-file
+1 1024 one-k-file foo
+10 1024 other-one-k-file bar
 EOF
 
 test_done
---

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

 Documentation/config.txt             |  5 +++++
 Documentation/git-index-pack.txt     |  2 ++
 Documentation/git-receive-pack.txt   |  3 +++
 Documentation/git-unpack-objects.txt |  3 +++
 builtin/index-pack.c                 |  5 +++++
 builtin/receive-pack.c               | 12 +++++++++++
 builtin/unpack-objects.c             |  7 ++++++
 t/t5546-push-limits.sh               | 42 ++++++++++++++++++++++++++++++++++++
 8 files changed, 79 insertions(+)
 create mode 100755 t/t5546-push-limits.sh

-- 
2.10.0.rc0.3.geb1f4c9.dirty

[PATCH v2 1/3] index-pack: add --max-input-size=<size> option

[Christian Couder]

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-index-pack.txt | 2 ++
 builtin/index-pack.c             | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 7a4e055..1b4b65d 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt
@@ -87,6 +87,8 @@ OPTIONS
 	Specifying 0 will cause Git to auto-detect the number of CPU's
 	and use maximum 3 threads.
 
+--max-input-size=<size>::
+	Die, if the pack is larger than <size>.
 
 Note
 ----
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 1d2ea58..4a8b4ae 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 = strtoumax(arg, NULL, 10);
 			} else
 				usage(index_pack_usage);
 			continue;
-- 
2.10.0.rc0.3.geb1f4c9.dirty

[PATCH v2 2/3] unpack-objects: add --max-input-size=<size> option

[Christian Couder]

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 <chriscool@tuxfamily.org>
---
 Documentation/git-unpack-objects.txt | 3 +++
 builtin/unpack-objects.c             | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/git-unpack-objects.txt b/Documentation/git-unpack-objects.txt
index 3e887d1..b3de50d 100644
--- a/Documentation/git-unpack-objects.txt
+++ b/Documentation/git-unpack-objects.txt
@@ -44,6 +44,9 @@ OPTIONS
 --strict::
 	Don't write objects with broken content or links.
 
+--max-input-size=<size>::
+	Die, if the pack is larger than <size>.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 172470b..4532aa0 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 = strtoumax(arg, NULL, 10);
+				continue;
+			}
 			usage(unpack_usage);
 		}
 
-- 
2.10.0.rc0.3.geb1f4c9.dirty

[PATCH v2 3/3] receive-pack: allow a maximum input size to be specified

[Christian Couder]

From: Jeff King <peff@peff.net>

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.

Cleaning up what has already been written to disk is a
related problem that is not addressed by this patch.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config.txt           |  5 +++++
 Documentation/git-receive-pack.txt |  3 +++
 builtin/receive-pack.c             | 12 +++++++++++
 t/t5546-push-limits.sh             | 42 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+)
 create mode 100755 t/t5546-push-limits.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..f5b6061 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2517,6 +2517,11 @@ receive.unpackLimit::
 	especially on slow filesystems.  If not set, the value of
 	`transfer.unpackLimit` is used instead.
 
+receive.maxsize::
+	If the size of a pack file is larger than this limit, then
+	git-receive-pack will error out, instead of accepting the pack
+	file. If not set or set to 0, then the size is unlimited.
+
 receive.denyDeletes::
 	If set to true, git-receive-pack will deny a ref update that deletes
 	the ref. Use this to prevent such a ref deletion via a push.
diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 000ee8d..0ccd5fb 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -33,6 +33,9 @@ post-update hooks found in the Documentation/howto directory.
 option, which tells it if updates to a ref should be denied if they
 are not fast-forwards.
 
+A number of other receive.* config options are available to tweak
+its behavior, see linkgit:git-config[1].
+
 OPTIONS
 -------
 <directory>::
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 92e1213..8c2943d 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_int64(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=%"PRIuMAX,
+				(uintmax_t)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=%"PRIuMAX,
+				(uintmax_t)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..09e958f
--- /dev/null
+++ b/t/t5546-push-limits.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='check input limits for pushing'
+. ./test-lib.sh
+
+test_expect_success 'create remote repository' '
+	git init --bare dest
+'
+
+# Let's run tests with different unpack limits: 1 and 10
+# When the limit is 1, `git receive-pack` will call `git index-pack`.
+# When the limit is 10, `git receive-pack` will call `git unpack-objects`.
+
+while read unpacklimit filesize filename seed
+do
+
+	test_expect_success "create known-size ($filesize bytes) commit '$filename'" '
+		test-genrandom "$seed" "$filesize" >"$filename" &&
+		git add "$filename" &&
+		test_commit "$filename"
+	'
+
+	test_expect_success "set unpacklimit to $unpacklimit" '
+		git --git-dir=dest config receive.unpacklimit "$unpacklimit"
+	'
+
+	test_expect_success 'setting receive.maxsize to 512 rejects push' '
+		git --git-dir=dest config receive.maxsize 512 &&
+		test_must_fail git push dest HEAD
+	'
+
+	test_expect_success 'bumping limit to 4k allows push' '
+		git --git-dir=dest config receive.maxsize 4k &&
+		git push dest HEAD
+	'
+
+done <<\EOF
+1 1024 one-k-file foo
+10 1024 other-one-k-file bar
+EOF
+
+test_done
-- 
2.10.0.rc0.3.geb1f4c9.dirty

Re: [PATCH v2 3/3] receive-pack: allow a maximum input size to be specified

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> From: Jeff King <peff@peff.net>
>
> 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.
>
> Cleaning up what has already been written to disk is a
> related problem that is not addressed by this patch.
>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  Documentation/config.txt           |  5 +++++
>  Documentation/git-receive-pack.txt |  3 +++
>  builtin/receive-pack.c             | 12 +++++++++++
>  t/t5546-push-limits.sh             | 42 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 62 insertions(+)
>  create mode 100755 t/t5546-push-limits.sh
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0bcb679..f5b6061 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2517,6 +2517,11 @@ receive.unpackLimit::
>  	especially on slow filesystems.  If not set, the value of
>  	`transfer.unpackLimit` is used instead.
>  
> +receive.maxsize::

Shouldn't this be maxInputSize, not just maxSize?  You are limiting
the size of the input, not the size of the resulting pack, right?

> +	If the size of a pack file is larger than this limit, then

So, s/a pack file/the incoming pack stream/ or something?

> diff --git a/t/t5546-push-limits.sh b/t/t5546-push-limits.sh
> new file mode 100755
> index 0000000..09e958f

Technically, that's receive-limits, no?  It is conceivable that the
client side can also grow a feature to limit the size of an outgoing
push, but that is not what this new script is about.

> --- /dev/null
> +++ b/t/t5546-push-limits.sh
> @@ -0,0 +1,42 @@
> +#!/bin/sh
> +
> +test_description='check input limits for pushing'
> +. ./test-lib.sh
> +
> +test_expect_success 'create remote repository' '
> +	git init --bare dest
> +'
> +

> +# Let's run tests with different unpack limits: 1 and 10
> +# When the limit is 1, `git receive-pack` will call `git index-pack`.
> +# When the limit is 10, `git receive-pack` will call `git unpack-objects`.

I agree with Peff that these tests should push into an empty,
pristine destination.  Move the "create remote" step inside the
while loop and prefix its "git init" with "rm -rf dest", or
something like that.

It would make it crystal clear that the produced packstream for our
transfer won't ever be affected by what is leftover in the
destination repository.

Also, I think it would make it far more readable if you made the
body of the while loop into a helper function that runs tests,
keeping "1/10 corresponds to index/unpack" magic hidden inside the
helper, i.e. something like

test_pack_input_limit () {
	size=$2
        case "$1" in
        unpack) unpack_limit=10000 ;;
        index) unpack_limit=1 ;;
	esac

	test_expect_success 'prepare destination repository' '
		rm -fr dest &&
		git --bare init dest
	'

	test_expect_success "set unpacklimit to $unpack_limit" '
		git --git-dir=dest config receive.unpacklimit "$unpack_limit"
	'

	test_expect_success 'setting receive.maxsize to 512 rejects push' '
		git --git-dir=dest config receive.maxsize 512 &&
		test_must_fail git push dest HEAD
	'

	test_expect_success 'bumping limit to 4k allows push' '
		git --git-dir=dest config receive.maxsize 4k &&
		git push dest HEAD
	'

	test_expect_success 'prepare destination repository (again)' '
		rm -fr dest &&
		git --bare init dest
	'

	test_expect_success 'lifting the limit allows push' '
		git --git-dir=dest config receive.maxsize 0 &&
		git push dest HEAD
	'
}

and have the body of the test more like this:

	test_expect_success 'setup' '
		test-genrandom foo 1024 >test-file &&
		git add test-file &&
		test_commit test-file
	'

	test_pack_input_limit unpack 1024 
	test_pack_input_limit index 1024 

Perhaps?

Re: [PATCH v2 3/3] receive-pack: allow a maximum input size to be specified

[Jeff King]

On Thu, Aug 18, 2016 at 01:25:43PM -0700, Junio C Hamano wrote:

> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 0bcb679..f5b6061 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -2517,6 +2517,11 @@ receive.unpackLimit::
> >  	especially on slow filesystems.  If not set, the value of
> >  	`transfer.unpackLimit` is used instead.
> >  
> > +receive.maxsize::
> 
> Shouldn't this be maxInputSize, not just maxSize?  You are limiting
> the size of the input, not the size of the resulting pack, right?

Yeah, that is probably a better name. I don't recall why I picked
maxSize long ago.

> [...]

I agree, too, with all of the other comments you made.

-Peff

Re: [PATCH v2 0/3] limit the size of the packs we receive

[Jeff King]

On Thu, Aug 18, 2016 at 03:15:50PM +0200, Christian Couder wrote:

> Diff with previous v1 version
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> diff --git a/t/t5546-push-limits.sh b/t/t5546-push-limits.sh
> index b38d508..09e958f 100755
> --- a/t/t5546-push-limits.sh
> +++ b/t/t5546-push-limits.sh
> @@ -11,11 +11,11 @@ test_expect_success 'create remote repository' '
>  # When the limit is 1, `git receive-pack` will call `git index-pack`.
>  # When the limit is 10, `git receive-pack` will call `git unpack-objects`.
>  
> -while read unpacklimit filesize filename
> +while read unpacklimit filesize filename seed
>  do
>  
>  	test_expect_success "create known-size ($filesize bytes) commit '$filename'" '
> -		test-genrandom foo "$filesize" >"$filename" &&
> +		test-genrandom "$seed" "$filesize" >"$filename" &&
>  		git add "$filename" &&
>  		test_commit "$filename"
>  	'
> @@ -35,8 +35,8 @@ do
>  	'
>  
>  done <<\EOF
> -1 1024 one-k-file
> -10 2048 two-k-file
> +1 1024 one-k-file foo
> +10 1024 other-one-k-file bar
>  EOF
>  
>  test_done

This is still not quite what I would have written, but I don't think
it's worth arguing over.

This version looks OK to me. Thanks.

-Peff