Re: [PATCH 7/5] fsck: use streaming interface for large blobs in pack

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 12, 2016, 19:06 UTC
Message-ID: <xmqqvb0a4qx3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqq7fcq66q6.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 19 quoted lines

>> +		if (type != OBJ_BLOB || size < big_file_threshold) {
>> +			data = unpack_entry(p, entries[i].offset, &type, &size);
>> +			data_valid = 1;
>
> This codepath slurps the data in-core to hash and data is later
> freed, i.e. non-blob objects and small ones are handled as before.
>
>> +		}
>> +
>> +		if (data_valid && !data)
>>  			err = error("cannot unpack %s from %s at offset %"PRIuMAX"",
>>  				    sha1_to_hex(entries[i].sha1), p->pack_name,
>>  				    (uintmax_t)entries[i].offset);
>
> Otherwise, we'd go to check_sha1_signature() with map==NULL.  And
> that is exactly what we want---map==NULL is the way we tell the
> function to use the streaming interface to check.
>
> Good.

But not quite correct yet ;-)

Here is what I'd propose to squash in. Even though data_valid protects the above if() condition from accessing an otherwise uninitialized "data", the call to check_sha1_signature() we have later will get noticed by the compiler that "data" is passed uninitialized.

More importantly, uninitialized data passed may be non-NULL, in which case it would not trigger the streaming interface.

 pack-check.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/pack-check.c b/pack-check.c
index 0777766..ac263fd 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -123,7 +123,14 @@ static int verify_packfile(struct packed_git *p,
 		type = unpack_object_header(p, w_curs, &curpos, &size);
 		unuse_pack(w_curs);
 
-		if (type != OBJ_BLOB || size < big_file_threshold) {
+		if (type == OBJ_BLOB && big_file_threshold <= size) {
+			/*
+			 * Let check_sha1_signature() to check it with
+			 * the streaming interface; no point slurping
+			 * the data in-core only to discard.
+			 */
+			data = NULL;
+		} else {
 			data = unpack_entry(p, entries[i].offset, &type, &size);
 			data_valid = 1;
 		}