On Wed, Jun 23, 2021 at 02:39:07PM -0400, Taylor Blau wrote:
> +int hashfile_checksum_valid(const unsigned char *data, size_t total_len)
> +{
> + unsigned char got[GIT_MAX_RAWSZ];
> + git_hash_ctx ctx;
> + size_t data_len = total_len - the_hash_algo->rawsz;
> +
> + if (total_len < the_hash_algo->rawsz)
> + return 0; /* say "too short"? */
> +
> + the_hash_algo->init_fn(&ctx);
> + the_hash_algo->update_fn(&ctx, data, data_len);
> + the_hash_algo->final_fn(got, &ctx);
> +
> + return hasheq(got, data + data_len);
> +}
This "too short" comment is a little ugly, but I don't think it's easy
to do better. This function does not otherwise print anything to stderr,
so calling error() here would probably be bad. And having multiple
return types would complicate the callers.
I _suspect_ it's hard to trigger anyway, because the callers would
generally have complained about a too-short file in the first place. So
possibly it could be a BUG(). But I think leaving it as-is is the more
conservative choice, as we don't know what checks callers will have
done.
In the long run, we might want to teach this function to actually write
to stderr (or an error buffer), because it may be useful to say:
hash mismatch: trailer checksum claims 1234abcd, but we computed 5678cdef
or similar. In which case we could then say "too short" in the same way.
I'm content to leave that for later if somebody cares. In every case of
file corruption I've seen, things are sufficiently complicated and
confusing that I turn to a hex dump of the file anyway. ;)
-Peff
