mbox series

[0/2] pack-objects: missing tests & --stdin-packs segfault fix

Message ID cover-0.2-00000000000-20210621T145819Z-avarab@gmail.com (mailing list archive)
Headers show
Series pack-objects: missing tests & --stdin-packs segfault fix | expand

Message

Ævar Arnfjörð Bjarmason June 21, 2021, 3:03 p.m. UTC
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.

This fixes the test blindspots, and 2/2 fixes the segfault.

As discussed in its commit message I'm being lazy about emitting the
error message. If you supply N bogus lines on stdin we'll error on the
first one, since the input is first sorted by the string-list.c
API. The test case for the error message relies on which of two SHA
lines sorts first, and I picked input that happens to sort the same
way under both SHA-1 and SHA-256.

Lazy, but I figured for this use-case it wasn't worth keeping track of
what line we saw when, or to refactor the parsing check on pack names
as we get input lines.

1. https://lore.kernel.org/git/cover-0.4-0000000000-20210617T105537Z-avarab@gmail.com/

Æ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 |  10 ++++
 t/t5300-pack-object.sh | 103 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+)

Comments

Taylor Blau June 21, 2021, 8:34 p.m. UTC | #1
On Mon, Jun 21, 2021 at 05:03:36PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 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.
>
> This fixes the test blindspots, and 2/2 fixes the segfault.

Thanks. I took a close look at the second patch, and it looks good to me
with a few minor comments. The first patch looks good as well, although
I didn't look as closely.

> As discussed in its commit message I'm being lazy about emitting the
> error message. If you supply N bogus lines on stdin we'll error on the
> first one, since the input is first sorted by the string-list.c
> API. The test case for the error message relies on which of two SHA
> lines sorts first, and I picked input that happens to sort the same
> way under both SHA-1 and SHA-256.
>
> Lazy, but I figured for this use-case it wasn't worth keeping track of
> what line we saw when, or to refactor the parsing check on pack names
> as we get input lines.

Yeah. I think what you wrote is entirely reasonable, too. I suggested
some alternatives if you are feeling motivated to make the error
reporting nicer, but as you say, I think the vast majority of use-cases
don't care about the output.

Thanks,
Taylor