git/pack-check.c, branch v2.33.6

csum-file: introduce checksum_valid()

2021-06-29T03:36:17Z

Introduce a new function which checks the validity of a file's trailing checksum. This is similar to hashfd_check(), but different since it is intended to be used by callers who aren't writing the same data (like `git index-pack --verify`), but who instead want to validate the integrity of data that they are reading. Rewrite the first of two callers which could benefit from this new function in pack-check.c. Subsequent callers will be added in the following patches. Helped-by: Jeff King Signed-off-by: Jeff King Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano

fsck: correctly compute checksums on idx files larger than 4GB

2020-11-16T21:41:35Z

When checking the trailing checksum hash of a .idx file, we pass the whole buffer (minus the trailing hash) into a single call to the_hash_algo->update_fn(). But we cast it to an "unsigned int". This comes from c4001d92be (Use off_t when we really mean a file offset., 2007-03-06). That commit started storing the index_size variable as an off_t, but our mozilla-sha1 implementation from the time was limited to a smaller size. Presumably the cast was a way of annotating that we expected .idx files to be small, and so we didn't need to loop (as we do for arbitrarily-large .pack files). Though as an aside it was still wrong, because the mozilla function actually took a signed int. These days our hash-update functions are defined to take a size_t, so we can pass the whole buffer in directly. The cast is actually causing a buggy truncation! While we're here, though, let's drop the confusing off_t variable in the first place. We're getting the size not from the filesystem anyway, but from p->index_size, which is a size_t. In fact, we can make the code a bit more readable by dropping our local variable duplicating p->index_size, and instead have one that stores the size of the actual index data, minus the trailing hash. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

compute pack .idx byte offsets using size_t

2020-11-16T21:41:35Z

A pack and its matching .idx file are limited to 2^32 objects, because the pack format contains a 32-bit field to store the number of objects. Hence we use uint32_t in the code. But the byte count of even a .idx file can be much larger than that, because it stores at least a hash and an offset for each object. So using SHA-1, a v2 .idx file will cross the 4GB boundary at 153,391,650 objects. This confuses load_idx(), which computes the minimum size like this: unsigned long min_size = 8 + 4*256 + nr*(hashsz + 4 + 4) + hashsz + hashsz; Even though min_size will be big enough on most 64-bit platforms, the actual arithmetic is done as a uint32_t, resulting in a truncation. We actually exceed that min_size, but then we do: unsigned long max_size = min_size; if (nr) max_size += (nr - 1)*8; to account for the variable-sized table. That computation doesn't overflow quite so low, but with the truncation for min_size, we end up with a max_size that is much smaller than our actual size. So we complain that the idx is invalid, and can't find any of its objects. We can fix this case by casting "nr" to a size_t, which will do the multiplication in 64-bits (assuming you're on a 64-bit platform; this will never work on a 32-bit system since we couldn't map the whole .idx anyway). Likewise, we don't have to worry about further additions, because adding a smaller number to a size_t will convert the other side to a size_t. A few notes: - obviously we could just declare "nr" as a size_t in the first place (and likewise, packed_git.num_objects). But it's conceptually a uint32_t because of the on-disk format, and we correctly treat it that way in other contexts that don't need to compute byte offsets (e.g., iterating over the set of objects should and generally does use a uint32_t). Switching to size_t would make all of those other cases look wrong. - it could be argued that the proper type is off_t to represent the file offset. But in practice the .idx file must fit within memory, because we mmap the whole thing. And the rest of the code (including the idx_size variable we're comparing against) uses size_t. - we'll add the same cast to the max_size arithmetic line. Even though we're adding to a larger type, which will convert our result, the multiplication is still done as a 32-bit value and can itself overflow. I didn't check this with my test case, since it would need an even larger pack (~530M objects), but looking at compiler output shows that it works this way. The standard should agree, but I couldn't find anything explicit in 6.3.1.8 ("usual arithmetic conversions"). The case in load_idx() was the most immediate one that I was able to trigger. After fixing it, looking up actual objects (including the very last one in sha1 order) works in a test repo with 153,725,110 objects. That's because bsearch_hash() works with uint32_t entry indices, and the actual byte access: int cmp = hashcmp(table + mi * stride, sha1); is done with "stride" as a size_t, causing the uint32_t "mi" to be promoted to a size_t. This is the way most code will access the index data. However, I audited all of the other byte-wise accesses of packed_git.index_data, and many of the others are suspect (they are similar to the max_size one, where we are adding to a properly sized offset or directly to a pointer, but the multiplication in the sub-expression can overflow). I didn't trigger any of these in practice, but I believe they're potential problems, and certainly adding in the cast is not going to hurt anything here. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

pack-check: push oid lookup into loop

2020-02-24T20:55:53Z

When we're checking a pack with fsck or verify-pack, we first sort the idx entries by offset, since accessing them in pack order is more efficient. To do so, we loop over them and fill in an array of structs with the offset, object_id, and index position of each, sort the result, and only then do we iterate over the sorted array and process each entry. In order to avoid the memory cost of storing the hash of each object, we just store a pointer into the copy in the mmap'd pack index file. To keep that property even as the rest of the code converted to "struct object_id", commit 9fd750461b (Convert the verify_pack callback to struct object_id, 2017-05-06) introduced a union in order to type-pun the pointer-to-hash into an object_id struct. But we can make this even simpler by observing that the sort operation doesn't need the object id at all! We only need them one at a time while we actually process each entry. So we can just omit the oid from the struct entirely and load it on the fly into a local variable in the second loop. This gets rid of the type-punning, and lets us directly use the more type-safe nth_packed_object_id(), simplifying the code. And as a bonus, it saves 8 bytes of memory per object. Note that this does mean we'll do the offset lookup for each object before the oid lookup. The oid lookup has more safety checks in it (e.g., for looking past p->num_objects) which in theory protected the offset lookup. But since violating those checks was already a BUG() condition (as described in the previous commit), it's not worth worrying about. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

pack-check: convert "internal error" die to a BUG()

2020-02-24T20:55:53Z

If we fail to load the oid from the index of a packfile, we'll die() with an "internal error". But this should never happen: we'd fail here only if the idx needed to be lazily opened (but we've already opened it) or if we asked for an out-of-range index (but we're iterating using the same count that we'd check the range against). A corrupted index might have a bogus count (e.g., too large for its size), but we'd have complained and aborted already when opening the index initially. While we're here, we can add a few details so that if this bug ever _does_ trigger, we'll have a bit more information. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

sha1-file: allow check_object_signature() to handle any repo

2020-01-31T18:45:39Z

Some callers of check_object_signature() can work on arbitrary repositories, but the repo does not get passed to this function. Instead, the_repository is always used internally. To fix possible inconsistencies, allow the function to receive a struct repository and make those callers pass on the repo being handled. Signed-off-by: Matheus Tavares Signed-off-by: Junio C Hamano

pack-check: use given repo's hash_algo at verify_packfile()

2020-01-31T18:45:39Z

At verify_packfile(), use the git_hash_algo from the provided repository instead of the_hash_algo, for consistency. Like the previous patch, this shouldn't bring any behavior changes, since this function is currently only receiving the_repository. Signed-off-by: Matheus Tavares Signed-off-by: Junio C Hamano

pack-check.c: remove the_repository references

2018-11-12T05:50:06Z

Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano

convert "hashcmp() != 0" to "!hasheq()"

2018-08-29T18:32:49Z

This rounds out the previous three patches, covering the inequality logic for the "hash" variant of the functions. As with the previous three, the accompanying code changes are the mechanical result of applying the coccinelle patch; see those patches for more discussion. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

packfile: add repository argument to unpack_entry

2018-04-26T01:54:27Z

Add a repository argument to allow the callers of unpack_entry to be more specific about which repository to act on. This is a small mechanical change; it doesn't change the implementation to handle repositories other than the_repository yet. As with the previous commits, use a macro to catch callers passing a repository other than the_repository at compile time. Signed-off-by: Jonathan Nieder Signed-off-by: Stefan Beller Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano