{"thread":{"id":"43860","subject":"[PATCH 0/3] limit the size of the packs we receive","startedAt":"2016-08-16T08:17:46Z","lastAt":"2016-08-18T14:08:18Z","messageCount":11,"participants":["Christian Couder","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"299432","messageId":"20160816081701.29949-1-chriscool@tuxfamily.org","threadId":"43860","inReplyTo":null,"subject":"[PATCH 0/3] limit the size of the packs we receive","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-08-16T08:16:58Z","receivedAt":"2016-08-16T08:17:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Goal\n~~~~\n\nIn https://public-inbox.org/git/20150612182045.GA23698%40peff.net/,\nPeff sent a patch that is used by GitHub to abort `git receive-pack`\nwhen the size of the pack we receive is bigger than a configured\nlimit.\n\nGitLab is interested in using the same approach and in standardizing\nthe error messages the user could get back.\n\nComments\n~~~~~~~~\n\nI kept Peff as the author of the patches that are made mostly from his\npatch, but I added my Signed-off-by to them.\n\nChanges from previous RFC version\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\n  - added documentation to all the 3 patches,\n\n  - changed strtoul() to strtoumax() in the first 2 patches, as\n    suggested by Peff,\n\n  - changed git_config_ulong() to git_config_int64() and used PRIuMAX\n    and uintmax_t in the last patch, as suggested by Peff,\n\n  - reorganized the tests in the last patch, as suggested by Peff\n\n  - improved commit message of the last patch with information from\n    Peff\n\nLinks\n~~~~~\n\nThis patch series is available here:\n\nhttps://github.com/chriscool/git/commits/max-receive\n\nThe previous RFC version is here on GitHub:\n\nhttps://github.com/chriscool/git/commits/max-receive2\n\nand here on the list:\n\nhttps://public-inbox.org/git/20160815195729.16826-1-chriscool@tuxfamily.org/\n\nPeff's initial patch is:\n\nhttps://public-inbox.org/git/20150612182045.GA23698%40peff.net/\n\nDiff with previous RFC version\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0bcb679..f5b6061 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2517,6 +2517,11 @@ receive.unpackLimit::\n \tespecially on slow filesystems.  If not set, the value of\n \t`transfer.unpackLimit` is used instead.\n \n+receive.maxsize::\n+\tIf the size of a pack file is larger than this limit, then\n+\tgit-receive-pack will error out, instead of accepting the pack\n+\tfile. If not set or set to 0, then the size is unlimited.\n+\n receive.denyDeletes::\n \tIf set to true, git-receive-pack will deny a ref update that deletes\n \tthe ref. Use this to prevent such a ref deletion via a push.\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 7a4e055..1b4b65d 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -87,6 +87,8 @@ OPTIONS\n \tSpecifying 0 will cause Git to auto-detect the number of CPU's\n \tand use maximum 3 threads.\n \n+--max-input-size=<size>::\n+\tDie, if the pack is larger than <size>.\n \n Note\n ----\ndiff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt\nindex 000ee8d..0ccd5fb 100644\n--- a/Documentation/git-receive-pack.txt\n+++ b/Documentation/git-receive-pack.txt\n@@ -33,6 +33,9 @@ post-update hooks found in the Documentation/howto directory.\n option, which tells it if updates to a ref should be denied if they\n are not fast-forwards.\n \n+A number of other receive.* config options are available to tweak\n+its behavior, see linkgit:git-config[1].\n+\n OPTIONS\n -------\n <directory>::\ndiff --git a/Documentation/git-unpack-objects.txt b/Documentation/git-unpack-objects.txt\nindex 3e887d1..b3de50d 100644\n--- a/Documentation/git-unpack-objects.txt\n+++ b/Documentation/git-unpack-objects.txt\n@@ -44,6 +44,9 @@ OPTIONS\n --strict::\n \tDon't write objects with broken content or links.\n \n+--max-input-size=<size>::\n+\tDie, if the pack is larger than <size>.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1fd60bd..4a8b4ae 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1718,7 +1718,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tif (*c || opts.off32_limit & 0x80000000)\n \t\t\t\t\tdie(_(\"bad %s\"), arg);\n \t\t\t} else if (skip_prefix(arg, \"--max-input-size=\", &arg)) {\n-\t\t\t\tmax_input_size = strtoul(arg, NULL, 10);\n+\t\t\t\tmax_input_size = strtoumax(arg, NULL, 10);\n \t\t\t} else\n \t\t\t\tusage(index_pack_usage);\n \t\t\tcontinue;\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 7627f7f..8c2943d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -214,7 +214,7 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t}\n \n \tif (strcmp(var, \"receive.maxsize\") == 0) {\n-\t\tmax_input_size = git_config_ulong(var, value);\n+\t\tmax_input_size = git_config_int64(var, value);\n \t\treturn 0;\n \t}\n \n@@ -1657,8 +1657,8 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\t\targv_array_pushf(&child.args, \"--strict%s\",\n \t\t\t\tfsck_msg_types.buf);\n \t\tif (max_input_size)\n-\t\t\targv_array_pushf(&child.args, \"--max-input-size=%lu\",\n-\t\t\t\tmax_input_size);\n+\t\t\targv_array_pushf(&child.args, \"--max-input-size=%\"PRIuMAX,\n+\t\t\t\t(uintmax_t)max_input_size);\n \t\tchild.no_stdout = 1;\n \t\tchild.err = err_fd;\n \t\tchild.git_cmd = 1;\n@@ -1688,8 +1688,8 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\tif (!reject_thin)\n \t\t\targv_array_push(&child.args, \"--fix-thin\");\n \t\tif (max_input_size)\n-\t\t\targv_array_pushf(&child.args, \"--max-input-size=%lu\",\n-\t\t\t\tmax_input_size);\n+\t\t\targv_array_pushf(&child.args, \"--max-input-size=%\"PRIuMAX,\n+\t\t\t\t(uintmax_t)max_input_size);\n \t\tchild.out = -1;\n \t\tchild.err = err_fd;\n \t\tchild.git_cmd = 1;\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 59b1f39..4532aa0 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -554,7 +554,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (skip_prefix(arg, \"--max-input-size=\", &arg)) {\n-\t\t\t\tmax_input_size = strtoul(arg, NULL, 10);\n+\t\t\t\tmax_input_size = strtoumax(arg, NULL, 10);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tusage(unpack_usage);\ndiff --git a/t/t5546-push-limits.sh b/t/t5546-push-limits.sh\nindex d3a4d1a..b38d508 100755\n--- a/t/t5546-push-limits.sh\n+++ b/t/t5546-push-limits.sh\n@@ -3,45 +3,40 @@\n test_description='check input limits for pushing'\n . ./test-lib.sh\n \n-test_expect_success 'create known-size commit' '\n-\ttest-genrandom foo 1024 >file &&\n-\tgit add file &&\n-\ttest_commit one-k\n-'\n-\n test_expect_success 'create remote repository' '\n-\tgit init --bare dest &&\n-\tgit --git-dir=dest config receive.unpacklimit 1\n+\tgit init --bare dest\n '\n \n-test_expect_success 'receive.maxsize rejects push' '\n-\tgit --git-dir=dest config receive.maxsize 512 &&\n-\ttest_must_fail git push dest HEAD\n-'\n+# Let's run tests with different unpack limits: 1 and 10\n+# When the limit is 1, `git receive-pack` will call `git index-pack`.\n+# When the limit is 10, `git receive-pack` will call `git unpack-objects`.\n \n-test_expect_success 'bumping limit allows push' '\n-\tgit --git-dir=dest config receive.maxsize 4k &&\n-\tgit push dest HEAD\n-'\n+while read unpacklimit filesize filename\n+do\n \n-test_expect_success 'create another known-size commit' '\n-\ttest-genrandom bar 2048 >file2 &&\n-\tgit add file2 &&\n-\ttest_commit two-k\n-'\n+\ttest_expect_success \"create known-size ($filesize bytes) commit '$filename'\" '\n+\t\ttest-genrandom foo \"$filesize\" >\"$filename\" &&\n+\t\tgit add \"$filename\" &&\n+\t\ttest_commit \"$filename\"\n+\t'\n \n-test_expect_success 'change unpacklimit' '\n-\tgit --git-dir=dest config receive.unpacklimit 10\n-'\n+\ttest_expect_success \"set unpacklimit to $unpacklimit\" '\n+\t\tgit --git-dir=dest config receive.unpacklimit \"$unpacklimit\"\n+\t'\n \n-test_expect_success 'receive.maxsize rejects push' '\n-\tgit --git-dir=dest config receive.maxsize 512 &&\n-\ttest_must_fail git push dest HEAD\n-'\n+\ttest_expect_success 'setting receive.maxsize to 512 rejects push' '\n+\t\tgit --git-dir=dest config receive.maxsize 512 &&\n+\t\ttest_must_fail git push dest HEAD\n+\t'\n \n-test_expect_success 'bumping limit allows push' '\n-\tgit --git-dir=dest config receive.maxsize 4k &&\n-\tgit push dest HEAD\n-'\n+\ttest_expect_success 'bumping limit to 4k allows push' '\n+\t\tgit --git-dir=dest config receive.maxsize 4k &&\n+\t\tgit push dest HEAD\n+\t'\n+\n+done <<\\EOF\n+1 1024 one-k-file\n+10 2048 two-k-file\n+EOF\n \n test_done\n\n\nChristian Couder (1):\n  unpack-objects: add --max-input-size=<size> option\n\nJeff King (2):\n  index-pack: add --max-input-size=<size> option\n  receive-pack: allow a maximum input size to be specified\n\n Documentation/config.txt             |  5 +++++\n Documentation/git-index-pack.txt     |  2 ++\n Documentation/git-receive-pack.txt   |  3 +++\n Documentation/git-unpack-objects.txt |  3 +++\n builtin/index-pack.c                 |  5 +++++\n builtin/receive-pack.c               | 12 +++++++++++\n builtin/unpack-objects.c             |  7 ++++++\n t/t5546-push-limits.sh               | 42 ++++++++++++++++++++++++++++++++++++\n 8 files changed, 79 insertions(+)\n create mode 100755 t/t5546-push-limits.sh\n\n-- \n2.10.0.rc0.3.g8535b4c\n\n"},{"id":"299433","messageId":"20160816081701.29949-2-chriscool@tuxfamily.org","threadId":"43860","inReplyTo":"20160816081701.29949-1-chriscool@tuxfamily.org","subject":"[PATCH 1/3] index-pack: add --max-input-size=<size> option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-08-16T08:16:59Z","receivedAt":"2016-08-16T08:17:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nWhen receiving a pack-file, it can be useful to abort the\n`git index-pack`, if the pack-file is too big.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n Documentation/git-index-pack.txt | 2 ++\n builtin/index-pack.c             | 5 +++++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 7a4e055..1b4b65d 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -87,6 +87,8 @@ OPTIONS\n \tSpecifying 0 will cause Git to auto-detect the number of CPU's\n \tand use maximum 3 threads.\n \n+--max-input-size=<size>::\n+\tDie, if the pack is larger than <size>.\n \n Note\n ----\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1d2ea58..4a8b4ae 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -87,6 +87,7 @@ static struct progress *progress;\n static unsigned char input_buffer[4096];\n static unsigned int input_offset, input_len;\n static off_t consumed_bytes;\n+static off_t max_input_size;\n static unsigned deepest_delta;\n static git_SHA_CTX input_ctx;\n static uint32_t input_crc32;\n@@ -297,6 +298,8 @@ static void use(int bytes)\n \tif (signed_add_overflows(consumed_bytes, bytes))\n \t\tdie(_(\"pack too large for current definition of off_t\"));\n \tconsumed_bytes += bytes;\n+\tif (max_input_size && consumed_bytes > max_input_size)\n+\t\tdie(_(\"pack exceeds maximum allowed size\"));\n }\n \n static const char *open_pack_file(const char *pack_name)\n@@ -1714,6 +1717,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\t\topts.off32_limit = strtoul(c+1, &c, 0);\n \t\t\t\tif (*c || opts.off32_limit & 0x80000000)\n \t\t\t\t\tdie(_(\"bad %s\"), arg);\n+\t\t\t} else if (skip_prefix(arg, \"--max-input-size=\", &arg)) {\n+\t\t\t\tmax_input_size = strtoumax(arg, NULL, 10);\n \t\t\t} else\n \t\t\t\tusage(index_pack_usage);\n \t\t\tcontinue;\n-- \n2.10.0.rc0.3.g8535b4c\n\n"},{"id":"299434","messageId":"20160816081701.29949-3-chriscool@tuxfamily.org","threadId":"43860","inReplyTo":"20160816081701.29949-1-chriscool@tuxfamily.org","subject":"[PATCH 2/3] unpack-objects: add --max-input-size=<size> option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-08-16T08:17:00Z","receivedAt":"2016-08-16T08:17:58Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When receiving a pack-file, it can be useful to abort the\n`git unpack-objects`, if the pack-file is too big.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n Documentation/git-unpack-objects.txt | 3 +++\n builtin/unpack-objects.c             | 7 +++++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/Documentation/git-unpack-objects.txt b/Documentation/git-unpack-objects.txt\nindex 3e887d1..b3de50d 100644\n--- a/Documentation/git-unpack-objects.txt\n+++ b/Documentation/git-unpack-objects.txt\n@@ -44,6 +44,9 @@ OPTIONS\n --strict::\n \tDon't write objects with broken content or links.\n \n+--max-input-size=<size>::\n+\tDie, if the pack is larger than <size>.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 172470b..4532aa0 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -19,6 +19,7 @@ static const char unpack_usage[] = \"git unpack-objects [-n] [-q] [-r] [--strict]\n static unsigned char buffer[4096];\n static unsigned int offset, len;\n static off_t consumed_bytes;\n+static off_t max_input_size;\n static git_SHA_CTX ctx;\n static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;\n \n@@ -87,6 +88,8 @@ static void use(int bytes)\n \tif (signed_add_overflows(consumed_bytes, bytes))\n \t\tdie(\"pack too large for current definition of off_t\");\n \tconsumed_bytes += bytes;\n+\tif (max_input_size && consumed_bytes > max_input_size)\n+\t\tdie(_(\"pack exceeds maximum allowed size\"));\n }\n \n static void *get_data(unsigned long size)\n@@ -550,6 +553,10 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\tlen = sizeof(*hdr);\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (skip_prefix(arg, \"--max-input-size=\", &arg)) {\n+\t\t\t\tmax_input_size = strtoumax(arg, NULL, 10);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tusage(unpack_usage);\n \t\t}\n \n-- \n2.10.0.rc0.3.g8535b4c\n\n"},{"id":"299435","messageId":"20160816081701.29949-4-chriscool@tuxfamily.org","threadId":"43860","inReplyTo":"20160816081701.29949-1-chriscool@tuxfamily.org","subject":"[PATCH 3/3] receive-pack: allow a maximum input size to be specified","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-08-16T08:17:01Z","receivedAt":"2016-08-16T08:18:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nReceive-pack feeds its input to either index-pack or\nunpack-objects, which will happily accept as many bytes as\na sender is willing to provide. Let's allow an arbitrary\ncutoff point where we will stop writing bytes to disk.\n\nCleaning up what has already been written to disk is a\nrelated problem that is not addressed by this patch.\n\nThe problem is that git-gc can clean up tmp_pack_* files\nafter their grace time expired, but that may not be\nenough if someone tries to \"git push\" in a loop.\n\nA simple fix is to call register_tempfile() in\nopen_pack_file(), and just have index-pack clean up the\nfile on its way out.\n\nBut there are harder cases. For instance, if somebody\npushes a 500MB file, and there is a pre-receive hook that\nsays \"too big; I won't accept this\". And then they push in\na loop, the incoming pack has already been accepted into\nthe repository by the time the pre-receive hook runs.\nIt's not possible to just delete it, because we don't know\nif other simultaneous processes have started to depend on\nthe objects.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n Documentation/config.txt           |  5 +++++\n Documentation/git-receive-pack.txt |  3 +++\n builtin/receive-pack.c             | 12 +++++++++++\n t/t5546-push-limits.sh             | 42 ++++++++++++++++++++++++++++++++++++++\n 4 files changed, 62 insertions(+)\n create mode 100755 t/t5546-push-limits.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0bcb679..f5b6061 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2517,6 +2517,11 @@ receive.unpackLimit::\n \tespecially on slow filesystems.  If not set, the value of\n \t`transfer.unpackLimit` is used instead.\n \n+receive.maxsize::\n+\tIf the size of a pack file is larger than this limit, then\n+\tgit-receive-pack will error out, instead of accepting the pack\n+\tfile. If not set or set to 0, then the size is unlimited.\n+\n receive.denyDeletes::\n \tIf set to true, git-receive-pack will deny a ref update that deletes\n \tthe ref. Use this to prevent such a ref deletion via a push.\ndiff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt\nindex 000ee8d..0ccd5fb 100644\n--- a/Documentation/git-receive-pack.txt\n+++ b/Documentation/git-receive-pack.txt\n@@ -33,6 +33,9 @@ post-update hooks found in the Documentation/howto directory.\n option, which tells it if updates to a ref should be denied if they\n are not fast-forwards.\n \n+A number of other receive.* config options are available to tweak\n+its behavior, see linkgit:git-config[1].\n+\n OPTIONS\n -------\n <directory>::\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 92e1213..8c2943d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -46,6 +46,7 @@ static int transfer_unpack_limit = -1;\n static int advertise_atomic_push = 1;\n static int advertise_push_options;\n static int unpack_limit = 100;\n+static off_t max_input_size;\n static int report_status;\n static int use_sideband;\n static int use_atomic;\n@@ -212,6 +213,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.maxsize\") == 0) {\n+\t\tmax_input_size = git_config_int64(var, value);\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -1650,6 +1656,9 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\tif (fsck_objects)\n \t\t\targv_array_pushf(&child.args, \"--strict%s\",\n \t\t\t\tfsck_msg_types.buf);\n+\t\tif (max_input_size)\n+\t\t\targv_array_pushf(&child.args, \"--max-input-size=%\"PRIuMAX,\n+\t\t\t\t(uintmax_t)max_input_size);\n \t\tchild.no_stdout = 1;\n \t\tchild.err = err_fd;\n \t\tchild.git_cmd = 1;\n@@ -1678,6 +1687,9 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\t\t\tfsck_msg_types.buf);\n \t\tif (!reject_thin)\n \t\t\targv_array_push(&child.args, \"--fix-thin\");\n+\t\tif (max_input_size)\n+\t\t\targv_array_pushf(&child.args, \"--max-input-size=%\"PRIuMAX,\n+\t\t\t\t(uintmax_t)max_input_size);\n \t\tchild.out = -1;\n \t\tchild.err = err_fd;\n \t\tchild.git_cmd = 1;\ndiff --git a/t/t5546-push-limits.sh b/t/t5546-push-limits.sh\nnew file mode 100755\nindex 0000000..b38d508\n--- /dev/null\n+++ b/t/t5546-push-limits.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+\n+test_description='check input limits for pushing'\n+. ./test-lib.sh\n+\n+test_expect_success 'create remote repository' '\n+\tgit init --bare dest\n+'\n+\n+# Let's run tests with different unpack limits: 1 and 10\n+# When the limit is 1, `git receive-pack` will call `git index-pack`.\n+# When the limit is 10, `git receive-pack` will call `git unpack-objects`.\n+\n+while read unpacklimit filesize filename\n+do\n+\n+\ttest_expect_success \"create known-size ($filesize bytes) commit '$filename'\" '\n+\t\ttest-genrandom foo \"$filesize\" >\"$filename\" &&\n+\t\tgit add \"$filename\" &&\n+\t\ttest_commit \"$filename\"\n+\t'\n+\n+\ttest_expect_success \"set unpacklimit to $unpacklimit\" '\n+\t\tgit --git-dir=dest config receive.unpacklimit \"$unpacklimit\"\n+\t'\n+\n+\ttest_expect_success 'setting receive.maxsize to 512 rejects push' '\n+\t\tgit --git-dir=dest config receive.maxsize 512 &&\n+\t\ttest_must_fail git push dest HEAD\n+\t'\n+\n+\ttest_expect_success 'bumping limit to 4k allows push' '\n+\t\tgit --git-dir=dest config receive.maxsize 4k &&\n+\t\tgit push dest HEAD\n+\t'\n+\n+done <<\\EOF\n+1 1024 one-k-file\n+10 2048 two-k-file\n+EOF\n+\n+test_done\n-- \n2.10.0.rc0.3.g8535b4c\n\n"},{"id":"299451","messageId":"20160816131145.apq77nc2x7wb7lba@sigill.intra.peff.net","threadId":"43860","inReplyTo":"20160816081701.29949-1-chriscool@tuxfamily.org","subject":"Re: [PATCH 0/3] limit the size of the packs we receive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-16T13:11:45Z","receivedAt":"2016-08-16T13:13:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 16, 2016 at 10:16:58AM +0200, Christian Couder wrote:\n\n> Changes from previous RFC version\n> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> \n>   - added documentation to all the 3 patches,\n\nGood idea.\n\n>   - changed strtoul() to strtoumax() in the first 2 patches, as\n>     suggested by Peff,\n> \n>   - changed git_config_ulong() to git_config_int64() and used PRIuMAX\n>     and uintmax_t in the last patch, as suggested by Peff,\n\nThinking a bit, off_t is actually signed. So maybe PRIdMAX (which we\ndon't seem to have compat macros for) would make more sense. I dunno if\nanybody actually cares. This value shouldn't be signed anyway, and\nnobody should be approaching the limits of a 64-bit number anyway (there\nis no point in limiting the incoming pack to the exabyte range).\n\nSo I'm inclined not to worry about it.\n\n-Peff\n"},{"id":"299453","messageId":"20160816131640.h2zzn3sy5qqdeewc@sigill.intra.peff.net","threadId":"43860","inReplyTo":"20160816081701.29949-4-chriscool@tuxfamily.org","subject":"Re: [PATCH 3/3] receive-pack: allow a maximum input size to be specified","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-16T13:16:41Z","receivedAt":"2016-08-16T13:17:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 16, 2016 at 10:17:01AM +0200, Christian Couder wrote:\n\n> From: Jeff King <peff@peff.net>\n> \n> Receive-pack feeds its input to either index-pack or\n> unpack-objects, which will happily accept as many bytes as\n> a sender is willing to provide. Let's allow an arbitrary\n> cutoff point where we will stop writing bytes to disk.\n> \n> Cleaning up what has already been written to disk is a\n> related problem that is not addressed by this patch.\n> \n> The problem is that git-gc can clean up tmp_pack_* files\n> after their grace time expired, but that may not be\n> enough if someone tries to \"git push\" in a loop.\n> \n> A simple fix is to call register_tempfile() in\n> open_pack_file(), and just have index-pack clean up the\n> file on its way out.\n> \n> But there are harder cases. For instance, if somebody\n> pushes a 500MB file, and there is a pre-receive hook that\n> says \"too big; I won't accept this\". And then they push in\n> a loop, the incoming pack has already been accepted into\n> the repository by the time the pre-receive hook runs.\n> It's not possible to just delete it, because we don't know\n> if other simultaneous processes have started to depend on\n> the objects.\n\nIMHO, everything after \"not addressed by this patch\" doesn't really add\nanything. This commit has value on its own, and the discussion about the\nnext steps is already documented on the list (and hopefully will be\nfixed with other patches!).\n\n> +test_expect_success 'create remote repository' '\n> +\tgit init --bare dest\n> +'\n> +\n> +# Let's run tests with different unpack limits: 1 and 10\n> +# When the limit is 1, `git receive-pack` will call `git index-pack`.\n> +# When the limit is 10, `git receive-pack` will call `git unpack-objects`.\n> +\n> +while read unpacklimit filesize filename\n> +do\n> [...]\n> +done <<\\EOF\n> +1 1024 one-k-file\n> +10 2048 two-k-file\n> +EOF\n\nIs there any reason to use different filenames and sizes for the two\nruns? They should behave identically, so it would make more sense to me\nto subject them to identical inputs.\n\nI know you need different files and filenames to continue pushing into\nthe same \"dest\", but the problem is easily solved by bumping the \"git\ninit\" into the loop.\n\n-Peff\n"},{"id":"299455","messageId":"CAP8UFD2r-Zi757zizRQq-TPp+dO=+=ho=7oOipjPxY4ksmzC=g@mail.gmail.com","threadId":"43860","inReplyTo":"20160816131640.h2zzn3sy5qqdeewc@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] receive-pack: allow a maximum input size to be specified","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-08-16T14:27:10Z","receivedAt":"2016-08-16T14:27:17Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Aug 16, 2016 at 3:16 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Aug 16, 2016 at 10:17:01AM +0200, Christian Couder wrote:\n>>\n>> Cleaning up what has already been written to disk is a\n>> related problem that is not addressed by this patch.\n>>\n>> The problem is that git-gc can clean up tmp_pack_* files\n>> after their grace time expired, but that may not be\n>> enough if someone tries to \"git push\" in a loop.\n>>\n>> A simple fix is to call register_tempfile() in\n>> open_pack_file(), and just have index-pack clean up the\n>> file on its way out.\n>>\n>> But there are harder cases. For instance, if somebody\n>> pushes a 500MB file, and there is a pre-receive hook that\n>> says \"too big; I won't accept this\". And then they push in\n>> a loop, the incoming pack has already been accepted into\n>> the repository by the time the pre-receive hook runs.\n>> It's not possible to just delete it, because we don't know\n>> if other simultaneous processes have started to depend on\n>> the objects.\n>\n> IMHO, everything after \"not addressed by this patch\" doesn't really add\n> anything. This commit has value on its own, and the discussion about the\n> next steps is already documented on the list (and hopefully will be\n> fixed with other patches!).\n\nOk, I will remove that in the next iteration.\n\n>> +test_expect_success 'create remote repository' '\n>> +     git init --bare dest\n>> +'\n>> +\n>> +# Let's run tests with different unpack limits: 1 and 10\n>> +# When the limit is 1, `git receive-pack` will call `git index-pack`.\n>> +# When the limit is 10, `git receive-pack` will call `git unpack-objects`.\n>> +\n>> +while read unpacklimit filesize filename\n>> +do\n>> [...]\n>> +done <<\\EOF\n>> +1 1024 one-k-file\n>> +10 2048 two-k-file\n>> +EOF\n>\n> Is there any reason to use different filenames and sizes for the two\n> runs? They should behave identically, so it would make more sense to me\n> to subject them to identical inputs.\n\nAbout the sizes, I thought that some people might want to try sizes\ncloser to the limit and also that it is good anyway in general to add\na bit of \"randomness\", or at least variability, in the tests.\n\n> I know you need different files and filenames to continue pushing into\n> the same \"dest\", but the problem is easily solved by bumping the \"git\n> init\" into the loop.\n\nI thought that it would be a bit less wasteful to use the same \"dest\"\nand also it would make the test more realistic as people often push in\nnon empty repos.\n"},{"id":"299456","messageId":"CAP8UFD0mhhfNbcsiOpLqJ-4ZWOkKwcoJ9k3yQyO4jTem=s+yBg@mail.gmail.com","threadId":"43860","inReplyTo":"20160816131145.apq77nc2x7wb7lba@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] limit the size of the packs we receive","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-08-16T14:44:01Z","receivedAt":"2016-08-16T14:44:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Aug 16, 2016 at 3:11 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Aug 16, 2016 at 10:16:58AM +0200, Christian Couder wrote:\n>\n>>   - changed strtoul() to strtoumax() in the first 2 patches, as\n>>     suggested by Peff,\n>>\n>>   - changed git_config_ulong() to git_config_int64() and used PRIuMAX\n>>     and uintmax_t in the last patch, as suggested by Peff,\n>\n> Thinking a bit, off_t is actually signed. So maybe PRIdMAX (which we\n> don't seem to have compat macros for) would make more sense. I dunno if\n> anybody actually cares. This value shouldn't be signed anyway, and\n> nobody should be approaching the limits of a 64-bit number anyway (there\n> is no point in limiting the incoming pack to the exabyte range).\n>\n> So I'm inclined not to worry about it.\n\nYeah, you previously wrote:\n\n> We seem to use strtoumax() elsewhere, so that's probably a good match\n> (technically it can overflow an off_t, but in practice this value comes from\n> the admin and they will set something sane).\n\nand I thought that the same would apply to using PRIuMAX and uintmax_t\nin this patch.\n\nI can add something along your explanations in the commit message if\nit is prefered.\n"},{"id":"299458","messageId":"20160816144821.tbktj3l7agnsqddw@sigill.intra.peff.net","threadId":"43860","inReplyTo":"CAP8UFD0mhhfNbcsiOpLqJ-4ZWOkKwcoJ9k3yQyO4jTem=s+yBg@mail.gmail.com","subject":"Re: [PATCH 0/3] limit the size of the packs we receive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-16T14:48:22Z","receivedAt":"2016-08-16T14:49:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 16, 2016 at 04:44:01PM +0200, Christian Couder wrote:\n\n>> [sizes and signedness of off_t]\n> I can add something along your explanations in the commit message if\n> it is prefered.\n\nI think it's probably OK without it.\n\n-Peff\n"},{"id":"299474","messageId":"20160816162141.b3jtmun3kynjnwxs@sigill.intra.peff.net","threadId":"43860","inReplyTo":"CAP8UFD2r-Zi757zizRQq-TPp+dO=+=ho=7oOipjPxY4ksmzC=g@mail.gmail.com","subject":"Re: [PATCH 3/3] receive-pack: allow a maximum input size to be specified","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-16T16:21:41Z","receivedAt":"2016-08-16T16:22:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 16, 2016 at 04:27:10PM +0200, Christian Couder wrote:\n\n> >> +while read unpacklimit filesize filename\n> >> +do\n> >> [...]\n> >> +done <<\\EOF\n> >> +1 1024 one-k-file\n> >> +10 2048 two-k-file\n> >> +EOF\n> >\n> > Is there any reason to use different filenames and sizes for the two\n> > runs? They should behave identically, so it would make more sense to me\n> > to subject them to identical inputs.\n> \n> About the sizes, I thought that some people might want to try sizes\n> closer to the limit and also that it is good anyway in general to add\n> a bit of \"randomness\", or at least variability, in the tests.\n\nIn general, I'd prefer a systematic approach to introducing variables\ninto tests. If it's important that we test different sizes, then we\nshould do so, and not only test some combinations (and if it is not\nimportant to test different sizes, then we should not waste CPU time and\nthe mental energy of people reading the tests).\n\nIOW, when I see this, I wonder why the index-pack code path is not\ntested against a 2k file. But there really isn't a good reason. So\neither it does matter, and our tests do not have good coverage, or it\ndoes not, and it is just making me wonder if the tests are buggy.\n\nWorse, both files are created with the same seed via test-genrandom. So\nI suspect the 2k file is a superset of the 1k file, and we may send it\nis a thin delta (so it really is only testing a 1k input anyway!).\n\n> I thought that it would be a bit less wasteful to use the same \"dest\"\n> and also it would make the test more realistic as people often push in\n> non empty repos.\n\nI doubt it is expensive enough to matter in practice. Though note that\nif you push the same commit to two new repositories, then you can\namortize the test-genrandom/add/commit step (i.e., push the exact same\npackfile in both cases).\n\n-Peff\n"},{"id":"299612","messageId":"CAP8UFD0Wtekn-qR+RvRmj+rHFZS2DvFhqYdnygU0LwSabYLyoA@mail.gmail.com","threadId":"43860","inReplyTo":"20160816162141.b3jtmun3kynjnwxs@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] receive-pack: allow a maximum input size to be specified","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-08-18T14:06:48Z","receivedAt":"2016-08-18T14:08:18Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Aug 16, 2016 at 6:21 PM, Jeff King <peff@peff.net> wrote:\n>> >\n>> > Is there any reason to use different filenames and sizes for the two\n>> > runs? They should behave identically, so it would make more sense to me\n>> > to subject them to identical inputs.\n>>\n>> About the sizes, I thought that some people might want to try sizes\n>> closer to the limit and also that it is good anyway in general to add\n>> a bit of \"randomness\", or at least variability, in the tests.\n>\n> In general, I'd prefer a systematic approach to introducing variables\n> into tests. If it's important that we test different sizes, then we\n> should do so, and not only test some combinations (and if it is not\n> important to test different sizes, then we should not waste CPU time and\n> the mental energy of people reading the tests).\n>\n> IOW, when I see this, I wonder why the index-pack code path is not\n> tested against a 2k file. But there really isn't a good reason. So\n> either it does matter, and our tests do not have good coverage, or it\n> does not, and it is just making me wonder if the tests are buggy.\n\nI don't see things in the same way, but I will make the second file a\n1k file too as I don't really care much about that.\n\n> Worse, both files are created with the same seed via test-genrandom. So\n> I suspect the 2k file is a superset of the 1k file, and we may send it\n> is a thin delta (so it really is only testing a 1k input anyway!).\n\nYeah, I will use a different seed for the second file.\n\n>> I thought that it would be a bit less wasteful to use the same \"dest\"\n>> and also it would make the test more realistic as people often push in\n>> non empty repos.\n>\n> I doubt it is expensive enough to matter in practice. Though note that\n> if you push the same commit to two new repositories, then you can\n> amortize the test-genrandom/add/commit step (i.e., push the exact same\n> packfile in both cases).\n\nYeah, it probably doesn't matter anyway regarding efficiency, but I\nstill prefer to not create a new repo as I would find it more\nconfusing for the reader to use different repos.\n\nThanks,\nChristian.\n"}]}