@matheusbsbs / matheus-bagliano / commits / 561fa03529

pack-objects: fix segfault in --stdin-packs option

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. 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 checking the lines we got on stdin. There was a check for excluded packs, but included packs were simply assumed to be valid. 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. 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@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Jul 9, 2021 at 12:13 UTC 561fa03529202a36e0d77548fdcb5d81422c1865

2 files changed +39 -3

builtin/pack-objects.c

+20 -3

@@ -3311,9 +3311,26 @@ static void read_packs_list_from_stdin(void)

3311 }

3312

3313 /*

3314 - * First handle all of the excluded packs, marking them as kept in-core

3315 - * so that later calls to add_object_entry() discards any objects that

3316 - * are also found in excluded packs.

3314 + * Arguments we got on stdin may not even be packs. First

3315 + * check that to avoid segfaulting later on in

3316 + * e.g. pack_mtime_cmp(), excluded packs are handled below.

3317 + *

3318 + * Since we first parsed our STDIN and then sorted the input

3319 + * lines the pack we error on will be whatever line happens to

3320 + * sort first. This is lazy, it's enough that we report one

3321 + * bad case here, we don't need to report the first/last one,

3322 + * or all of them.

3323 + */

3324 + for_each_string_list_item(item, &include_packs) {

3325 + struct packed_git *p = item->util;

3326 + if (!p)

3327 + die(_("could not find pack '%s'"), item->string);

3328 + }

3329 +

3330 + /*

3331 + * Then, handle all of the excluded packs, marking them as

3332 + * kept in-core so that later calls to add_object_entry()

3333 + * discards any objects that are also found in excluded packs.

3334 */

3335 for_each_string_list_item(item, &exclude_packs) {

3336 struct packed_git *p = item->util;

t/t5300-pack-object.sh

+19

@@ -119,6 +119,25 @@ test_expect_success 'pack-object <stdin parsing: [|--revs] with --stdin' '

119 test_cmp err.expect err.actual

120 '

121

122 +test_expect_success 'pack-object <stdin parsing: --stdin-packs handles garbage' '

123 + cat >in <<-EOF &&

124 + $(git -C pack-object-stdin rev-parse one)

125 + $(git -C pack-object-stdin rev-parse two)

126 + EOF

127 +

128 + # That we get "two" and not "one" has to do with OID

129 + # ordering. It happens to be the same here under SHA-1 and

130 + # SHA-256. See commentary in pack-objects.c

131 + cat >err.expect <<-EOF &&

132 + fatal: could not find pack '"'"'$(git -C pack-object-stdin rev-parse two)'"'"'

133 + EOF

134 + test_must_fail git \

135 + -C pack-object-stdin \

136 + pack-objects stdin-with-stdin-option --stdin-packs \

137 + <in 2>err.actual &&

138 + test_cmp err.expect err.actual

139 +'

140 +

141 # usage: check_deltas <stderr_from_pack_objects> <cmp_op> <nr_deltas>

142 # e.g.: check_deltas stderr -gt 0

143 check_deltas() {