[PATCH] unpack_entry: do not die when we fail to apply a delta

[PATCH] unpack_entry: do not die when we fail to apply a delta

[Jeff King]

When we try to load an object from disk and fail, our
general strategy is to see if we can get it from somewhere
else (e.g., a loose object). That lets users fix corruption
problems by copying known-good versions of objects into the
object database.

We already handle the case where we were not able to read
the delta from disk. However, when we find that the delta we
read does not apply, we simply die.  This case is harder to
trigger, as corruption in the delta data itself would
trigger a crc error from zlib.  However, a corruption that
pointed us at the wrong delta base might cause it.

We can do the same "fail and try to find the object
elsewhere" trick instead of dying. This not only gives us a
chance to recover, but also puts us on code paths that will
alert the user to the problem (with the current message,
they do not even know which sha1 caused the problem).

Signed-off-by: Jeff King <peff@peff.net>
---
I needed this earlier today to recover from a corrupted packfile (I
fortunately had an older version of the repo in backups). Still tracking
down the exact nature of the corruption.

 sha1_file.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index 5c08701..d458708 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2135,8 +2135,17 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
 		data = patch_delta(base, base_size,
 				   delta_data, delta_size,
 				   &size);
+
+		/*
+		 * We could not apply the delta; warn the user, but keep going.
+		 * Our failure will be noticed either in the next iteration of
+		 * the loop, or if this is the final delta, in the caller when
+		 * we return NULL. Those code paths will take care of making
+		 * a more explicit warning and retrying with another copy of
+		 * the object.
+		 */
 		if (!data)
-			die("failed to apply delta");
+			error("failed to apply delta");
 
 		free(delta_data);
 	}
-- 
1.8.3.rc2.14.g7eee6b3

Re: [PATCH] unpack_entry: do not die when we fail to apply a delta

[Nicolas Pitre]

On Thu, 13 Jun 2013, Jeff King wrote:

> When we try to load an object from disk and fail, our
> general strategy is to see if we can get it from somewhere
> else (e.g., a loose object). That lets users fix corruption
> problems by copying known-good versions of objects into the
> object database.
> 
> We already handle the case where we were not able to read
> the delta from disk. However, when we find that the delta we
> read does not apply, we simply die.  This case is harder to
> trigger, as corruption in the delta data itself would
> trigger a crc error from zlib.  However, a corruption that
> pointed us at the wrong delta base might cause it.
> 
> We can do the same "fail and try to find the object
> elsewhere" trick instead of dying. This not only gives us a
> chance to recover, but also puts us on code paths that will
> alert the user to the problem (with the current message,
> they do not even know which sha1 caused the problem).
> 
> Signed-off-by: Jeff King <peff@peff.net>

That makes sense.

Could you produce a test case to go along with this change?

> ---
> I needed this earlier today to recover from a corrupted packfile (I
> fortunately had an older version of the repo in backups). Still tracking
> down the exact nature of the corruption.
> 
>  sha1_file.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/sha1_file.c b/sha1_file.c
> index 5c08701..d458708 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2135,8 +2135,17 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
>  		data = patch_delta(base, base_size,
>  				   delta_data, delta_size,
>  				   &size);
> +
> +		/*
> +		 * We could not apply the delta; warn the user, but keep going.
> +		 * Our failure will be noticed either in the next iteration of
> +		 * the loop, or if this is the final delta, in the caller when
> +		 * we return NULL. Those code paths will take care of making
> +		 * a more explicit warning and retrying with another copy of
> +		 * the object.
> +		 */
>  		if (!data)
> -			die("failed to apply delta");
> +			error("failed to apply delta");
>  
>  		free(delta_data);
>  	}
> -- 
> 1.8.3.rc2.14.g7eee6b3
> 

[PATCH 0/2] recover from "failed to apply delta"

[Jeff King]

On Thu, Jun 13, 2013 at 08:05:21PM -0400, Nicolas Pitre wrote:

> > We already handle the case where we were not able to read
> > the delta from disk. However, when we find that the delta we
> > read does not apply, we simply die.  This case is harder to
> > trigger, as corruption in the delta data itself would
> > trigger a crc error from zlib.  However, a corruption that
> > pointed us at the wrong delta base might cause it.
>
> That makes sense.
> 
> Could you produce a test case to go along with this change?

Yes. I was a little worried I would have trouble doing it without
relying on a lot of pack internals, but the infrastructure you set up in
t5303 makes it relatively easy (and we do not have to make any
assumptions that t5303 does not already make).

Here is a re-roll; the first patch is a small cleanup in t5303 that is
required for the new tests to work.

  [1/2]: t5303: drop "count=1" from corruption dd
  [2/2]: unpack_entry: do not die when we fail to apply a delta

-Peff

[PATCH 1/2] t5303: drop "count=1" from corruption dd

[Jeff King]

This test corrupts pack objects by using "dd" with a seek
command. It passes "count=1 bs=1" to munge just a single
byte. However, the test added in commit b3118bdc wants to
munge two bytes, and the second byte of corruption is
silently ignored.

This turned out not to impact the test, however. The idea
was to reduce the "size of this entry" part of the header so
that zlib runs out of input bytes while inflating the entry.
That header is two bytes long, and the test reduced the
value of both bytes; since we experience the problem if we
are off by even 1 byte, it is sufficient to munge only the
first one.

Even though the test would have worked with only a single
byte munged, and we could simply tweak the test to use a
single byte, it makes sense to lift this 1-byte restriction
from do_corrupt_object. It will allow future tests that do
need to change multiple bytes to do so.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5303-pack-corruption-resilience.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 5b1250f..9cb8172 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -51,7 +51,7 @@ do_corrupt_object() {
     ofs=`git show-index < ${pack}.idx | grep $1 | cut -f1 -d" "` &&
     ofs=$(($ofs + $2)) &&
     chmod +w ${pack}.pack &&
-    dd of=${pack}.pack count=1 bs=1 conv=notrunc seek=$ofs &&
+    dd of=${pack}.pack bs=1 conv=notrunc seek=$ofs &&
     test_must_fail git verify-pack ${pack}.pack
 }
 
-- 
1.8.3.rc2.14.g7eee6b3

Re: [PATCH 0/2] recover from "failed to apply delta"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Jun 13, 2013 at 08:05:21PM -0400, Nicolas Pitre wrote:
>
>> > We already handle the case where we were not able to read
>> > the delta from disk. However, when we find that the delta we
>> > read does not apply, we simply die.  This case is harder to
>> > trigger, as corruption in the delta data itself would
>> > trigger a crc error from zlib.  However, a corruption that
>> > pointed us at the wrong delta base might cause it.
>>
>> That makes sense.
>> 
>> Could you produce a test case to go along with this change?
>
> Yes. I was a little worried I would have trouble doing it without
> relying on a lot of pack internals, but the infrastructure you set up in
> t5303 makes it relatively easy (and we do not have to make any
> assumptions that t5303 does not already make).
>
> Here is a re-roll; the first patch is a small cleanup in t5303 that is
> required for the new tests to work.

Heh, I was doing the same, but I cheated ;-)

diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 5b1250f..57436db 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -51,7 +51,7 @@ do_corrupt_object() {
     ofs=`git show-index < ${pack}.idx | grep $1 | cut -f1 -d" "` &&
     ofs=$(($ofs + $2)) &&
     chmod +w ${pack}.pack &&
-    dd of=${pack}.pack count=1 bs=1 conv=notrunc seek=$ofs &&
+    dd of=${pack}.pack count=${3-1} bs=1 conv=notrunc seek=$ofs &&
     test_must_fail git verify-pack ${pack}.pack
 }
 
@@ -276,6 +276,30 @@ test_expect_success \
      git cat-file blob $blob_3 > /dev/null'
 
 test_expect_success \
+    'corrupt delta-part of a packed object, fall back to loose' \
+    'create_new_pack &&
+     path=$(echo "$blob_3" | sed -e "s|^\(..\)|\1/|") &&
+     cat ".git/objects/$path" >saved &&
+     git prune-packed &&
+
+     dd if=${pack}.idx bs=1 count=20 skip=1032 >blob1-bin &&
+     dd if=${pack}.pack bs=1 count=20 skip=2233 >blob3-delta-base-bin &&
+
+     # At the beginning of the REF_DELTA representation of $blob_3,
+     # write 20-byte base object name for $blob_1, instead of $blob_2.
+     # The binary representation of object name for $blob_1 is found
+     # at offset 4 + 4 + 256*4 = 1032 for 20 bytes.
+     dd if=${pack}.idx bs=1 count=20 skip=1032 | do_corrupt_object $blob_3 2 20 &&
+     test_must_fail git cat-file blob $blob_3 >/dev/null &&
+
+     # Resurrect the loose object for $blob_3
+     mkdir -p .git/objects/$(echo "$path" | sed -e "s|^\(..\).*|\1|") &&
+     cat saved >".git/objects/$path" &&
+
+     git cat-file blob $blob_3 >/dev/null
+'
+
+test_expect_success \
     'corrupting header to have too small output buffer fails unpack' \
     'create_new_pack &&
      git prune-packed &&

Re: [PATCH 0/2] recover from "failed to apply delta"

[Jeff King]

On Fri, Jun 14, 2013 at 02:51:35PM -0700, Junio C Hamano wrote:

> > Here is a re-roll; the first patch is a small cleanup in t5303 that is
> > required for the new tests to work.
> 
> Heh, I was doing the same, but I cheated ;-)
> 
> diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
> index 5b1250f..57436db 100755
> --- a/t/t5303-pack-corruption-resilience.sh
> +++ b/t/t5303-pack-corruption-resilience.sh
> @@ -51,7 +51,7 @@ do_corrupt_object() {
>      ofs=`git show-index < ${pack}.idx | grep $1 | cut -f1 -d" "` &&
>      ofs=$(($ofs + $2)) &&
>      chmod +w ${pack}.pack &&
> -    dd of=${pack}.pack count=1 bs=1 conv=notrunc seek=$ofs &&
> +    dd of=${pack}.pack count=${3-1} bs=1 conv=notrunc seek=$ofs &&
>      test_must_fail git verify-pack ${pack}.pack

Yeah, I almost did that, but then I realized that dd will simply read
all of its input, anyway.

>  test_expect_success \
> +    'corrupt delta-part of a packed object, fall back to loose' \
> +    'create_new_pack &&
> +     path=$(echo "$blob_3" | sed -e "s|^\(..\)|\1/|") &&
> +     cat ".git/objects/$path" >saved &&
> +     git prune-packed &&
> +
> +     dd if=${pack}.idx bs=1 count=20 skip=1032 >blob1-bin &&
> +     dd if=${pack}.pack bs=1 count=20 skip=2233 >blob3-delta-base-bin &&
> +
> +     # At the beginning of the REF_DELTA representation of $blob_3,
> +     # write 20-byte base object name for $blob_1, instead of $blob_2.
> +     # The binary representation of object name for $blob_1 is found
> +     # at offset 4 + 4 + 256*4 = 1032 for 20 bytes.
> +     dd if=${pack}.idx bs=1 count=20 skip=1032 | do_corrupt_object $blob_3 2 20 &&
> +     test_must_fail git cat-file blob $blob_3 >/dev/null &&

I didn't want to bother coming up with the binary version of the
REF_DELTA sha1, so I used OFS_DELTA. :)

-Peff

Re: [PATCH 0/2] recover from "failed to apply delta"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +     # At the beginning of the REF_DELTA representation of $blob_3,
>> +     # write 20-byte base object name for $blob_1, instead of $blob_2.
>> +     # The binary representation of object name for $blob_1 is found
>> +     # at offset 4 + 4 + 256*4 = 1032 for 20 bytes.
>> +     dd if=${pack}.idx bs=1 count=20 skip=1032 | do_corrupt_object $blob_3 2 20 &&
>> +     test_must_fail git cat-file blob $blob_3 >/dev/null &&
>
> I didn't want to bother coming up with the binary version of the
> REF_DELTA sha1, so I used OFS_DELTA. :)

Yeah, I contemplated on doing something like this

	printf "$(echo $blob_3 | sed -e 's/\(..\)/\\x\1/g')"

but of course printf "\xAA" is not in POSIX and at that point I
punted and instead read from .idx at the known offset ;-)

[PATCH 2/2] unpack_entry: do not die when we fail to apply a delta

[Jeff King]

When we try to load an object from disk and fail, our
general strategy is to see if we can get it from somewhere
else (e.g., a loose object). That lets users fix corruption
problems by copying known-good versions of objects into the
object database.

We already handle the case where we were not able to read
the delta from disk. However, when we find that the delta we
read does not apply, we simply die.  This case is harder to
trigger, as corruption in the delta data itself would
trigger a crc error from zlib.  However, a corruption that
pointed us at the wrong delta base might cause it.

We can do the same "fail and try to find the object
elsewhere" trick instead of dying. This not only gives us a
chance to recover, but also puts us on code paths that will
alert the user to the problem (with the current message,
they do not even know which sha1 caused the problem).

Note that unlike some other pack corruptions, we do not
recover automatically from this case when doing a repack.
There is nothing apparently wrong with the delta, as it
points to a valid, accessible object, and we realize the
error only when the resulting size does not match up. And in
theory, one could even have a case where the corrupted size
is the same, and the problem would only be noticed by
recomputing the sha1.

We can get around this by recomputing the deltas with
--no-reuse-delta, which our test does (and this is probably
good advice for anyone recovering from pack corruption).

Signed-off-by: Jeff King <peff@peff.net>
---
I don't know if it is worth showing that pack-objects without
"--no-reuse-delta" does not help this case with a test_expect_failure. I
don't have plans to work on it, and I'm not sure that it is a big deal.

 sha1_file.c                           | 11 ++++++++++-
 t/t5303-pack-corruption-resilience.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index 5c08701..d458708 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2135,8 +2135,17 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
 		data = patch_delta(base, base_size,
 				   delta_data, delta_size,
 				   &size);
+
+		/*
+		 * We could not apply the delta; warn the user, but keep going.
+		 * Our failure will be noticed either in the next iteration of
+		 * the loop, or if this is the final delta, in the caller when
+		 * we return NULL. Those code paths will take care of making
+		 * a more explicit warning and retrying with another copy of
+		 * the object.
+		 */
 		if (!data)
-			die("failed to apply delta");
+			error("failed to apply delta");
 
 		free(delta_data);
 	}
diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 9cb8172..35926de 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -276,6 +276,33 @@ test_expect_success \
      git cat-file blob $blob_3 > /dev/null'
 
 test_expect_success \
+    'corruption of delta base reference pointing to wrong object' \
+    'create_new_pack --delta-base-offset &&
+     git prune-packed &&
+     printf "\220\033" | do_corrupt_object $blob_3 2 &&
+     git cat-file blob $blob_1 >/dev/null &&
+     git cat-file blob $blob_2 >/dev/null &&
+     test_must_fail git cat-file blob $blob_3 >/dev/null'
+
+test_expect_success \
+    '... but having a loose copy allows for full recovery' \
+    'mv ${pack}.idx tmp &&
+     git hash-object -t blob -w file_3 &&
+     mv tmp ${pack}.idx &&
+     git cat-file blob $blob_1 > /dev/null &&
+     git cat-file blob $blob_2 > /dev/null &&
+     git cat-file blob $blob_3 > /dev/null'
+
+test_expect_success \
+    '... and then a repack "clears" the corruption' \
+    'do_repack --delta-base-offset --no-reuse-delta &&
+     git prune-packed &&
+     git verify-pack ${pack}.pack &&
+     git cat-file blob $blob_1 > /dev/null &&
+     git cat-file blob $blob_2 > /dev/null &&
+     git cat-file blob $blob_3 > /dev/null'
+
+test_expect_success \
     'corrupting header to have too small output buffer fails unpack' \
     'create_new_pack &&
      git prune-packed &&
-- 
1.8.3.rc2.14.g7eee6b3

Re: [PATCH 2/2] unpack_entry: do not die when we fail to apply a delta

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>  test_expect_success \
> +    'corruption of delta base reference pointing to wrong object' \
> +    'create_new_pack --delta-base-offset &&
> +     git prune-packed &&
> +     printf "\220\033" | do_corrupt_object $blob_3 2 &&

Interesting.  You cheated in a different way with a hardcoded
offset, instead of hardcoded knowledge of where the object name
is stored in binary in the .idx file ;-)

> +     git cat-file blob $blob_1 >/dev/null &&
> +     git cat-file blob $blob_2 >/dev/null &&
> +     test_must_fail git cat-file blob $blob_3 >/dev/null'
> +
> +test_expect_success \
> +    '... but having a loose copy allows for full recovery' \
> +    'mv ${pack}.idx tmp &&
> +     git hash-object -t blob -w file_3 &&
> +     mv tmp ${pack}.idx &&
> +     git cat-file blob $blob_1 > /dev/null &&
> +     git cat-file blob $blob_2 > /dev/null &&
> +     git cat-file blob $blob_3 > /dev/null'
> +
> +test_expect_success \
> +    '... and then a repack "clears" the corruption' \
> +    'do_repack --delta-base-offset --no-reuse-delta &&
> +     git prune-packed &&
> +     git verify-pack ${pack}.pack &&
> +     git cat-file blob $blob_1 > /dev/null &&
> +     git cat-file blob $blob_2 > /dev/null &&
> +     git cat-file blob $blob_3 > /dev/null'

Nice.  Will replace the one I queued yesterday with these two patches.

> +test_expect_success \
>      'corrupting header to have too small output buffer fails unpack' \
>      'create_new_pack &&
>       git prune-packed &&

Re: [PATCH 2/2] unpack_entry: do not die when we fail to apply a delta

[Jeff King]

On Fri, Jun 14, 2013 at 02:59:00PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >  test_expect_success \
> > +    'corruption of delta base reference pointing to wrong object' \
> > +    'create_new_pack --delta-base-offset &&
> > +     git prune-packed &&
> > +     printf "\220\033" | do_corrupt_object $blob_3 2 &&
> 
> Interesting.  You cheated in a different way with a hardcoded
> offset, instead of hardcoded knowledge of where the object name
> is stored in binary in the .idx file ;-)

Yes. We could get it with:

  git show-index <"$pack.idx" |
  cut -d' ' -f1 |
  perl -e '
    @pos = map { chomp; $_ } <>;
    my $ofs = $pos[2] - $pos[0];

    my @bin;
    unshift @bin, $ofs & 127;
    while ($ofs >>= 7) {
      $ofs--;
      unshift @bin, 128 | ($ofs & 127);
    }

    binmode STDOUT;
    print chr for @bin;
  '

if that's not too ugly. Maybe the REF_DELTA one is less ugly, then, as
it would not need to do the packed offset encoding, but just convert
$blob1 from hex into binary.

-Peff

Re: [PATCH] unpack_entry: do not die when we fail to apply a delta

[Junio C Hamano]

Makes sense.  Thanks, will queue.