{"thread":{"id":"55976","subject":"[PATCH 0/2] pack-objects: missing tests & --stdin-packs segfault fix","startedAt":"2021-06-21T15:03:46Z","lastAt":"2021-07-20T16:59:12Z","messageCount":11,"participants":["Ævar Arnfjörð Bjarmason","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"428038","messageId":"cover-0.2-00000000000-20210621T145819Z-avarab@gmail.com","threadId":"55976","inReplyTo":null,"subject":"[PATCH 0/2] pack-objects: missing tests & --stdin-packs segfault fix","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T15:03:36Z","receivedAt":"2021-06-21T15:03:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When re-rolling an unrelated series[1] dealing with pack-objects.c and\nrevision.c I discovered that we have some test blindspots, and that\nthe newly added --stdin-packs option in v2.32.0 will segfault if fed\ngarbage data.\n\nThis fixes the test blindspots, and 2/2 fixes the segfault.\n\nAs discussed in its commit message I'm being lazy about emitting the\nerror message. If you supply N bogus lines on stdin we'll error on the\nfirst one, since the input is first sorted by the string-list.c\nAPI. The test case for the error message relies on which of two SHA\nlines sorts first, and I picked input that happens to sort the same\nway under both SHA-1 and SHA-256.\n\nLazy, but I figured for this use-case it wasn't worth keeping track of\nwhat line we saw when, or to refactor the parsing check on pack names\nas we get input lines.\n\n1. https://lore.kernel.org/git/cover-0.4-0000000000-20210617T105537Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (2):\n pack-objects tests: cover blindspots in stdin handling\n pack-objects: fix segfault in --stdin-packs option\n\n builtin/pack-objects.c | 10 ++++\n t/t5300-pack-object.sh | 103 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 113 insertions(+)\n\n-- \n2.32.0.599.g3967b4fa4ac\n\n"},{"id":"428039","messageId":"patch-1.2-dd2efd1cfb9-20210621T145819Z-avarab@gmail.com","threadId":"55976","inReplyTo":"cover-0.2-00000000000-20210621T145819Z-avarab@gmail.com","subject":"[PATCH 1/2] pack-objects tests: cover blindspots in stdin handling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T15:03:37Z","receivedAt":"2021-06-21T15:03:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Cover blindspots in the testing of stdin handling, including the\n\"!len\" condition added in b5d97e6b0a0 (pack-objects: run rev-list\nequivalent internally., 2006-09-04). The codepath taken with --revs\nand read_object_list_from_stdin() acts differently in some of these\ncommon cases, let's test for those.\n\nThe \"--stdin --revs\" test being added here stresses the combination of\n--stdin-packs and the revision.c --stdin argument, some of this was\ncovered in a test added in 339bce27f4f (builtin/pack-objects.c: add\n'--stdin-packs' option, 2021-02-22), but let's make sure that\nGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true keeps erroring out about\n--stdin, and it isn't picked up by the revision.c API's handling of\nthat option.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5300-pack-object.sh | 85 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 85 insertions(+)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 5c5e53f0be9..65e991e3706 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -34,6 +34,91 @@ test_expect_success 'setup' '\n \t} >expect\n '\n \n+test_expect_success 'setup pack-object in <<-EOF &&\n+\t$(git -C pack-object-stdin rev-parse one)\n+\tEOF\n+\n+\tgit -C pack-object-stdin pack-objects basic-stdin actual &&\n+\ttest_line_count = 1 actual &&\n+\n+\tgit -C pack-object-stdin pack-objects --revs basic-stdin-revs actual &&\n+\ttest_line_count = 3 actual\n+'\n+\n+test_expect_success 'pack-object in <<-EOF &&\n+\t$(git -C pack-object-stdin rev-parse one)\n+\tgarbage\n+\t$(git -C pack-object-stdin rev-parse two)\n+\tEOF\n+\n+\tsed \"s/^> //g\" >err.expect <<-EOF &&\n+\tfatal: expected object ID, got garbage:\n+\t> garbage\n+\n+\tEOF\n+\ttest_must_fail git -C pack-object-stdin pack-objects bad-line-stdin err.actual &&\n+\ttest_cmp err.expect err.actual &&\n+\n+\tcat >err.expect <<-EOF &&\n+\tfatal: bad revision '\"'\"'garbage'\"'\"'\n+\tEOF\n+\ttest_must_fail git -C pack-object-stdin pack-objects --revs bad-line-stdin-revs err.actual &&\n+\ttest_cmp err.expect err.actual\n+'\n+\n+test_expect_success 'pack-object in <<-EOF &&\n+\t$(git -C pack-object-stdin rev-parse one)\n+\n+\t$(git -C pack-object-stdin rev-parse two)\n+\tEOF\n+\n+\tsed -e \"s/^> //g\" -e \"s/Z$//g\" >err.expect <<-EOF &&\n+\tfatal: expected object ID, got garbage:\n+\t> Z\n+\n+\tEOF\n+\ttest_must_fail git -C pack-object-stdin pack-objects empty-line-stdin err.actual &&\n+\ttest_cmp err.expect err.actual &&\n+\n+\tgit -C pack-object-stdin pack-objects --revs empty-line-stdin-revs actual &&\n+\ttest_line_count = 3 actual\n+'\n+\n+test_expect_success 'pack-object in <<-EOF &&\n+\t$(git -C pack-object-stdin rev-parse one)\n+\t$(git -C pack-object-stdin rev-parse two)\n+\tEOF\n+\n+\t# There is the \"--stdin-packs is incompatible with --revs\"\n+\t# test below, but we should make sure that the revision.c\n+\t# --stdin is not picked up\n+\tcat >err.expect <<-EOF &&\n+\tfatal: disallowed abbreviated or ambiguous option '\"'\"'stdin'\"'\"'\n+\tEOF\n+\ttest_must_fail git -C pack-object-stdin pack-objects stdin-with-stdin-option --stdin err.actual &&\n+\ttest_cmp err.expect err.actual &&\n+\n+\ttest_must_fail git -C pack-object-stdin pack-objects --stdin --revs stdin-with-stdin-option-revs 2>err.actual \n # e.g.: check_deltas stderr -gt 0\n check_deltas() {\n-- \n2.32.0.599.g3967b4fa4ac\n\n"},{"id":"428040","messageId":"patch-2.2-a9702132385-20210621T145819Z-avarab@gmail.com","threadId":"55976","inReplyTo":"cover-0.2-00000000000-20210621T145819Z-avarab@gmail.com","subject":"[PATCH 2/2] pack-objects: fix segfault in --stdin-packs option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T15:03:38Z","receivedAt":"2021-06-21T15:03:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a segfault in the --stdin-packs option added in\n339bce27f4f (builtin/pack-objects.c: add '--stdin-packs' option,\n2021-02-22). The read_packs_list_from_stdin() function didn't check\nthat the lines it was reading were valid packs, and thus when doing\nthe QSORT() with pack_mtime_cmp() we'd have a NULL \"util\" field.\n\nThe logic error was in assuming that we could iterate all packs and\nannotate the excluded and included packs we got, as opposed to\nchecking the lines we got on stdin. There was a check for excluded\npacks, but included packs were simply assumed to be valid.\n\nAs noted in the test we'll not report the first bad line, but whatever\nline sorted first according to the string-list.c API. In this case I\nthink that's fine.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 10 ++++++++++\n t/t5300-pack-object.sh | 18 ++++++++++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex de00adbb9e0..65579e09fe0 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3310,6 +3310,16 @@ static void read_packs_list_from_stdin(void)\n \t\t\titem->util = p;\n \t}\n \n+\t/*\n+\t * Arguments we got on stdin may not even be packs. Check that\n+\t * to avoid segfaulting later on in e.g. pack_mtime_cmp().\n+\t */\n+\tfor_each_string_list_item(item, &include_packs) {\n+\t\tstruct packed_git *p = item->util;\n+\t\tif (!p)\n+\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n+\t}\n+\n \t/*\n \t * First handle all of the excluded packs, marking them as kept in-core\n \t * so that later calls to add_object_entry() discards any objects that\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 65e991e3706..330deec656b 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -119,6 +119,24 @@ test_expect_success 'pack-object in <<-EOF &&\n+\t$(git -C pack-object-stdin rev-parse one)\n+\t$(git -C pack-object-stdin rev-parse two)\n+\tEOF\n+\n+\t# We actually just report the first bad line in strcmp()\n+\t# order, it just so happens that we get the same result under\n+\t# SHA-1 and SHA-256 here. It does not really matter that we\n+\t# report the first bad item in this obscure case, so this\n+\t# oddity of the test is OK.\n+\tcat >err.expect <<-EOF &&\n+\tfatal: could not find pack '\"'\"'$(git -C pack-object-stdin rev-parse two)'\"'\"'\n+\tEOF\n+\ttest_must_fail git -C pack-object-stdin pack-objects stdin-with-stdin-option --stdin-packs err.actual &&\n+\ttest_cmp err.expect err.actual\n+'\n+\n # usage: check_deltas \n # e.g.: check_deltas stderr -gt 0\n check_deltas() {\n-- \n2.32.0.599.g3967b4fa4ac\n\n"},{"id":"428102","messageId":"YND3h2l10PlnSNGJ@nand.local","threadId":"55976","inReplyTo":"patch-2.2-a9702132385-20210621T145819Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] pack-objects: fix segfault in --stdin-packs option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-21T20:33:11Z","receivedAt":"2021-06-21T20:33:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jun 21, 2021 at 05:03:38PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Fix a segfault in the --stdin-packs option added in\n> 339bce27f4f (builtin/pack-objects.c: add '--stdin-packs' option,\n> 2021-02-22). The read_packs_list_from_stdin() function didn't check\n> that the lines it was reading were valid packs, and thus when doing\n> the QSORT() with pack_mtime_cmp() we'd have a NULL \"util\" field.\n\nIt may be worth mentioning that the util pointer is used to associate\nthe names of included/excluded packs with the packed_git structs they\ncorrespond to. I see it's mentioned in the very next paragraph, but it\nmay be helpful for other readers to see this information earlier.\n\n> The logic error was in assuming that we could iterate all packs and\n> annotate the excluded and included packs we got, as opposed to\n> checking the lines we got on stdin. There was a check for excluded\n> packs, but included packs were simply assumed to be valid.\n>\n> As noted in the test we'll not report the first bad line, but whatever\n> line sorted first according to the string-list.c API. In this case I\n> think that's fine.\n\nYeah. There isn't really a better way to do that since we don't have a\nconvenient function to look up packs by their name. Much more convenient\nis to loop through all packs and assign them to entries in the\nstring_list one by one. That's O(n*log(n)), but it doesn't really matter\nhere since we expect n to be small-ish, and this is by far not the most\nexpensive part of writing a pack.\n\nYou could imagine doing something O(n^2) by looping through all packs\neach time you receive a line of input. That performs worse, but arguably\nprovides a better experience when using this mode interactively. But\nthat is probably a relatively rare occurrence, so it likely doesn't\nmatter.\n\nEqually, you could build a mapping from pack name to packed_git struct\nahead of time, and then do the lookups in constant time. That's linear,\nof course, but you pay for it in memory. Honestly, the memory cost is\nprobably quite reasonable, but it may not be worth the effort, since I\nsuspect the vast majority of usage here is from 'git repack\n--geometric'.\n\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/pack-objects.c | 10 ++++++++++\n> t/t5300-pack-object.sh | 18 ++++++++++++++++++\n> 2 files changed, 28 insertions(+)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index de00adbb9e0..65579e09fe0 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3310,6 +3310,16 @@ static void read_packs_list_from_stdin(void)\n> \t\t\titem->util = p;\n> \t}\n>\n> +\t/*\n> +\t * Arguments we got on stdin may not even be packs. Check that\n> +\t * to avoid segfaulting later on in e.g. pack_mtime_cmp().\n> +\t */\n\nCould be worth adding \"excluded packs are handled below\".\n\n> +\tfor_each_string_list_item(item, &include_packs) {\n> +\t\tstruct packed_git *p = item->util;\n> +\t\tif (!p)\n> +\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n> +\t}\n> +\n> \t/*\n> \t * First handle all of the excluded packs, marking them as kept in-core\n\n...and it may be worth updating this comment with s/First/Then.\n\n> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\n> index 65e991e3706..330deec656b 100755\n> --- a/t/t5300-pack-object.sh\n> +++ b/t/t5300-pack-object.sh\n> @@ -119,6 +119,24 @@ test_expect_success 'pack-object \ttest_cmp err.expect err.actual\n> '\n>\n> +test_expect_success 'pack-object +\tcat >in <<-EOF &&\n> +\t$(git -C pack-object-stdin rev-parse one)\n> +\t$(git -C pack-object-stdin rev-parse two)\n> +\tEOF\n\nIt's not a big deal, but here-doc directly into `git pack-objects` is\nmuch more common in t5300 than first redirecting it to a separate file.\nI probably would have written (in a sub-shell to avoid -C\npack-object-stdin everywhere):\n\n\n cd pack-object-stdin &&\n test_must_fail git pack-objects --stdout --stdin-packs >/dev/null 2>actual <<-EOF\n $(git rev-parse one)\n $(git rev-parse two)\n EOF\n\nAlthough the line is kind of long anyway (and it'd be even longer since\nthe subshell will get its own level of indentation). So I could entirely\nbuy that you did this for readability, which is fine by me.\n\n> +\n> +\t# We actually just report the first bad line in strcmp()\n> +\t# order, it just so happens that we get the same result under\n> +\t# SHA-1 and SHA-256 here. It does not really matter that we\n> +\t# report the first bad item in this obscure case, so this\n> +\t# oddity of the test is OK.\n> +\tcat >err.expect <<-EOF &&\n> +\tfatal: could not find pack '\"'\"'$(git -C pack-object-stdin rev-parse two)'\"'\"'\n> +\tEOF\n> +\ttest_must_fail git -C pack-object-stdin pack-objects stdin-with-stdin-option --stdin-packs err.actual &&\n> +\ttest_cmp err.expect err.actual\n\nIf we don't care which is reported (and it just so happens that we'll\nget the first one in lexical order), I would be fine with\n\n test_i18ngrep \"could not find pack\" err.actual\n\ntoo. It would be good to get rid of this comment and put it in the patch\nmessage in more detail (instead of just referring to it as \"[a]s noted\nin the test\".\n\nThanks,\nTaylor\n"},{"id":"428103","messageId":"YND32s96yDkM51qZ@nand.local","threadId":"55976","inReplyTo":"cover-0.2-00000000000-20210621T145819Z-avarab@gmail.com","subject":"Re: [PATCH 0/2] pack-objects: missing tests & --stdin-packs segfault fix","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-21T20:34:34Z","receivedAt":"2021-06-21T20:34:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jun 21, 2021 at 05:03:36PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> When re-rolling an unrelated series[1] dealing with pack-objects.c and\n> revision.c I discovered that we have some test blindspots, and that\n> the newly added --stdin-packs option in v2.32.0 will segfault if fed\n> garbage data.\n>\n> This fixes the test blindspots, and 2/2 fixes the segfault.\n\nThanks. I took a close look at the second patch, and it looks good to me\nwith a few minor comments. The first patch looks good as well, although\nI didn't look as closely.\n\n> As discussed in its commit message I'm being lazy about emitting the\n> error message. If you supply N bogus lines on stdin we'll error on the\n> first one, since the input is first sorted by the string-list.c\n> API. The test case for the error message relies on which of two SHA\n> lines sorts first, and I picked input that happens to sort the same\n> way under both SHA-1 and SHA-256.\n>\n> Lazy, but I figured for this use-case it wasn't worth keeping track of\n> what line we saw when, or to refactor the parsing check on pack names\n> as we get input lines.\n\nYeah. I think what you wrote is entirely reasonable, too. I suggested\nsome alternatives if you are feeling motivated to make the error\nreporting nicer, but as you say, I think the vast majority of use-cases\ndon't care about the output.\n\nThanks,\nTaylor\n"},{"id":"429530","messageId":"cover-0.2-00000000000-20210709T101014Z-avarab@gmail.com","threadId":"55976","inReplyTo":"cover-0.2-00000000000-20210621T145819Z-avarab@gmail.com","subject":"[PATCH v2 0/2] pack-objects: missing tests & --stdin-packs segfault fix","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-09T10:13:46Z","receivedAt":"2021-07-09T10:16:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When re-rolling an unrelated series[1] dealing with pack-objects.c and\nrevision.c I discovered that we have some test blindspots, and that\nthe newly added --stdin-packs option in v2.32.0 will segfault if fed\ngarbage data.\n\nSee\nhttp://lore.kernel.org/git/cover-0.2-00000000000-20210621T145819Z-avarab@gmail.com\nfor the v1 of this series.\n\nThis hopefully addresses Taylor's comments in\nhttps://lore.kernel.org/git/YND3h2l10PlnSNGJ@nand.local/\n\nI didn't end up moving away from the \"\n \n ## builtin/pack-objects.c ##\n @@ builtin/pack-objects.c: static void read_packs_list_from_stdin(void)\n - \t\t\titem->util = p;\n \t}\n \n -+\t/*\n -+\t * Arguments we got on stdin may not even be packs. Check that\n -+\t * to avoid segfaulting later on in e.g. pack_mtime_cmp().\n + \t/*\n +-\t * First handle all of the excluded packs, marking them as kept in-core\n +-\t * so that later calls to add_object_entry() discards any objects that\n +-\t * are also found in excluded packs.\n ++\t * Arguments we got on stdin may not even be packs. First\n ++\t * check that to avoid segfaulting later on in\n ++\t * e.g. pack_mtime_cmp(), excluded packs are handled below.\n ++\t *\n ++\t * Since we first parsed our STDIN and then sorted the input\n ++\t * lines the pack we error on will be whatever line happens to\n ++\t * sort first. This is lazy, it's enough that we report one\n ++\t * bad case here, we don't need to report the first/last one,\n ++\t * or all of them.\n +\t */\n +\tfor_each_string_list_item(item, &include_packs) {\n +\t\tstruct packed_git *p = item->util;\n @@ builtin/pack-objects.c: static void read_packs_list_from_stdin(void)\n +\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n +\t}\n +\n - \t/*\n - \t * First handle all of the excluded packs, marking them as kept in-core\n - \t * so that later calls to add_object_entry() discards any objects that\n ++\t/*\n ++\t * Then, handle all of the excluded packs, marking them as\n ++\t * kept in-core so that later calls to add_object_entry()\n ++\t * discards any objects that are also found in excluded packs.\n + \t */\n + \tfor_each_string_list_item(item, &exclude_packs) {\n + \t\tstruct packed_git *p = item->util;\n \n ## t/t5300-pack-object.sh ##\n @@ t/t5300-pack-object.sh: test_expect_success 'pack-object err.expect <<-EOF &&\n +\tfatal: could not find pack '\"'\"'$(git -C pack-object-stdin rev-parse two)'\"'\"'\n +\tEOF\n -+\ttest_must_fail git -C pack-object-stdin pack-objects stdin-with-stdin-option --stdin-packs err.actual &&\n ++\ttest_must_fail git \\\n ++\t\t-C pack-object-stdin \\\n ++\t\tpack-objects stdin-with-stdin-option --stdin-packs \\\n ++\t\terr.actual &&\n +\ttest_cmp err.expect err.actual\n +'\n +\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429531","messageId":"patch-1.2-8a4d4b820e7-20210709T101015Z-avarab@gmail.com","threadId":"55976","inReplyTo":"cover-0.2-00000000000-20210709T101014Z-avarab@gmail.com","subject":"[PATCH v2 1/2] pack-objects tests: cover blindspots in stdin handling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-09T10:13:47Z","receivedAt":"2021-07-09T10:16:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Cover blindspots in the testing of stdin handling, including the\n\"!len\" condition added in b5d97e6b0a0 (pack-objects: run rev-list\nequivalent internally., 2006-09-04). The codepath taken with --revs\nand read_object_list_from_stdin() acts differently in some of these\ncommon cases, let's test for those.\n\nThe \"--stdin --revs\" test being added here stresses the combination of\n--stdin-packs and the revision.c --stdin argument, some of this was\ncovered in a test added in 339bce27f4f (builtin/pack-objects.c: add\n'--stdin-packs' option, 2021-02-22), but let's make sure that\nGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true keeps erroring out about\n--stdin, and it isn't picked up by the revision.c API's handling of\nthat option.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5300-pack-object.sh | 85 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 85 insertions(+)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 5c5e53f0be9..65e991e3706 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -34,6 +34,91 @@ test_expect_success 'setup' '\n \t} >expect\n '\n \n+test_expect_success 'setup pack-object in <<-EOF &&\n+\t$(git -C pack-object-stdin rev-parse one)\n+\tEOF\n+\n+\tgit -C pack-object-stdin pack-objects basic-stdin actual &&\n+\ttest_line_count = 1 actual &&\n+\n+\tgit -C pack-object-stdin pack-objects --revs basic-stdin-revs actual &&\n+\ttest_line_count = 3 actual\n+'\n+\n+test_expect_success 'pack-object in <<-EOF &&\n+\t$(git -C pack-object-stdin rev-parse one)\n+\tgarbage\n+\t$(git -C pack-object-stdin rev-parse two)\n+\tEOF\n+\n+\tsed \"s/^> //g\" >err.expect <<-EOF &&\n+\tfatal: expected object ID, got garbage:\n+\t> garbage\n+\n+\tEOF\n+\ttest_must_fail git -C pack-object-stdin pack-objects bad-line-stdin err.actual &&\n+\ttest_cmp err.expect err.actual &&\n+\n+\tcat >err.expect <<-EOF &&\n+\tfatal: bad revision '\"'\"'garbage'\"'\"'\n+\tEOF\n+\ttest_must_fail git -C pack-object-stdin pack-objects --revs bad-line-stdin-revs err.actual &&\n+\ttest_cmp err.expect err.actual\n+'\n+\n+test_expect_success 'pack-object in <<-EOF &&\n+\t$(git -C pack-object-stdin rev-parse one)\n+\n+\t$(git -C pack-object-stdin rev-parse two)\n+\tEOF\n+\n+\tsed -e \"s/^> //g\" -e \"s/Z$//g\" >err.expect <<-EOF &&\n+\tfatal: expected object ID, got garbage:\n+\t> Z\n+\n+\tEOF\n+\ttest_must_fail git -C pack-object-stdin pack-objects empty-line-stdin err.actual &&\n+\ttest_cmp err.expect err.actual &&\n+\n+\tgit -C pack-object-stdin pack-objects --revs empty-line-stdin-revs actual &&\n+\ttest_line_count = 3 actual\n+'\n+\n+test_expect_success 'pack-object in <<-EOF &&\n+\t$(git -C pack-object-stdin rev-parse one)\n+\t$(git -C pack-object-stdin rev-parse two)\n+\tEOF\n+\n+\t# There is the \"--stdin-packs is incompatible with --revs\"\n+\t# test below, but we should make sure that the revision.c\n+\t# --stdin is not picked up\n+\tcat >err.expect <<-EOF &&\n+\tfatal: disallowed abbreviated or ambiguous option '\"'\"'stdin'\"'\"'\n+\tEOF\n+\ttest_must_fail git -C pack-object-stdin pack-objects stdin-with-stdin-option --stdin err.actual &&\n+\ttest_cmp err.expect err.actual &&\n+\n+\ttest_must_fail git -C pack-object-stdin pack-objects --stdin --revs stdin-with-stdin-option-revs 2>err.actual \n # e.g.: check_deltas stderr -gt 0\n check_deltas() {\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429532","messageId":"patch-2.2-c7315f2b378-20210709T101015Z-avarab@gmail.com","threadId":"55976","inReplyTo":"cover-0.2-00000000000-20210709T101014Z-avarab@gmail.com","subject":"[PATCH v2 2/2] pack-objects: fix segfault in --stdin-packs option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-09T10:13:48Z","receivedAt":"2021-07-09T10:16:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a segfault in the --stdin-packs option added in\n339bce27f4f (builtin/pack-objects.c: add '--stdin-packs' option,\n2021-02-22).\n\nThe read_packs_list_from_stdin() function didn't check that the lines\nit was reading were valid packs, and thus when doing the QSORT() with\npack_mtime_cmp() we'd have a NULL \"util\" field. The \"util\" field is\nused to associate the names of included/excluded packs with the\npacked_git structs they correspond to.\n\nThe logic error was in assuming that we could iterate all packs and\nannotate the excluded and included packs we got, as opposed to\nchecking the lines we got on stdin. There was a check for excluded\npacks, but included packs were simply assumed to be valid.\n\nAs noted in the test we'll not report the first bad line, but whatever\nline sorted first according to the string-list.c API. In this case I\nthink that's fine. There was further discussion of alternate\napproaches in [1].\n\nEven though we're being lazy let's assert the line we do expect to get\nin the test, since whoever changes this code in the future might miss\nthis case, and would want to update the test and comments.\n\n1. http://lore.kernel.org/git/YND3h2l10PlnSNGJ@nand.local\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 23 ++++++++++++++++++++---\n t/t5300-pack-object.sh | 19 +++++++++++++++++++\n 2 files changed, 39 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex de00adbb9e0..df49f656b96 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3311,9 +3311,26 @@ static void read_packs_list_from_stdin(void)\n \t}\n \n \t/*\n-\t * First handle all of the excluded packs, marking them as kept in-core\n-\t * so that later calls to add_object_entry() discards any objects that\n-\t * are also found in excluded packs.\n+\t * Arguments we got on stdin may not even be packs. First\n+\t * check that to avoid segfaulting later on in\n+\t * e.g. pack_mtime_cmp(), excluded packs are handled below.\n+\t *\n+\t * Since we first parsed our STDIN and then sorted the input\n+\t * lines the pack we error on will be whatever line happens to\n+\t * sort first. This is lazy, it's enough that we report one\n+\t * bad case here, we don't need to report the first/last one,\n+\t * or all of them.\n+\t */\n+\tfor_each_string_list_item(item, &include_packs) {\n+\t\tstruct packed_git *p = item->util;\n+\t\tif (!p)\n+\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n+\t}\n+\n+\t/*\n+\t * Then, handle all of the excluded packs, marking them as\n+\t * kept in-core so that later calls to add_object_entry()\n+\t * discards any objects that are also found in excluded packs.\n \t */\n \tfor_each_string_list_item(item, &exclude_packs) {\n \t\tstruct packed_git *p = item->util;\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 65e991e3706..e13a8842075 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -119,6 +119,25 @@ test_expect_success 'pack-object in <<-EOF &&\n+\t$(git -C pack-object-stdin rev-parse one)\n+\t$(git -C pack-object-stdin rev-parse two)\n+\tEOF\n+\n+\t# That we get \"two\" and not \"one\" has to do with OID\n+\t# ordering. It happens to be the same here under SHA-1 and\n+\t# SHA-256. See commentary in pack-objects.c\n+\tcat >err.expect <<-EOF &&\n+\tfatal: could not find pack '\"'\"'$(git -C pack-object-stdin rev-parse two)'\"'\"'\n+\tEOF\n+\ttest_must_fail git \\\n+\t\t-C pack-object-stdin \\\n+\t\tpack-objects stdin-with-stdin-option --stdin-packs \\\n+\t\terr.actual &&\n+\ttest_cmp err.expect err.actual\n+'\n+\n # usage: check_deltas \n # e.g.: check_deltas stderr -gt 0\n check_deltas() {\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"430557","messageId":"YPXvQXOcdKdCzcFb@nand.local","threadId":"55976","inReplyTo":"patch-2.2-c7315f2b378-20210709T101015Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/2] pack-objects: fix segfault in --stdin-packs option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-19T21:31:45Z","receivedAt":"2021-07-19T23:11:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jul 09, 2021 at 12:13:48PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\nThanks for the update, and sorry that it took me so long to get to. I\nsee that this still hasn't quite made its way to 'next', so I'll just\nadd one comment.\n\n> +test_expect_success 'pack-object +\tcat >in <<-EOF &&\n> +\t$(git -C pack-object-stdin rev-parse one)\n> +\t$(git -C pack-object-stdin rev-parse two)\n> +\tEOF\n\nI see that you left my suggestion to inline this here-doc with the\nactual 'pack-objects' invocation below alone, which is fine. I think\nthat it does help the readability, too, since it separates the input\nfrom the command its being fed to.\n\n> +\t# That we get \"two\" and not \"one\" has to do with OID\n> +\t# ordering. It happens to be the same here under SHA-1 and\n> +\t# SHA-256. See commentary in pack-objects.c\n> +\tcat >err.expect <<-EOF &&\n> +\tfatal: could not find pack '\"'\"'$(git -C pack-object-stdin rev-parse two)'\"'\"'\n> +\tEOF\n\nOn the other hand, crafting this err.expect with one of the object's\nfull OID still sits funny with me. I appreciate you checking that this\nis the correct object to test with in SHA-1 and SHA-256 mode, but isn't\nthe point that we shouldn't be relying on which object comes out?\n\nI think that dropping this down to just something like:\n\n grep 'could not find' err.actual\n\nwould be an improvement since it avoids the finicky shell quoting,\nhardens this test in the event of a future change in hashing algorithm,\nand brings the test more in line with the spirit of the patch itself\n(which is to report some of its input, not necessarily the first one\ngiven).\n\n\nThanks,\nTaylor\n"},{"id":"430624","messageId":"87zguhum6y.fsf@evledraar.gmail.com","threadId":"55976","inReplyTo":"YPXvQXOcdKdCzcFb@nand.local","subject":"Re: [PATCH v2 2/2] pack-objects: fix segfault in --stdin-packs option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T11:55:31Z","receivedAt":"2021-07-20T13:57:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jul 19 2021, Taylor Blau wrote:\n\n> On Fri, Jul 09, 2021 at 12:13:48PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> Thanks for the update, and sorry that it took me so long to get to. I\n> see that this still hasn't quite made its way to 'next', so I'll just\n> add one comment.\n>\n>> +test_expect_success 'pack-object > +\tcat >in <<-EOF &&\n>> +\t$(git -C pack-object-stdin rev-parse one)\n>> +\t$(git -C pack-object-stdin rev-parse two)\n>> +\tEOF\n>\n> I see that you left my suggestion to inline this here-doc with the\n> actual 'pack-objects' invocation below alone, which is fine. I think\n> that it does help the readability, too, since it separates the input\n> from the command its being fed to.\n\nYeah, per CL:\n \n I didn't end up moving away from the \"> +\t# That we get \"two\" and not \"one\" has to do with OID\n>> +\t# ordering. It happens to be the same here under SHA-1 and\n>> +\t# SHA-256. See commentary in pack-objects.c\n>> +\tcat >err.expect <<-EOF &&\n>> +\tfatal: could not find pack '\"'\"'$(git -C pack-object-stdin rev-parse two)'\"'\"'\n>> +\tEOF\n>\n> On the other hand, crafting this err.expect with one of the object's\n> full OID still sits funny with me. I appreciate you checking that this\n> is the correct object to test with in SHA-1 and SHA-256 mode, but isn't\n> the point that we shouldn't be relying on which object comes out?\n>\n> I think that dropping this down to just something like:\n>\n> grep 'could not find' err.actual\n>\n> would be an improvement since it avoids the finicky shell quoting,\n> hardens this test in the event of a future change in hashing algorithm,\n> and brings the test more in line with the spirit of the patch itself\n> (which is to report some of its input, not necessarily the first one\n> given).\n\nIf we've got another hash transition (unlikely, at least near-ish term)\nwe can just look at this test again.\n\nMore plausibly it's a common pattern in our test suite that greps like\nthat elide actual problems, e.g. a loop printing the error N times, that\nseems more likely in this case.\n\nDo you mind if it's just left as it is?\n"},{"id":"430639","messageId":"YPcA0oxJgedIf57K@nand.local","threadId":"55976","inReplyTo":"87zguhum6y.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/2] pack-objects: fix segfault in --stdin-packs option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-20T16:58:58Z","receivedAt":"2021-07-20T16:59:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 20, 2021 at 01:55:31PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> On Mon, Jul 19 2021, Taylor Blau wrote:\n> > On Fri, Jul 09, 2021 at 12:13:48PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> >> +test_expect_success 'pack-object >> +\tcat >in <<-EOF &&\n> >> +\t$(git -C pack-object-stdin rev-parse one)\n> >> +\t$(git -C pack-object-stdin rev-parse two)\n> >> +\tEOF\n> >\n> > I see that you left my suggestion to inline this here-doc with the\n> > actual 'pack-objects' invocation below alone, which is fine. I think\n> > that it does help the readability, too, since it separates the input\n> > from the command its being fed to.\n>\n> Yeah, per CL:\n>\n> I didn't end up moving away from the \" because it makes manual inspection easier, and the tests above this\n> one used it consistently, so I left it in place.\n\nAh, my apologies for reading right past it. Thanks!\n\n> >> +\t# That we get \"two\" and not \"one\" has to do with OID\n> >> +\t# ordering. It happens to be the same here under SHA-1 and\n> >> +\t# SHA-256. See commentary in pack-objects.c\n> >> +\tcat >err.expect <<-EOF &&\n> >> +\tfatal: could not find pack '\"'\"'$(git -C pack-object-stdin rev-parse two)'\"'\"'\n> >> +\tEOF\n> >\n> > On the other hand, crafting this err.expect with one of the object's\n> > full OID still sits funny with me. I appreciate you checking that this\n> > is the correct object to test with in SHA-1 and SHA-256 mode, but isn't\n> > the point that we shouldn't be relying on which object comes out?\n> >\n> > I think that dropping this down to just something like:\n> >\n> > grep 'could not find' err.actual\n> >\n> > would be an improvement since it avoids the finicky shell quoting,\n> > hardens this test in the event of a future change in hashing algorithm,\n> > and brings the test more in line with the spirit of the patch itself\n> > (which is to report some of its input, not necessarily the first one\n> > given).\n>\n> If we've got another hash transition (unlikely, at least near-ish term)\n> we can just look at this test again.\n>\n> More plausibly it's a common pattern in our test suite that greps like\n> that elide actual problems, e.g. a loop printing the error N times, that\n> seems more likely in this case.\n\nI understand the reasoning, but I'm not quite sure that I buy that that\nis a likely defect here. Of course, that doesn't mean that stricter\ntests aren't good for nothing, but it's a tradeoff.\n\n> Do you mind if it's just left as it is?\n\nNo, I don't mind, but I do think that changing the test to be looser\nwould be an improvement. Of course, there are lots of unlikely/rare\nthings that could cause this test to break (like picking a different\nhashing algorithm), but I think fundamentally this test disagrees in\nspirit with the code.\n\nI.e., it seems odd to me that the code says \"well, we have to sort this\nlist before we can produce an error, so the error won't necessarily\ncorrespond to your first line of input\" but the test says \"we should\ncheck that exactly such-and-such an object is included in the error\".\n\nObviously that is far from the worst outcome in the world, but it does\nseem a little odd to me. In any case, I don't feel particularly strongly\nabout it, and (as I said much earlier in the thread) this probably is\nall academic anyway, since I would expect the vast majority of uses of\n'--stdin-packs' are from 'git repack --geometric' anyway.\n\nThanks,\nTaylor\n"}]}