Re: [PATCH v3 01/10] packfile: prepare for the existence of '*.rev' files

From: Jeff King <peff@peff.net>
Date: Jan 29, 2021, 00:27 UTC
Message-ID: <YBNWXU8xzw0087DC@coredump.intra.peff.net>
In-Reply-To: <6f8b70ab276c0579c957c315743fdab63462a605.1611617820.git.me@ttaylorr.com>

On Mon, Jan 25, 2021 at 06:37:14PM -0500, Taylor Blau wrote:

Show 12 quoted lines

> +struct revindex_header {
> +	uint32_t signature;
> +	uint32_t version;
> +	uint32_t hash_id;
> +};
> [...]
> +	struct revindex_header *hdr;
> [...]
> +	data = xmmap(NULL, revindex_size, PROT_READ, MAP_PRIVATE, fd, 0);
> +	hdr = data;
> +
> +	if (ntohl(hdr->signature) != RIDX_SIGNATURE) {

I guess this technique was pulled from the .idx code paths, but IMHO we should avoid this kind of struct-casting. It relies on the compiler's struct packing, and I'm not 100% sure it doesn't violate some weird pointer-aliasing rules.

OTOH, as long as we do not ever care about sizeof(revindex_header), this is likely to work in practice, since every field is at least 4-byte aligned (but there is probably an extra 4 bytes of padding at the end).

The "right" way is probably something like:

  const char *header = data;
  if (get_be32(header) != RIDX_SIGNATURE)
          error...;
  header += 4;
  if (get_be32(header) != 1)
          error...;
  header += 4;
  ...etc...

I thought we had helpers to read and advance the pointer, but it looks like those are specific to the bitmap code (e.g., read_be32(), though it uses a separate offset variable).

I dunno. Maybe I am being overly picky. The .idx code already does it like this, and I believe the index (as in .git/index) does, too. We have run into problems (as in b5007211b6 (pack-bitmap: do not use gcc packed attribute, 2014-11-27)), but that was due to a more odd-sized struct, as well as using sizeof().

The rest of the patch looks good to me.

-Peff