Re: [PATCH 1/4] csum-file: introduce checksum_valid()

From: Jeff King <peff@peff.net>
Date: Jun 24, 2021, 19:42 UTC
Message-ID: <YNTgCMN3ImaLmzKT@coredump.intra.peff.net>
In-Reply-To: <3f1fcf348473d7320d46e3df6fe97c482b9888f2.1624473543.git.me@ttaylorr.com>

On Wed, Jun 23, 2021 at 02:39:07PM -0400, Taylor Blau wrote:

Show 15 quoted lines

> +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