Re: [BUG] commit walk machinery is dangerous !

[Junio C Hamano <gitster@pobox.com>]

Nicolas Pitre <nico@cam.org> writes:

> It is those with semantic meaning (e.g. object doesn't exist) which 
> should be audited, especially if used in the context of repository 
> modification, which pretty much limits it to the test case I produced.

I've been wondering if we should make the change 8eca0b4 (implement some
resilience against pack corruptions, 2008-06-23) less aggressive.

It makes loose objects and data from other packs to be used as fallback
where we used to just punt, which is a genuine improvement for "salvaging"
mode of operation, but at the same time, it now forbids the callers to
expect that the objects they learned to exist from has_sha1_file() or
nth_packed_object_sha1() should never result NULL return value from
read_sha1_file().

It may make it safe again to fail if you cannot salvage using fallback
method after all.  Something like the attached.

This is unrelated to the issue at hand, but I also notice that there are
few callsites outside sha1_file.c that bypasses cache_or_unpack_entry()
and call unpack_entry() directly.  I wonder if they should be using the
cached version, making unpack_entry() static...

 sha1_file.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 2df78b5..55aa361 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1649,7 +1649,7 @@ static void *unpack_delta_entry(struct packed_git *p,
 		mark_bad_packed_object(p, base_sha1);
 		base = read_sha1_file(base_sha1, type, &base_size);
 		if (!base)
-			return NULL;
+			exit(129);
 	}
 
 	delta_data = unpack_compressed_entry(p, w_curs, curpos, delta_size);
@@ -1946,6 +1946,8 @@ static void *read_packed_sha1(const unsigned char *sha1,
 		      sha1_to_hex(sha1), (uintmax_t)e.offset, e.p->pack_name);
 		mark_bad_packed_object(e.p, sha1);
 		data = read_sha1_file(sha1, type, size);
+		if (!data)
+			exit(129);
 	}
 	return data;
 }