Re: [PATCH v3 2/4] eoie: add End of Index Entry (EOIE) extension

From: Martin Ågren <martin.agren@gmail.com>
Date: Sep 8, 2018, 06:29 UTC
Message-ID: <CAN0heSreAfMsseZcxR75CFDph-n1b8EUNsRhpFsVqxMLc0hvpA@mail.gmail.com>
In-Reply-To: <fc531863-c46c-6d27-4749-c6b092a14a6f@gmail.com>

On Fri, 7 Sep 2018 at 22:24, Ben Peart <peartben@gmail.com> wrote:

> > Ben Peart <benpeart@microsoft.com> writes:

> >> - 160-bit SHA-1 over the extension types and their sizes (but not
> >> their contents).  E.g. if we have "TREE" extension that is N-bytes
> >> long, "REUC" extension that is M-bytes long, followed by "EOIE",
> >> then the hash would be:

> The purpose of the SHA isn't to detect disk corruption (we already have
> a SHA for the entire index that can serve that purpose) but to help
> ensure that this was actually a valid EOIE extension and not a lucky
> random set of bytes. [...]

> >> +#define EOIE_SIZE 24 /* <4-byte offset> + <20-byte hash> */

> >> +    the_hash_algo->init_fn(&c);
> >> +    while (src_offset < mmap_size - the_hash_algo->rawsz - EOIE_SIZE_WITH_HEADER) {

[...]

Show 7 quoted lines

> >> +    the_hash_algo->final_fn(hash, &c);
> >> +    if (hashcmp(hash, (unsigned char *)index))
> >> +            return 0;
> >> +
> >> +    /* Validate that the extension offsets returned us back to the eoie extension. */
> >> +    if (src_offset != mmap_size - the_hash_algo->rawsz - EOIE_SIZE_WITH_HEADER)
> >> +            return 0;

Besides the issue you and Junio discussed with "should we document this as being SHA-1 or NewHash" (or "the hash algo"), it seems to me that this implementation is living somewhere between using SHA-1 and "the hash algo". The hashing uses `the_hash_algo`, but the hash size is hardcoded at 20 bytes.

Maybe it all works out, e.g., so that when someone (brian) merges a NewHash and runs the testsuite, this will fail consistently and in a safe way. But I wonder if it would be too hard to avoid the hardcoded 24 already now.

Martin