Junio C Hamano <gitster@xxxxxxxxx> writes:
>> + 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;
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html