Re: [PATCH] t1450: fix quoting of NUL byte when corrupting pack
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 2, 2020, 00:45 UTC
- Message-ID
- <xmqqmu3d27vs.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20200801220611.10453-1-martin.agren@gmail.com>
Martin Ågren <martin.agren@gmail.com> writes:
Show 25 quoted lines
> We use > > printf '\0' > > to generate a NUL byte which we then `dd` into the packfile to ensure > that we modify the first byte of the first object, thereby > (probabilistically) invalidating the checksum. Except the single quotes > we're using are interpreted to match with the ones we enclose the whole > test in. So we actually execute > > printf \0 > > and end up injecting the ASCII code for "0", 0x30, instead. > > The comment right above this `printf` invocation says that "at least one > of [the type bits] is not zero, so setting the first byte to 0 is > sufficient". Substituting "0x30" for "0" in that comment won't do: we'd > need to reason about which bits go where and just what the packfile > looks like that we're modifying in this test. > > Let's avoid all of that by actually executing > > printf "\0" > > to generate a NUL byte, as intended.
Thanks. Very well explained.
I wonder if it is an easy way to find similar problems without too much hand-parsing of the test scripts. Inside a modern test_expect_* that begins and ends the test body with a single quote, any line that has a single quote that is not quoted could be suspect, but that would probably give us too many false positive.
Show 25 quoted lines
> Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- > If my reading is correct, when we substitute 0x30, the type will be 3 > (blob) and the size will be zero. So there might actually exist > formally valid packfiles where this byte that we're modifying is > already zero. What matters in the end is whether we might be using such > a packfile in this exact test and from what I can tell, no, we won't be > doing that. > > t/t1450-fsck.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh > index 344a2aad82..af2a2c4682 100755 > --- a/t/t1450-fsck.sh > +++ b/t/t1450-fsck.sh > @@ -714,7 +714,7 @@ test_expect_success 'fsck fails on corrupt packfile' ' > # at least one of which is not zero, so setting the first byte to 0 is > # sufficient.) > chmod a+w .git/objects/pack/pack-$pack.pack && > - printf '\0' | dd of=.git/objects/pack/pack-$pack.pack bs=1 conv=notrunc seek=12 && > + printf "\0" | dd of=.git/objects/pack/pack-$pack.pack bs=1 conv=notrunc seek=12 && > > test_when_finished "rm -f .git/objects/pack/pack-$pack.*" && > remove_object $hsh &&