When re-rolling an unrelated series[1] dealing with pack-objects.c and revision.c I discovered that we have some test blindspots, and that the newly added --stdin-packs option in v2.32.0 will segfault if fed garbage data. See http://lore.kernel.org/git/cover-0.2-00000000000-20210621T145819Z-avarab@xxxxxxxxx for the v1 of this series. This hopefully addresses Taylor's comments in https://lore.kernel.org/git/YND3h2l10PlnSNGJ@nand.local/ I didn't end up moving away from the "<in" pattern. I prefer it because it makes manual inspection easier, and the tests above this one used it consistently, so I left it in place. I also ended up keeping the test_cmp for the reasons noted in the updated 2/2 commit message. I.e. if we're commeting on this tricky edge case let's have our tests assert that the code works that way, and that the comment still holds. Ævar Arnfjörð Bjarmason (2): pack-objects tests: cover blindspots in stdin handling pack-objects: fix segfault in --stdin-packs option builtin/pack-objects.c | 23 +++++++-- t/t5300-pack-object.sh | 104 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 3 deletions(-) Range-diff against v1: 1: dd2efd1cfb9 = 1: 8a4d4b820e7 pack-objects tests: cover blindspots in stdin handling 2: a9702132385 ! 2: c7315f2b378 pack-objects: fix segfault in --stdin-packs option @@ Commit message Fix a segfault in the --stdin-packs option added in 339bce27f4f (builtin/pack-objects.c: add '--stdin-packs' option, - 2021-02-22). The read_packs_list_from_stdin() function didn't check - that the lines it was reading were valid packs, and thus when doing - the QSORT() with pack_mtime_cmp() we'd have a NULL "util" field. + 2021-02-22). + + The read_packs_list_from_stdin() function didn't check that the lines + it was reading were valid packs, and thus when doing the QSORT() with + pack_mtime_cmp() we'd have a NULL "util" field. The "util" field is + used to associate the names of included/excluded packs with the + packed_git structs they correspond to. The logic error was in assuming that we could iterate all packs and annotate the excluded and included packs we got, as opposed to @@ Commit message As noted in the test we'll not report the first bad line, but whatever line sorted first according to the string-list.c API. In this case I - think that's fine. + think that's fine. There was further discussion of alternate + approaches in [1]. + + Even though we're being lazy let's assert the line we do expect to get + in the test, since whoever changes this code in the future might miss + this case, and would want to update the test and comments. + + 1. http://lore.kernel.org/git/YND3h2l10PlnSNGJ@nand.local Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## builtin/pack-objects.c ## @@ builtin/pack-objects.c: static void read_packs_list_from_stdin(void) - item->util = p; } -+ /* -+ * Arguments we got on stdin may not even be packs. Check that -+ * to avoid segfaulting later on in e.g. pack_mtime_cmp(). + /* +- * First handle all of the excluded packs, marking them as kept in-core +- * so that later calls to add_object_entry() discards any objects that +- * are also found in excluded packs. ++ * Arguments we got on stdin may not even be packs. First ++ * check that to avoid segfaulting later on in ++ * e.g. pack_mtime_cmp(), excluded packs are handled below. ++ * ++ * Since we first parsed our STDIN and then sorted the input ++ * lines the pack we error on will be whatever line happens to ++ * sort first. This is lazy, it's enough that we report one ++ * bad case here, we don't need to report the first/last one, ++ * or all of them. + */ + for_each_string_list_item(item, &include_packs) { + struct packed_git *p = item->util; @@ builtin/pack-objects.c: static void read_packs_list_from_stdin(void) + die(_("could not find pack '%s'"), item->string); + } + - /* - * First handle all of the excluded packs, marking them as kept in-core - * so that later calls to add_object_entry() discards any objects that ++ /* ++ * Then, handle all of the excluded packs, marking them as ++ * kept in-core so that later calls to add_object_entry() ++ * discards any objects that are also found in excluded packs. + */ + for_each_string_list_item(item, &exclude_packs) { + struct packed_git *p = item->util; ## t/t5300-pack-object.sh ## @@ t/t5300-pack-object.sh: test_expect_success 'pack-object <stdin parsing: [|--revs] with --stdin' ' @@ t/t5300-pack-object.sh: test_expect_success 'pack-object <stdin parsing: [|--rev + $(git -C pack-object-stdin rev-parse two) + EOF + -+ # We actually just report the first bad line in strcmp() -+ # order, it just so happens that we get the same result under -+ # SHA-1 and SHA-256 here. It does not really matter that we -+ # report the first bad item in this obscure case, so this -+ # oddity of the test is OK. ++ # That we get "two" and not "one" has to do with OID ++ # ordering. It happens to be the same here under SHA-1 and ++ # SHA-256. See commentary in pack-objects.c + cat >err.expect <<-EOF && + fatal: could not find pack '"'"'$(git -C pack-object-stdin rev-parse two)'"'"' + EOF -+ test_must_fail git -C pack-object-stdin pack-objects stdin-with-stdin-option --stdin-packs <in 2>err.actual && ++ test_must_fail git \ ++ -C pack-object-stdin \ ++ pack-objects stdin-with-stdin-option --stdin-packs \ ++ <in 2>err.actual && + test_cmp err.expect err.actual +' + -- 2.32.0.636.g43e71d69cff
