* [PATCH v2 0/3] limit the size of the packs we receive
@ 2016-08-18 13:15 Christian Couder
2016-08-18 13:15 ` [PATCH v2 1/3] index-pack: add --max-input-size=<size> option Christian Couder
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Christian Couder @ 2016-08-18 13:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 1/3] index-pack: add --max-input-size=<size> option
2016-08-18 13:15 [PATCH v2 0/3] limit the size of the packs we receive Christian Couder
@ 2016-08-18 13:15 ` Christian Couder
2016-08-18 13:15 ` [PATCH v2 2/3] unpack-objects: " Christian Couder
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Christian Couder @ 2016-08-18 13:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] unpack-objects: add --max-input-size=<size> option
2016-08-18 13:15 [PATCH v2 0/3] limit the size of the packs we receive Christian Couder
2016-08-18 13:15 ` [PATCH v2 1/3] index-pack: add --max-input-size=<size> option Christian Couder
@ 2016-08-18 13:15 ` Christian Couder
2016-08-18 13:15 ` [PATCH v2 3/3] receive-pack: allow a maximum input size to be specified Christian Couder
2016-08-18 13:47 ` [PATCH v2 0/3] limit the size of the packs we receive Jeff King
3 siblings, 0 replies; 7+ messages in thread
From: Christian Couder @ 2016-08-18 13:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] receive-pack: allow a maximum input size to be specified
2016-08-18 13:15 [PATCH v2 0/3] limit the size of the packs we receive Christian Couder
2016-08-18 13:15 ` [PATCH v2 1/3] index-pack: add --max-input-size=<size> option Christian Couder
2016-08-18 13:15 ` [PATCH v2 2/3] unpack-objects: " Christian Couder
@ 2016-08-18 13:15 ` Christian Couder
2016-08-18 20:25 ` Junio C Hamano
2016-08-18 13:47 ` [PATCH v2 0/3] limit the size of the packs we receive Jeff King
3 siblings, 1 reply; 7+ messages in thread
From: Christian Couder @ 2016-08-18 13:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/3] limit the size of the packs we receive
2016-08-18 13:15 [PATCH v2 0/3] limit the size of the packs we receive Christian Couder
` (2 preceding siblings ...)
2016-08-18 13:15 ` [PATCH v2 3/3] receive-pack: allow a maximum input size to be specified Christian Couder
@ 2016-08-18 13:47 ` Jeff King
3 siblings, 0 replies; 7+ messages in thread
From: Jeff King @ 2016-08-18 13:47 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Junio C Hamano, Christian Couder
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/3] receive-pack: allow a maximum input size to be specified
2016-08-18 13:15 ` [PATCH v2 3/3] receive-pack: allow a maximum input size to be specified Christian Couder
@ 2016-08-18 20:25 ` Junio C Hamano
2016-08-18 21:43 ` Jeff King
0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2016-08-18 20:25 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Jeff King, Christian Couder
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/3] receive-pack: allow a maximum input size to be specified
2016-08-18 20:25 ` Junio C Hamano
@ 2016-08-18 21:43 ` Jeff King
0 siblings, 0 replies; 7+ messages in thread
From: Jeff King @ 2016-08-18 21:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Christian Couder, git, Christian Couder
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-08-19 1:03 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-18 13:15 [PATCH v2 0/3] limit the size of the packs we receive Christian Couder
2016-08-18 13:15 ` [PATCH v2 1/3] index-pack: add --max-input-size=<size> option Christian Couder
2016-08-18 13:15 ` [PATCH v2 2/3] unpack-objects: " Christian Couder
2016-08-18 13:15 ` [PATCH v2 3/3] receive-pack: allow a maximum input size to be specified Christian Couder
2016-08-18 20:25 ` Junio C Hamano
2016-08-18 21:43 ` Jeff King
2016-08-18 13:47 ` [PATCH v2 0/3] limit the size of the packs we receive Jeff King
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).