{"thread":{"id":"43856","subject":"[RFC/PATCH 0/3] limit the size of the packs we receive","startedAt":"2016-08-15T19:58:36Z","lastAt":"2016-08-16T14:46:55Z","messageCount":11,"participants":["Christian Couder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"299380","messageId":"20160815195729.16826-1-chriscool@tuxfamily.org","threadId":"43856","inReplyTo":null,"subject":"[RFC/PATCH 0/3] limit the size of the packs we receive","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-08-15T19:57:26Z","receivedAt":"2016-08-15T19:58:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In 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\nSo I rebased Peff's patch to the current master, refreshed it a bit,\nsplit it, and added the missing --max-input-size= option to\n`git unpack-objects` - to make it work for all `transfer.unpacklimit`\nvalues - in a new patch.\n\nThere is no documentation yet for the `--max-input-size=`\noptions added to `git index-pack` and `git unpack-objects`, nor for\nthe new `receive.maxsize` config option.\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\nChristian Couder (1):\n unpack-objects: add --max-input-size= option\n\nJeff King (2):\n index-pack: add --max-input-size= option\n receive-pack: allow a maximum input size to be specified\n\n builtin/index-pack.c | 5 +++++\n builtin/receive-pack.c | 12 ++++++++++++\n builtin/unpack-objects.c | 7 +++++++\n t/t5546-push-limits.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 71 insertions(+)\n create mode 100755 t/t5546-push-limits.sh\n\n-- \n2.10.0.rc0.4.g229e32c.dirty\n\n"},{"id":"299381","messageId":"20160815195729.16826-2-chriscool@tuxfamily.org","threadId":"43856","inReplyTo":"20160815195729.16826-1-chriscool@tuxfamily.org","subject":"[RFC/PATCH 1/3] index-pack: add --max-input-size= option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-08-15T19:57:27Z","receivedAt":"2016-08-15T19:59:05Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Jeff King \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 \nSigned-off-by: Christian Couder \n---\n builtin/index-pack.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1d2ea58..1fd60bd 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 = strtoul(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.4.g229e32c.dirty\n\n"},{"id":"299382","messageId":"20160815195729.16826-3-chriscool@tuxfamily.org","threadId":"43856","inReplyTo":"20160815195729.16826-1-chriscool@tuxfamily.org","subject":"[RFC/PATCH 2/3] unpack-objects: add --max-input-size= option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-08-15T19:57:28Z","receivedAt":"2016-08-15T19:59:08Z","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 \n---\n builtin/unpack-objects.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 172470b..59b1f39 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 = strtoul(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.4.g229e32c.dirty\n\n"},{"id":"299383","messageId":"20160815195729.16826-4-chriscool@tuxfamily.org","threadId":"43856","inReplyTo":"20160815195729.16826-1-chriscool@tuxfamily.org","subject":"[RFC/PATCH 3/3] receive-pack: allow a maximum input size to be specified","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-08-15T19:57:29Z","receivedAt":"2016-08-15T19:59:19Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Jeff King \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\nWhat has already been written to disk can be cleaned\noutside of receive-pack.\n\nSigned-off-by: Jeff King \nSigned-off-by: Christian Couder \n---\n builtin/receive-pack.c | 12 ++++++++++++\n t/t5546-push-limits.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 59 insertions(+)\n create mode 100755 t/t5546-push-limits.sh\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 92e1213..7627f7f 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_ulong(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=%lu\",\n+\t\t\t\tmax_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=%lu\",\n+\t\t\t\tmax_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..d3a4d1a\n--- /dev/null\n+++ b/t/t5546-push-limits.sh\n@@ -0,0 +1,47 @@\n+#!/bin/sh\n+\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+'\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+\n+test_expect_success 'bumping limit allows push' '\n+\tgit --git-dir=dest config receive.maxsize 4k &&\n+\tgit push dest HEAD\n+'\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+\n+test_expect_success 'change unpacklimit' '\n+\tgit --git-dir=dest config receive.unpacklimit 10\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+\n+test_expect_success 'bumping limit allows push' '\n+\tgit --git-dir=dest config receive.maxsize 4k &&\n+\tgit push dest HEAD\n+'\n+\n+test_done\n-- \n2.10.0.rc0.4.g229e32c.dirty\n\n"},{"id":"299386","messageId":"20160815201058.v2u24nnu6rjwlr2k@sigill.intra.peff.net","threadId":"43856","inReplyTo":"20160815195729.16826-2-chriscool@tuxfamily.org","subject":"Re: [RFC/PATCH 1/3] index-pack: add --max-input-size= option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-15T20:10:58Z","receivedAt":"2016-08-15T20:11:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 15, 2016 at 09:57:27PM +0200, Christian Couder wrote:\n\n> From: Jeff King \n> \n> When receiving a pack-file, it can be useful to abort the\n> `git index-pack`, if the pack-file is too big.\n\nNot much rationale here. I guess because it is all in the 3rd patch,\nwhich ties it into receive-pack. I'm not sure it's worth repeating. I\nguess it could all be squished back into one patch. I'm OK either way.\n\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 1d2ea58..1fd60bd 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\nLooks good. I see you marked it for translation, which makes sense.\n\nOn the original, I waffled on whether to share the size with the user in\nthe message. I didn't want to encourage people with \"oh, if it's under\n2G it must be OK, then!\". Because really 2G was meant to be a \"you\nreally shouldn't get this high, and we will unceremoniously dump your\npush if you do\".\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 = strtoul(arg, NULL, 10);\n\nmax_input_size is an off_t, but your parse only up to ULONG_MAX here.\nFor my purposes in the original patch, this was OK, as we set it at 2GB,\nwhich works everywhere (and also, GitHub systems all have 64-bit \"long\"\nthese days). But somebody on a 32-bit system could not set this to 4GB,\neven though I think index-pack could otherwise handle it. We seem to use\nstrtoumax() elsewhere, so that's probably a good match (technically it\ncan overflow an off_t, but in practice this value comes from the admin\nand they will set something sane).\n\n-Peff\n"},{"id":"299387","messageId":"20160815201132.73wls7gk5hquuib3@sigill.intra.peff.net","threadId":"43856","inReplyTo":"20160815195729.16826-3-chriscool@tuxfamily.org","subject":"Re: [RFC/PATCH 2/3] unpack-objects: add --max-input-size= option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-15T20:11:33Z","receivedAt":"2016-08-15T20:11:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 15, 2016 at 09:57:28PM +0200, Christian Couder wrote:\n\n> When receiving a pack-file, it can be useful to abort the\n> `git unpack-objects`, if the pack-file is too big.\n> \n> Signed-off-by: Christian Couder \n\nSame remarks here as on the last patch, including strtoumax. :)\n\n-Peff\n"},{"id":"299394","messageId":"20160815204034.rrjn57wigxtjpgye@sigill.intra.peff.net","threadId":"43856","inReplyTo":"20160815195729.16826-4-chriscool@tuxfamily.org","subject":"Re: [RFC/PATCH 3/3] receive-pack: allow a maximum input size to be specified","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-15T20:40:34Z","receivedAt":"2016-08-15T20:40:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 15, 2016 at 09:57:29PM +0200, Christian Couder wrote:\n\n> From: Jeff King \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> What has already been written to disk can be cleaned\n> outside of receive-pack.\n\nThis second paragraph hints at a related problem.\n\nGit is generally happy to leave tmp_pack_* around to be cleaned up later\nnext time git-gc runs. Including its default 2-week grace time.\n\nSo imagine that tries to \"git push\" in a loop. And each time they push,\nyou say \"nope, that's too big\". And each time you acquire a new 2GB\ntmp_pack file. If your goal was to prevent somebody from streaming\nstraight to your filesystem and filling up your disk, then it wasn't\nvery successful. :)\n\nThe simple fix is to call register_tempfile() in open_pack_file(), and\njust have index-pack clean up the file on its way out.\n\nBut there are harder cases. For instance, imagine somebody pushes a\n500MB file, and you have a pre-receive hook that says \"too big; I won't\naccept this\". And then they push in a loop, as before. You've accepted\nthe incoming pack into the repository by the time the pre-receive runs.\nYou can't just delete it, because you don't know if other simultaneous\nprocesses have started to depend on the objects.\n\nTo solve that, I have patches that put incoming packfiles into a\n\"quarantine\" area, then run the connectivity check and pre-receive hooks\nwith the quarantine accessible via GIT_ALTERNATE_OBJECT_DIRECTORIES. And\nthen we either move the quarantine packs into the real repo, or blow\naway the tmpdir, depending on whether the hooks said the objects were\nOK.\n\nThose are patches I plan to share upstream but just haven't gotten\naround to yet.\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 92e1213..7627f7f 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_ulong(var, value);\n> +\t\treturn 0;\n> +\t}\n\nAnother off_t/ulong mismatch. I think you want git_config_int64() here.\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=%lu\",\n> +\t\t\t\tmax_input_size);\n\nAnd here, PRIuMAX and uintmax_t. Or perhaps simpler, just store the\nvalue as a string here and pass it on to index-pack (which would then\nneed to learn to handle suffixes like \"2g\"). We do a similar trick in\nrepack; see b861e23 (repack: propagate pack-objects options as strings,\n2014-01-22).\n\n> diff --git a/t/t5546-push-limits.sh b/t/t5546-push-limits.sh\n> new file mode 100755\n> index 0000000..d3a4d1a\n> --- /dev/null\n> +++ b/t/t5546-push-limits.sh\n> @@ -0,0 +1,47 @@\n> +#!/bin/sh\n> +\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> +'\n\nWe're going to do basically the same battery of tests against an\nunpacklimit of \"1\" (to catch index-pack) and of \"10\" (to catch\nunpack-objects). It might be clearer to just have a for-loop like:\n\n for unpacklimit in 1 100\n do\n\ttest_expect_success 'create remote repository' '\n\t\trm -rf dest &&\n\t\tgit init --bare dest &&\n\t\tgit -C dest config receive.unpacklimit $unpacklimit\n\t'\n\n\ttest_expect_success 'receive.maxsize rejects push' '\n\t\tgit -C dest config receive.maxsize 512 &&\n\t\ttest_must_fail git push dest HEAD &&\n\t'\n\n\ttest_expect_success 'bumping limit allows push' '\n\t\tgit -C dest config receive.maxsize 4k &&\n\t\tgit push dest HEAD\n\t'\n done\n\nand it's probably worth a comment at the top of the loop explaining what\nthe heck those numbers mean. :)\n\n-Peff\n"},{"id":"299419","messageId":"xmqqr39pejhk.fsf@gitster.mtv.corp.google.com","threadId":"43856","inReplyTo":"20160815204034.rrjn57wigxtjpgye@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 3/3] receive-pack: allow a maximum input size to be specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-15T22:48:55Z","receivedAt":"2016-08-15T22:49:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The simple fix is to call register_tempfile() in open_pack_file(), and\n> just have index-pack clean up the file on its way out.\n>\n> But there are harder cases. For instance, imagine somebody pushes a\n> 500MB file, and you have a pre-receive hook that says \"too big; I won't\n> accept this\". And then they push in a loop, as before. You've accepted\n> the incoming pack into the repository by the time the pre-receive runs.\n> You can't just delete it, because you don't know if other simultaneous\n> processes have started to depend on the objects.\n>\n> To solve that, I have patches that put incoming packfiles into a\n> \"quarantine\" area, then run the connectivity check and pre-receive hooks\n> with the quarantine accessible via GIT_ALTERNATE_OBJECT_DIRECTORIES. And\n> then we either move the quarantine packs into the real repo, or blow\n> away the tmpdir, depending on whether the hooks said the objects were\n> OK.\n>\n> Those are patches I plan to share upstream but just haven't gotten\n> around to yet.\n\nI think these other patches can come later, independent from this\nthree-patch series resurrected from the archive, so I can take a\nreroll of these once the integer-size issues you pointed out are\nsorted out.\n"},{"id":"299426","messageId":"20160816010309.zstrljqknc3vjziq@sigill.intra.peff.net","threadId":"43856","inReplyTo":"xmqqr39pejhk.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 3/3] receive-pack: allow a maximum input size to be specified","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-16T01:03:09Z","receivedAt":"2016-08-16T01:03:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 15, 2016 at 03:48:55PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The simple fix is to call register_tempfile() in open_pack_file(), and\n> > just have index-pack clean up the file on its way out.\n> >\n> > But there are harder cases. For instance, imagine somebody pushes a\n> > 500MB file, and you have a pre-receive hook that says \"too big; I won't\n> > accept this\". And then they push in a loop, as before. You've accepted\n> > the incoming pack into the repository by the time the pre-receive runs.\n> > You can't just delete it, because you don't know if other simultaneous\n> > processes have started to depend on the objects.\n> >\n> > To solve that, I have patches that put incoming packfiles into a\n> > \"quarantine\" area, then run the connectivity check and pre-receive hooks\n> > with the quarantine accessible via GIT_ALTERNATE_OBJECT_DIRECTORIES. And\n> > then we either move the quarantine packs into the real repo, or blow\n> > away the tmpdir, depending on whether the hooks said the objects were\n> > OK.\n> >\n> > Those are patches I plan to share upstream but just haven't gotten\n> > around to yet.\n> \n> I think these other patches can come later, independent from this\n> three-patch series resurrected from the archive, so I can take a\n> reroll of these once the integer-size issues you pointed out are\n> sorted out.\n\nYeah, definitely it's independent. I was mostly suggesting to Christian\nthat he may want to look into the \"easy\" register_tempfile() case, as\nGitLab may find this is only half of the fix that they want.\n\nAlso a patch I may try to polish and share in the future, but not as\nhigh priority as some of the other stuff.\n\n-Peff\n"},{"id":"299437","messageId":"CAP8UFD2Bpp-gcxEQGNSFxvKyptkyeAsMtaHZaBebJO2XiF+W3g@mail.gmail.com","threadId":"43856","inReplyTo":"20160816010309.zstrljqknc3vjziq@sigill.intra.peff.net","subject":"Re: [RFC/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:25:44Z","receivedAt":"2016-08-16T08:26:06Z","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:03 AM, Jeff King wrote:\n> On Mon, Aug 15, 2016 at 03:48:55PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>>\n>> > The simple fix is to call register_tempfile() in open_pack_file(), and\n>> > just have index-pack clean up the file on its way out.\n>> >\n>> > But there are harder cases. For instance, imagine somebody pushes a\n>> > 500MB file, and you have a pre-receive hook that says \"too big; I won't\n>> > accept this\". And then they push in a loop, as before. You've accepted\n>> > the incoming pack into the repository by the time the pre-receive runs.\n>> > You can't just delete it, because you don't know if other simultaneous\n>> > processes have started to depend on the objects.\n>> >\n>> > To solve that, I have patches that put incoming packfiles into a\n>> > \"quarantine\" area, then run the connectivity check and pre-receive hooks\n>> > with the quarantine accessible via GIT_ALTERNATE_OBJECT_DIRECTORIES. And\n>> > then we either move the quarantine packs into the real repo, or blow\n>> > away the tmpdir, depending on whether the hooks said the objects were\n>> > OK.\n\nThanks Peff for the review and all these explanations and suggestions!\n\n>> > Those are patches I plan to share upstream but just haven't gotten\n>> > around to yet.\n\nI would be happy to help if I can on these patches too!\n\n>> I think these other patches can come later, independent from this\n>> three-patch series resurrected from the archive, so I can take a\n>> reroll of these once the integer-size issues you pointed out are\n>> sorted out.\n\nOk, I just sent a non RFC reroll of the series with those issues\nhopefully fixed.\n\n> Yeah, definitely it's independent. I was mostly suggesting to Christian\n> that he may want to look into the \"easy\" register_tempfile() case, as\n> GitLab may find this is only half of the fix that they want.\n\nYeah, thanks again for this suggestion!\n\n> Also a patch I may try to polish and share in the future, but not as\n> high priority as some of the other stuff.\n\nI can help polishing this patch if you want.\n\nThanks,\nChristian.\n"},{"id":"299457","messageId":"20160816144642.5ikkta4l5hyx6act@sigill.intra.peff.net","threadId":"43856","inReplyTo":"CAP8UFD2Bpp-gcxEQGNSFxvKyptkyeAsMtaHZaBebJO2XiF+W3g@mail.gmail.com","subject":"Re: [RFC/PATCH 3/3] receive-pack: allow a maximum input size to be specified","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-16T14:46:42Z","receivedAt":"2016-08-16T14:46:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 16, 2016 at 10:25:44AM +0200, Christian Couder wrote:\n\n> >> > Those are patches I plan to share upstream but just haven't gotten\n> >> > around to yet.\n> \n> I would be happy to help if I can on these patches too!\n\nThanks. I'll try to extract the quarantine patches, though I have a few\nother things in my backlog first. I wrote them with the intent of\nupstreaming, so I think they should be fairly clean.\n\n> [register_tempfile on odb tmpfiles]\n> > Also a patch I may try to polish and share in the future, but not as\n> > high priority as some of the other stuff.\n> \n> I can help polishing this patch if you want.\n\nThe original patch is not worth looking at; it predated all of the\ntempfile.c infrastructure, so it added its own hacky versions those\nfunctions.\n\nThis is basically what we're doing now (this is extracted from a larger\ndiff, so the line numbers may be a little funny):\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 3431de2..9c34353 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -304,8 +346,10 @@ static const char *open_pack_file(const char *pack_name)\n \t\tinput_fd = 0;\n \t\tif (!pack_name) {\n \t\t\tstatic char tmp_file[PATH_MAX];\n+\t\t\tstruct tempfile *t = xcalloc(1, sizeof(*t));\n \t\t\toutput_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),\n \t\t\t\t\t\t\"pack/tmp_pack_XXXXXX\");\n+\t\t\tregister_tempfile(t, tmp_file);\n \t\t\tpack_name = xstrdup(tmp_file);\n \t\t} else\n \t\t\toutput_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);\ndiff --git a/pack-write.c b/pack-write.c\nindex 33293ce..14597b4 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"pack.h\"\n #include \"csum-file.h\"\n+#include \"tempfile.h\"\n \n void reset_pack_idx_option(struct pack_idx_option *opts)\n {\n@@ -73,7 +74,9 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \t} else {\n \t\tif (!index_name) {\n \t\t\tstatic char tmp_file[PATH_MAX];\n+\t\t\tstruct tempfile *t = xcalloc(1, sizeof(*t));\n \t\t\tfd = odb_mkstemp(tmp_file, sizeof(tmp_file), \"pack/tmp_idx_XXXXXX\");\n+\t\t\tregister_tempfile(t, tmp_file);\n \t\t\tindex_name = xstrdup(tmp_file);\n \t\t} else {\n \t\t\tunlink(index_name);\n@@ -327,10 +330,13 @@ int encode_in_pack_object_header(enum object_type type, uintmax_t size, unsigned\n \n struct sha1file *create_tmp_packfile(char **pack_tmp_name)\n {\n+\tstruct tempfile *t = xcalloc(1, sizeof(*t));\n \tchar tmpname[PATH_MAX];\n \tint fd;\n \n \tfd = odb_mkstemp(tmpname, sizeof(tmpname), \"pack/tmp_pack_XXXXXX\");\n+\n+\tregister_tempfile(t, tmpname);\n \t*pack_tmp_name = xstrdup(tmpname);\n \treturn sha1fd(fd, *pack_tmp_name);\n }\n\nBut it makes me wonder if we should just be switching odb_mkstemp() to\ncalling register_tempfile(). Or better yet, switching out\ngit_mkstemp_mode() for one of the tempfile.h functions (mks_tempfile_m,\nI think). There may be some trickery with the allocation of \"struct\ntempfile\" though (the struct needs to persist for the remainder of the\nprogram, but we would not want to allocate and leak one per object, so\nwe need some way of reusing them).\n\nI also wonder if it should be configurable whether to keep half-written\ntemporary files. In the early days, it was a good source of debugging\n(e.g., look at this half-written pack and see why index-pack choked on\nit). But these days I doubt that is really that helpful. So maybe it\nwould be OK to just drop them unconditionally.\n\n-Peff\n"}]}