{"thread":{"id":"56612","subject":"[PATCH] receive-pack: allow a maximum input object size specified","startedAt":"2021-09-30T12:11:23Z","lastAt":"2021-10-01T18:43:40Z","messageCount":10,"participants":["Han Xin","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jiang Xin","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"437517","messageId":"20210930121058.5771-1-chiyutianyi@gmail.com","threadId":"56612","inReplyTo":null,"subject":"[PATCH] receive-pack: allow a maximum input object size specified","fromName":"Han Xin","fromEmail":"chiyutianyi@gmail.com","sentAt":"2021-09-30T12:10:58Z","receivedAt":"2021-09-30T12:11:23Z","isPatch":true,"sender":{"key":"chiyutianyi@gmail.com","avatar":null},"body":"From: Han Xin <hanxin.hx@alibaba-inc.com>\n\n'receive.maxInputSize' help us to stop writing bytes\nto disk by a global cutoff point, but sometimes we only\nwant to say no for large objects. Let's allow a new cutoff\npoint where we will stop writing big objects' bytes to disk.\n\nSigned-off-by: Han Xin <hanxin.hx@alibaba-inc.com>\n---\n builtin/index-pack.c      |  5 +++++\n builtin/receive-pack.c    | 12 ++++++++++++\n builtin/unpack-objects.c  |  8 ++++++++\n t/t5546-receive-limits.sh | 33 +++++++++++++++++++++++++++++----\n 4 files changed, 54 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 8336466865..0e62b356c6 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -133,6 +133,7 @@ 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 off_t max_input_object_size;\n static unsigned deepest_delta;\n static git_hash_ctx input_ctx;\n static uint32_t input_crc32;\n@@ -519,6 +520,8 @@ static void *unpack_raw_entry(struct object_entry *obj,\n \t\tshift += 7;\n \t}\n \tobj->size = size;\n+\tif (max_input_object_size && size > max_input_object_size)\n+\t\tdie(_(\"object exceeds maximum allowed size \"));\n \n \tswitch (obj->type) {\n \tcase OBJ_REF_DELTA:\n@@ -1825,6 +1828,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\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 if (skip_prefix(arg, \"--max-input-object-size=\", &arg)) {\n+\t\t\t\tmax_input_object_size = strtoumax(arg, NULL, 10);\n \t\t\t} else if (skip_prefix(arg, \"--object-format=\", &arg)) {\n \t\t\t\thash_algo = hash_algo_by_name(arg);\n \t\t\t\tif (hash_algo == GIT_HASH_UNKNOWN)\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 2d1f97e1ca..82ff0c61ff 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -57,6 +57,7 @@ static int advertise_push_options;\n static int advertise_sid;\n static int unpack_limit = 100;\n static off_t max_input_size;\n+static off_t max_input_object_size;\n static int report_status;\n static int report_status_v2;\n static int use_sideband;\n@@ -242,6 +243,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.maxinputobjectsize\") == 0) {\n+\t\tmax_input_object_size = git_config_int64(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (strcmp(var, \"receive.procreceiverefs\") == 0) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n@@ -2237,6 +2243,9 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\tif (max_input_size)\n \t\t\tstrvec_pushf(&child.args, \"--max-input-size=%\"PRIuMAX,\n \t\t\t\t     (uintmax_t)max_input_size);\n+\t\tif (max_input_object_size)\n+\t\t\tstrvec_pushf(&child.args, \"--max-input-object-size=%\"PRIuMAX,\n+\t\t\t\t     (uintmax_t)max_input_object_size);\n \t\tchild.no_stdout = 1;\n \t\tchild.err = err_fd;\n \t\tchild.git_cmd = 1;\n@@ -2268,6 +2277,9 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\tif (max_input_size)\n \t\t\tstrvec_pushf(&child.args, \"--max-input-size=%\"PRIuMAX,\n \t\t\t\t     (uintmax_t)max_input_size);\n+\t\tif (max_input_object_size)\n+\t\t\tstrvec_pushf(&child.args, \"--max-input-object-size=%\"PRIuMAX,\n+\t\t\t\t     (uintmax_t)max_input_object_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 4a9466295b..04d9fa918f 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -22,6 +22,7 @@ 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 off_t max_input_object_size;\n static git_hash_ctx ctx;\n static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;\n static struct progress *progress;\n@@ -466,6 +467,9 @@ static void unpack_one(unsigned nr)\n \t\tshift += 7;\n \t}\n \n+\tif (max_input_object_size && size > max_input_object_size)\n+\t\tdie(_(\"object exceeds maximum allowed size \"));\n+\n \tswitch (type) {\n \tcase OBJ_COMMIT:\n \tcase OBJ_TREE:\n@@ -568,6 +572,10 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\tmax_input_size = strtoumax(arg, NULL, 10);\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (skip_prefix(arg, \"--max-input-object-size=\", &arg)) {\n+\t\t\t\tmax_input_object_size = strtoumax(arg, NULL, 10);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tusage(unpack_usage);\n \t\t}\n \ndiff --git a/t/t5546-receive-limits.sh b/t/t5546-receive-limits.sh\nindex 0b0e987fdb..11fd374abc 100755\n--- a/t/t5546-receive-limits.sh\n+++ b/t/t5546-receive-limits.sh\n@@ -19,16 +19,16 @@ test_pack_input_limit () {\n \t'\n \n \ttest_expect_success \"set unpacklimit to $unpack_limit\" '\n-\t\tgit --git-dir=dest config receive.unpacklimit \"$unpack_limit\"\n+\t\tgit -C dest config receive.unpacklimit \"$unpack_limit\"\n \t'\n \n \ttest_expect_success 'setting receive.maxInputSize to 512 rejects push' '\n-\t\tgit --git-dir=dest config receive.maxInputSize 512 &&\n+\t\tgit -C dest config receive.maxInputSize 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.maxInputSize 4k &&\n+\t\tgit -C dest config receive.maxInputSize 4k &&\n \t\tgit push dest HEAD\n \t'\n \n@@ -38,7 +38,32 @@ test_pack_input_limit () {\n \t'\n \n \ttest_expect_success 'lifting the limit allows push' '\n-\t\tgit --git-dir=dest config receive.maxInputSize 0 &&\n+\t\tgit -C dest config receive.maxInputSize 0 &&\n+\t\tgit push dest HEAD\n+\t'\n+\n+\ttest_expect_success 'prepare destination repository' '\n+\t\trm -fr dest &&\n+\t\tgit --bare init dest\n+\t'\n+\n+\ttest_expect_success 'setting receive.maxInputObjectSize to 512 rejects push' '\n+\t\tgit -C dest config receive.maxInputObjectSize 512 &&\n+\t\ttest_must_fail git push dest HEAD\n+\t'\n+\n+\ttest_expect_success 'bumping limit to 2k allows push' '\n+\t\tgit -C dest config receive.maxInputObjectSize 2k &&\n+\t\tgit push dest HEAD\n+\t'\n+\n+\ttest_expect_success 'prepare destination repository (again)' '\n+\t\trm -fr dest &&\n+\t\tgit --bare init dest\n+\t'\n+\n+\ttest_expect_success 'lifting the limit allows push' '\n+\t\tgit --git-dir=dest config receive.maxInputObjectSize 0 &&\n \t\tgit push dest HEAD\n \t'\n }\n-- \n2.33.0.1.g1026118a84\n\n"},{"id":"437520","messageId":"20210930132004.16075-1-chiyutianyi@gmail.com","threadId":"56612","inReplyTo":"20210930121058.5771-1-chiyutianyi@gmail.com","subject":"[PATCH v2] receive-pack: not receive pack file with large object","fromName":"Han Xin","fromEmail":"chiyutianyi@gmail.com","sentAt":"2021-09-30T13:20:04Z","receivedAt":"2021-09-30T13:20:23Z","isPatch":true,"sender":{"key":"chiyutianyi@gmail.com","avatar":null},"body":"From: Han Xin <hanxin.hx@alibaba-inc.com>\n\nIn addition to using 'receive.maxInputSize' to limit the overall size\nof the received packfile, a new config variable\n'receive.maxInputObjectSize' is added to limit the push of a single\nobject larger than this threshold.\n\nSigned-off-by: Han Xin <hanxin.hx@alibaba-inc.com>\n---\n Documentation/config/receive.txt |  6 ++++++\n builtin/index-pack.c             |  5 +++++\n builtin/receive-pack.c           | 12 ++++++++++++\n builtin/unpack-objects.c         |  8 ++++++++\n t/t5546-receive-limits.sh        | 25 +++++++++++++++++++++++++\n 5 files changed, 56 insertions(+)\n\ndiff --git a/Documentation/config/receive.txt b/Documentation/config/receive.txt\nindex 85d5b5a3d2..4823ead8e4 100644\n--- a/Documentation/config/receive.txt\n+++ b/Documentation/config/receive.txt\n@@ -74,6 +74,12 @@ receive.maxInputSize::\n \taccepting the pack file. If not set or set to 0, then the size\n \tis unlimited.\n \n+receive.maxInputObjectSize::\n+\tIf one of the objects in the incoming pack stream is larger than\n+\tthis limit, then git-receive-pack will error out, instead of\n+\taccepting the pack file. If not set or set to 0, then the size\n+\tis 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/builtin/index-pack.c b/builtin/index-pack.c\nindex 8336466865..0e62b356c6 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -133,6 +133,7 @@ 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 off_t max_input_object_size;\n static unsigned deepest_delta;\n static git_hash_ctx input_ctx;\n static uint32_t input_crc32;\n@@ -519,6 +520,8 @@ static void *unpack_raw_entry(struct object_entry *obj,\n \t\tshift += 7;\n \t}\n \tobj->size = size;\n+\tif (max_input_object_size && size > max_input_object_size)\n+\t\tdie(_(\"object exceeds maximum allowed size \"));\n \n \tswitch (obj->type) {\n \tcase OBJ_REF_DELTA:\n@@ -1825,6 +1828,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\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 if (skip_prefix(arg, \"--max-input-object-size=\", &arg)) {\n+\t\t\t\tmax_input_object_size = strtoumax(arg, NULL, 10);\n \t\t\t} else if (skip_prefix(arg, \"--object-format=\", &arg)) {\n \t\t\t\thash_algo = hash_algo_by_name(arg);\n \t\t\t\tif (hash_algo == GIT_HASH_UNKNOWN)\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 2d1f97e1ca..82ff0c61ff 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -57,6 +57,7 @@ static int advertise_push_options;\n static int advertise_sid;\n static int unpack_limit = 100;\n static off_t max_input_size;\n+static off_t max_input_object_size;\n static int report_status;\n static int report_status_v2;\n static int use_sideband;\n@@ -242,6 +243,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.maxinputobjectsize\") == 0) {\n+\t\tmax_input_object_size = git_config_int64(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (strcmp(var, \"receive.procreceiverefs\") == 0) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n@@ -2237,6 +2243,9 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\tif (max_input_size)\n \t\t\tstrvec_pushf(&child.args, \"--max-input-size=%\"PRIuMAX,\n \t\t\t\t     (uintmax_t)max_input_size);\n+\t\tif (max_input_object_size)\n+\t\t\tstrvec_pushf(&child.args, \"--max-input-object-size=%\"PRIuMAX,\n+\t\t\t\t     (uintmax_t)max_input_object_size);\n \t\tchild.no_stdout = 1;\n \t\tchild.err = err_fd;\n \t\tchild.git_cmd = 1;\n@@ -2268,6 +2277,9 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\tif (max_input_size)\n \t\t\tstrvec_pushf(&child.args, \"--max-input-size=%\"PRIuMAX,\n \t\t\t\t     (uintmax_t)max_input_size);\n+\t\tif (max_input_object_size)\n+\t\t\tstrvec_pushf(&child.args, \"--max-input-object-size=%\"PRIuMAX,\n+\t\t\t\t     (uintmax_t)max_input_object_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 4a9466295b..04d9fa918f 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -22,6 +22,7 @@ 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 off_t max_input_object_size;\n static git_hash_ctx ctx;\n static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;\n static struct progress *progress;\n@@ -466,6 +467,9 @@ static void unpack_one(unsigned nr)\n \t\tshift += 7;\n \t}\n \n+\tif (max_input_object_size && size > max_input_object_size)\n+\t\tdie(_(\"object exceeds maximum allowed size \"));\n+\n \tswitch (type) {\n \tcase OBJ_COMMIT:\n \tcase OBJ_TREE:\n@@ -568,6 +572,10 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\tmax_input_size = strtoumax(arg, NULL, 10);\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (skip_prefix(arg, \"--max-input-object-size=\", &arg)) {\n+\t\t\t\tmax_input_object_size = strtoumax(arg, NULL, 10);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tusage(unpack_usage);\n \t\t}\n \ndiff --git a/t/t5546-receive-limits.sh b/t/t5546-receive-limits.sh\nindex 0b0e987fdb..52c8f1f2e8 100755\n--- a/t/t5546-receive-limits.sh\n+++ b/t/t5546-receive-limits.sh\n@@ -41,6 +41,31 @@ test_pack_input_limit () {\n \t\tgit --git-dir=dest config receive.maxInputSize 0 &&\n \t\tgit push dest HEAD\n \t'\n+\n+\ttest_expect_success 'prepare destination repository (test for large object)' '\n+\t\trm -fr dest &&\n+\t\tgit --bare init dest\n+\t'\n+\n+\ttest_expect_success 'setting receive.maxInputObjectSize to 512 rejects push large object' '\n+\t\tgit -C dest config receive.maxInputObjectSize 512 &&\n+\t\ttest_must_fail git push dest HEAD\n+\t'\n+\n+\ttest_expect_success 'bumping limit to 2k allows push large object' '\n+\t\tgit -C dest config receive.maxInputObjectSize 2k &&\n+\t\tgit push dest HEAD\n+\t'\n+\n+\ttest_expect_success 'prepare destination repository (test for large object,again)' '\n+\t\trm -fr dest &&\n+\t\tgit --bare init dest\n+\t'\n+\n+\ttest_expect_success 'lifting the limit allows push' '\n+\t\tgit -C dest config receive.maxInputObjectSize 0 &&\n+\t\tgit push dest HEAD\n+\t'\n }\n \n test_expect_success \"create known-size (1024 bytes) commit\" '\n-- \n2.33.0.1.g0542904d0a.dirty\n\n"},{"id":"437542","messageId":"87pmsqtb2p.fsf@evledraar.gmail.com","threadId":"56612","inReplyTo":"20210930132004.16075-1-chiyutianyi@gmail.com","subject":"Re: [PATCH v2] receive-pack: not receive pack file with large object","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-30T13:42:29Z","receivedAt":"2021-09-30T14:04:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 30 2021, Han Xin wrote:\n\n> From: Han Xin <hanxin.hx@alibaba-inc.com>\n>\n> In addition to using 'receive.maxInputSize' to limit the overall size\n> of the received packfile, a new config variable\n> 'receive.maxInputObjectSize' is added to limit the push of a single\n> object larger than this threshold.\n\nMaybe an unfair knee-jerk reaction: I think we should really be pushing\nthis sort of thing into pre-receive hooks and/or the proc-receive hook,\ni.e. see 15d3af5e22e (receive-pack: add new proc-receive hook,\n2020-08-27).\n\nThe latter pre-dates c08db5a2d0d (receive-pack: allow a maximum input\nsize to be specified, 2016-08-24), which may or may not be relevant\nhere.\n\nAnyway, I think there may be dragons here that you haven't\nconsidered. Is the \"size\" here the absolute size on disk, or the delta\nsize (I'm offhand not familiar enough with unpack-objects.c to\nknow). Does this have the same semantics no matter the\ntransfer.unpackLimit?\n\nEither way, if it's the absolute size you may have a 100MB object that's\na fantastic delta candidate, so it may only add a few bytes to your\nrepo, or it's /dev/urandom output and really is adding 100MB.\n\nIf we're relying on deltas there's no guarantee that what the client is\nsending is delta-ing against something we can delta against, although\nadmittedly this is also an issue with receive.maxInputSize.\n\nAnyway, all of this is stuff that people \"in the wild\" don't consider\nalready, so maybe I'm being too much of a curmudgeon here :)\n\nAt an ex-job I re-wrote some \"big push\" blocking hook that had had N\niterations of other implementations getting it subtly wrong. I.e. if you\ndefine \"size\" the wrong way you end up blocking things like the revert\nthat reverts \"master\" to yesterday's commit.\n\nThat's somehing that takes git almost no size at all to store, but which\ndepending on the stupidity of the hook that's on the other end may be\nblocked as a \"big push\".\n\nSo I think if we're going to expand on the existing\n\"receive.maxInputSize\" we should really be considering the edge cases\ncarefully.\n\nBut even then I'm somewhat skeptical of the benefit of doing this in\ngit's own guts v.s. a hook, or expanding the hook infrastructure to\naccommodate it, we have \"receive.maxInputSize\", now maybe\n\"receive.maxInputObjectSize\", then after that perhaps\n\"receive.maxInputBinaryObjectSize\",\n\"receive.maxInputObjectSizeGlob=*.mp3\" etc.\n\nI'm not saying our hook infrastructure is good enough for this right\nnow, but surely anything that would be wanted as a check in this area\ncould be done by something that \"git cat-file --batch\"-style feeds OIDs\nto a new hook over stdin from \"index-pack\" and \"unpack-objects\"? With\nthat hook aware of the temporary staged objects in the incoming-* area\nof the store?. I.e. if the performance aspect of not waiting until\n\"pre-receive\" time is a concern...\n"},{"id":"437554","messageId":"xmqqczoqdn4m.fsf@gitster.g","threadId":"56612","inReplyTo":"20210930132004.16075-1-chiyutianyi@gmail.com","subject":"Re: [PATCH v2] receive-pack: not receive pack file with large object","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-30T16:49:45Z","receivedAt":"2021-09-30T16:49:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Xin <chiyutianyi@gmail.com> writes:\n\n> @@ -519,6 +520,8 @@ static void *unpack_raw_entry(struct object_entry *obj,\n>  \t\tshift += 7;\n>  \t}\n>  \tobj->size = size;\n> +\tif (max_input_object_size && size > max_input_object_size)\n> +\t\tdie(_(\"object exceeds maximum allowed size \"));\n>  \n>  \tswitch (obj->type) {\n>  \tcase OBJ_REF_DELTA:\n\nHere obj->size is the inflated payload size of a single entry in the\npackfile.  If it happens to be represented as a base object\n(i.e. without delta, just deflated), it would be close to the size\nof the blob in the working tree (but LF->CRLF conversion and the\nlike may further inflate it), but if it is a delta object, this size\nis just the size of the delta data we feed patch_delta() with, and\nhas no relevance to the actual \"file size\".\n\nSure, it is called max_INPUT_object_size and we can say we are not\nlimiting the final disk size, and that might be a workable excuse\nto check based on the obj->size here, but then its usefulness from\nthe point of view of end users, who decide to set the variable to\nlimit \"some\" usage, becomes dubious.\n\nSo...\n\n"},{"id":"437601","messageId":"CANYiYbHNBcDaoF+QE_+62EXUZD_caaJDFmt7v1_BddQfpdVcvg@mail.gmail.com","threadId":"56612","inReplyTo":"87pmsqtb2p.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] receive-pack: not receive pack file with large object","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2021-10-01T02:30:24Z","receivedAt":"2021-10-01T02:30:38Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Thu, Sep 30, 2021 at 10:05 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Thu, Sep 30 2021, Han Xin wrote:\n>\n> > From: Han Xin <hanxin.hx@alibaba-inc.com>\n> >\n> > In addition to using 'receive.maxInputSize' to limit the overall size\n> > of the received packfile, a new config variable\n> > 'receive.maxInputObjectSize' is added to limit the push of a single\n> > object larger than this threshold.\n>\n> Maybe an unfair knee-jerk reaction: I think we should really be pushing\n> this sort of thing into pre-receive hooks and/or the proc-receive hook,\n> i.e. see 15d3af5e22e (receive-pack: add new proc-receive hook,\n> 2020-08-27).\n\nLast week, one user complained that he cannot push to his repo in our\nserver, and later Han Xin discovered the user was trying to push a\nvery big blob object over 10GB. For this case, the \"pre-receive\" hook\nhad no change to execute because \"git-receive-pack\" died early because\nof OOM.  The function \"unpack_non_delta_entry()\" in\n\"builtin/unpack-objects.c\" will try to allocate memory for the whole\n10GB blob but no lucky.\n\nHan Xin is preparing another patch to resolve the OOM issue found in\n\"unpack_non_delta_entry()\". But we think it is reasonable to prevent\nsuch a big blob in a pack to git-receive-pack, because it will be\nslower to check objects from pack and loose objects in the quarantine\nusing pre-receive hook.\n\n> Anyway, I think there may be dragons here that you haven't\n> considered. Is the \"size\" here the absolute size on disk, or the delta\n> size (I'm offhand not familiar enough with unpack-objects.c to\n> know). Does this have the same semantics no matter the\n> transfer.unpackLimit?\n\nYes, according to setting of transfer.unpackLimit, may call\ngit-index-pack to save the pack directly, or expand it by calling\ngit-unpack-object. The \"size\" may be the absolute size on disk, or the\ndelta size. But we know blob over 500MB (default value of\ncore.bigFileThreshold) will not be deltafied, so can we assume this\n\"size\" is the absolute size on disk?\n\n--\nJiang Xin\n"},{"id":"437603","messageId":"CANYiYbHfw1=MLVv1+utXPUtg3mn1DoZGL0t5WH+w8sjdDrkHYA@mail.gmail.com","threadId":"56612","inReplyTo":"xmqqczoqdn4m.fsf@gitster.g","subject":"Re: [PATCH v2] receive-pack: not receive pack file with large object","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2021-10-01T02:52:15Z","receivedAt":"2021-10-01T02:52:29Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Fri, Oct 1, 2021 at 12:50 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Han Xin <chiyutianyi@gmail.com> writes:\n>\n> > @@ -519,6 +520,8 @@ static void *unpack_raw_entry(struct object_entry *obj,\n> >               shift += 7;\n> >       }\n> >       obj->size = size;\n> > +     if (max_input_object_size && size > max_input_object_size)\n> > +             die(_(\"object exceeds maximum allowed size \"));\n> >\n> >       switch (obj->type) {\n> >       case OBJ_REF_DELTA:\n>\n> Here obj->size is the inflated payload size of a single entry in the\n> packfile.  If it happens to be represented as a base object\n> (i.e. without delta, just deflated), it would be close to the size\n> of the blob in the working tree (but LF->CRLF conversion and the\n> like may further inflate it), but if it is a delta object, this size\n> is just the size of the delta data we feed patch_delta() with, and\n> has no relevance to the actual \"file size\".\n>\n> Sure, it is called max_INPUT_object_size and we can say we are not\n> limiting the final disk size, and that might be a workable excuse\n> to check based on the obj->size here, but then its usefulness from\n> the point of view of end users, who decide to set the variable to\n> limit \"some\" usage, becomes dubious.\n\nJust like what I replied to Ævar, if the max_input_object_size is\ngreater than core.bigFileThreshold, is it save to save the size here\nis almost the actual \"file size\"?\n\nBTW, Han Xin will continue to resolvie the OOM issue found in\n\"unpack_non_delta_entry()\" after our Nation Day holiday.\n\n--\nJiang Xin\n"},{"id":"437611","messageId":"YVan+n1surlXfiEw@coredump.intra.peff.net","threadId":"56612","inReplyTo":"CANYiYbHNBcDaoF+QE_+62EXUZD_caaJDFmt7v1_BddQfpdVcvg@mail.gmail.com","subject":"Re: [PATCH v2] receive-pack: not receive pack file with large object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-01T06:17:30Z","receivedAt":"2021-10-01T06:17:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 01, 2021 at 10:30:24AM +0800, Jiang Xin wrote:\n\n> > Maybe an unfair knee-jerk reaction: I think we should really be pushing\n> > this sort of thing into pre-receive hooks and/or the proc-receive hook,\n> > i.e. see 15d3af5e22e (receive-pack: add new proc-receive hook,\n> > 2020-08-27).\n> \n> Last week, one user complained that he cannot push to his repo in our\n> server, and later Han Xin discovered the user was trying to push a\n> very big blob object over 10GB. For this case, the \"pre-receive\" hook\n> had no change to execute because \"git-receive-pack\" died early because\n> of OOM.  The function \"unpack_non_delta_entry()\" in\n> \"builtin/unpack-objects.c\" will try to allocate memory for the whole\n> 10GB blob but no lucky.\n> \n> Han Xin is preparing another patch to resolve the OOM issue found in\n> \"unpack_non_delta_entry()\". But we think it is reasonable to prevent\n> such a big blob in a pack to git-receive-pack, because it will be\n> slower to check objects from pack and loose objects in the quarantine\n> using pre-receive hook.\n\nI think index-pack handles this case correctly, at least for base\nobjects. In unpack_entry_data(), it will stream anything bigger than\nbig_file_threshold. Probably unpack-objects needs to learn the same\ntrick.\n\nIn general, the code in index-pack has gotten a _lot_ more attention\nover the years than unpack-objects. I'd trust its security a lot more,\nand it has extra performance enhancements (like multithreading and\nstreaming). At GitHub, we always run index-pack on incoming packs, and\nnever unpack-objects. I'm tempted to say we should stop using\nunpack-objects entirely for incoming packs, and then either:\n\n  - just keep packs for incoming objects. It's not really any worse of a\n    state than loose objects, and it all eventually gets rolled up by gc\n    anyway. (The exception is that thin packs may duplicate base objects\n    until that rollup).\n\n  - teach index-pack an \"--unpack\" option. I actually wrote some patches\n    for this a while back. It's not very much code, but there were some\n    rough edges and I never came back to them. I'm happy to share if\n    anybody's interested.\n\nThough I would note that for deltas, even index-pack will not stream the\ncontents. You generally shouldn't have very large deltas, as clients\nwill also avoid computing them (because that also involves putting the\nwhole object in memory). But the notion of \"large\" is not necessarily\nthe same between client and server. And of course somebody can\nmaliciously make a giant delta.\n\n-Peff\n"},{"id":"437613","messageId":"YVaptAklXNShTY0j@coredump.intra.peff.net","threadId":"56612","inReplyTo":"CANYiYbHfw1=MLVv1+utXPUtg3mn1DoZGL0t5WH+w8sjdDrkHYA@mail.gmail.com","subject":"Re: [PATCH v2] receive-pack: not receive pack file with large object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-01T06:24:52Z","receivedAt":"2021-10-01T06:24:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 01, 2021 at 10:52:15AM +0800, Jiang Xin wrote:\n\n> > Sure, it is called max_INPUT_object_size and we can say we are not\n> > limiting the final disk size, and that might be a workable excuse\n> > to check based on the obj->size here, but then its usefulness from\n> > the point of view of end users, who decide to set the variable to\n> > limit \"some\" usage, becomes dubious.\n> \n> Just like what I replied to Ævar, if the max_input_object_size is\n> greater than core.bigFileThreshold, is it save to save the size here\n> is almost the actual \"file size\"?\n\nIf we are storing a pack with index-pack, the on-disk size will match\nexactly this input size. If we unpack it to loose, then big files don't\ntend to have deltas or to compress with zlib, but that is not always the\ncase. I have definitely seen people try to store gigantic text files.\n\nIf your goal is introduce a user-facing object-size limit, then I think\nthe \"logical\" size of the uncompressed object is the only thing that\nmakes sense. Everything else is subject to change, and can be gamed in\nweird ways.\n\nIf your goal is to avoid malicious pushers causing you to allocate too\nmuch memory, then you might want to have some limits on the compressed\nsizes you'll deal with, especially for deltas. But I don't think the\nchecks here do that, because I can send a small delta that reconstructs\na much larger object (which we'd eventually reconstruct in order to\ncompute its sha1).\n\n-Peff\n"},{"id":"437615","messageId":"YVaw6agcPNclhws8@coredump.intra.peff.net","threadId":"56612","inReplyTo":"87pmsqtb2p.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] receive-pack: not receive pack file with large object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-01T06:55:37Z","receivedAt":"2021-10-01T06:55:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 30, 2021 at 03:42:29PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > From: Han Xin <hanxin.hx@alibaba-inc.com>\n> >\n> > In addition to using 'receive.maxInputSize' to limit the overall size\n> > of the received packfile, a new config variable\n> > 'receive.maxInputObjectSize' is added to limit the push of a single\n> > object larger than this threshold.\n> \n> Maybe an unfair knee-jerk reaction: I think we should really be pushing\n> this sort of thing into pre-receive hooks and/or the proc-receive hook,\n> i.e. see 15d3af5e22e (receive-pack: add new proc-receive hook,\n> 2020-08-27).\n\nYes, this could be done as a hook, but it's actually kind of tricky.\nWe have a similar max-object-size limit in our fork at GitHub, and it's\nimplemented inside index-pack (we don't have a match in unpack-objects,\nbut we always run index-pack on incoming packs).\n\nUnlike the patches here, ours is limiting the logical size of an object\n(so a 100MB blob limit is on the uncompressed size). It happens in\nsha1_object(), where we have that size.\n\nThe main reason we put it there is for performance. index-pack is\nalready computing the size, so it's free to check it there. But it does\nmake some things awkward. Rather than just dying with \"woah, some object\nis too big\", it's nice to tell the user \"hey, the object 1234abcd at\nfoo.bin is too big\". To do that, we actually collect the list of too-big\nobjects and index them anyway (remember that we're in the quarantine\ndirectory, so nothing is permanent until later). We write the list to a\n\".large-objects\" file in the quarantine directory. And then hooks are\nfree to produce a much nicer message (e.g., by running something like\n\"log --find-objects\" to get the path name at which we see the blob).\n\nIt would be nice if the hook could just find the objects itself, but\nthere are some gotchas:\n\n  - finding the list of pushed objects is awkward and/or expensive. You\n    can use rev-list, but that's very costly, as it has to inflate all\n    of the new trees. You really just want the list of what's in the\n    quarantine directory, but there's no single command to get that. You\n    have to run show-index on the incoming pack, plus poke through the\n    loose object directories.\n\n    It would be much easier if \"cat-file --batch-all-objects\" could be\n    asked to only look at local objects. That would also allow some\n    other optimizations to reduce the cost. For instance, even when\n    iterating packs, cat-file then feeds each sha1 back to\n    oid_object_info(), even though we already know the exact pack/offset\n    where it can be found. Likewise, it could be taught some basic\n    filtering (like \"show only objects with size > X\") to avoid dumping\n    millions of oid/size pairs to a separate script to do the filtering.\n\n    But all of that would just be approaching the speed of having\n    index-pack do the check, since it has the size already there.\n\n  - it wouldn't help index-pack at all with memory attacks by malicious\n    pushers. Here somebody accidentally pushed up a big blob, and it\n    caused unpack-objects to OOM. But I also remember a problem ages ago\n    where some of the fsck_tree() code had an integer overflow\n    vulnerability, which could only be triggered by a gigantic tree.\n    In the patch we use at GitHub, we allow large blobs to make it to\n    the hook level, but too-large trees, commits, and tags are\n    unceremoniously aborted with an immediate die(). Real users\n    accidentally try to push 2GB objects. Trees of that size are no\n    accident. :)\n\n    Having index-pack enforce size limits helps a bit there. And\n    streaming large blobs helps protect against accidents. But there are\n    still OOM problems with maliciously constructed inputs, because the\n    delta code only works on full buffers (so it really wants to\n    construct the whole object before we get to the sha1_object()\n    limits). It would be nice if this could stream the output, but I\n    suspect it would require pretty major surgery to the delta code.\n\n    Instead, we've put in a crude limit by having receive-pack set\n    GIT_ALLOC_LIMIT in the environment, which then ensures that its\n    child index-pack won't allocate too much (the limit we use is much\n    higher than the more user-facing object limit, because the point is\n    just to avoid DoS attacks, and not enforce any kind of policy).\n\n    It might be nice to have a config option for setting this limit (and\n    perhaps even putting it at a reasonable defensive default).\n\nSo I do think there are some interesting paths forward for doing more of\nthis in hooks, but there's work to be done to get there.\n\n> The latter pre-dates c08db5a2d0d (receive-pack: allow a maximum input\n> size to be specified, 2016-08-24), which may or may not be relevant\n> here.\n\nYeah, I introduced that limit. It wasn't really about object size or\nmemory, but just about the fact that without it, a malicious pusher can\njust send you bytes forever which the server will write to disk. It also\nhelps with the most naive kind of large-object attacks, but it wouldn't\nhelp with a cleverly constructed delta.\n\nIt is, unfortunately, a pretty unceremonious die(), and has caused more\nthan one support ticket (especially because GitHub has used 2GB for the\nlimit for ages, and the practical limit for repositories has been\ngrowing).\n\nSo there. That's probably more than anybody wanted to know about push\nlimits. ;) I'm happy to share any of the code from GitHub's fork in\nthese areas. The main reason I haven't is just that some of it is just\nkind of ugly and may need polishing (e.g., the whole \"write to\n.large-objects\" is really an awful interface).\n\n-Peff\n"},{"id":"437718","messageId":"xmqqv92g7fhm.fsf@gitster.g","threadId":"56612","inReplyTo":"YVaw6agcPNclhws8@coredump.intra.peff.net","subject":"Re: [PATCH v2] receive-pack: not receive pack file with large object","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-01T18:43:33Z","receivedAt":"2021-10-01T18:43:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Unlike the patches here, ours is limiting the logical size of an object\n> (so a 100MB blob limit is on the uncompressed size). It happens in\n> sha1_object(), where we have that size.\n\nI like it when I hear people doing things in the \"right\" way (the\ndefinition of being \"right\" in this case is to apply the limit to\nthe size that is end-user facing---otherwise you cannot justify an\narbitrary limit).  ;-)\n"}]}