Re: [PATCH] read-cache: avoid misaligned reads in index v4

[Jeff King <peff@peff.net>]

On Mon, Sep 26, 2022 at 03:08:28PM -0400, Jeff King wrote:

> So we either need to go out of order, or parse into a dummy ce_entry and
> then memcpy the results into the heap-allocated one.

Here's the "dummy" version. I did this mostly to satisfy my own
curiosity, and am sharing so the effort isn't lost. If you are ready to
move on from the topic, don't feel compelled to read or respond. :)

I do find it a bit more straight-forward, but the extra copy is ugly,
plus the "yuck" comment.

diff --git a/read-cache.c b/read-cache.c
index d16eb97906..8773f833bb 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1875,17 +1875,19 @@ static int read_index_extension(struct index_state *istate,
 
 static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
 					    unsigned int version,
-					    const char *ondisk,
+					    const void *ondisk_map,
 					    unsigned long *ent_size,
 					    const struct cache_entry *previous_ce)
 {
+	const unsigned char *ondisk = ondisk_map;
+	struct cache_entry pe; /* parsed entry; not sized to hold name */
 	struct cache_entry *ce;
 	size_t len;
-	const char *name;
 	const unsigned hashsz = the_hash_algo->rawsz;
-	const char *flagsp = ondisk + offsetof(struct ondisk_cache_entry, data) + hashsz;
 	unsigned int flags;
 	size_t copy_len = 0;
+	size_t pos = 0;
+
 	/*
 	 * Adjacent cache entries tend to share the leading paths, so it makes
 	 * sense to only store the differences in later entries.  In the v4
@@ -1895,24 +1897,35 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
 	 */
 	int expand_name_field = version == 4;
 
-	/* On-disk flags are just 16 bits */
-	flags = get_be16(flagsp);
+	pe.ce_stat_data.sd_ctime.sec = read_be32(ondisk, &pos);
+	pe.ce_stat_data.sd_ctime.nsec = read_be32(ondisk, &pos);
+	pe.ce_stat_data.sd_mtime.sec = read_be32(ondisk, &pos);
+	pe.ce_stat_data.sd_mtime.nsec = read_be32(ondisk, &pos);
+	pe.ce_stat_data.sd_dev   = read_be32(ondisk, &pos);
+	pe.ce_stat_data.sd_ino   = read_be32(ondisk, &pos);
+	pe.ce_mode  = read_be32(ondisk, &pos);
+	pe.ce_stat_data.sd_uid   = read_be32(ondisk, &pos);
+	pe.ce_stat_data.sd_gid   = read_be32(ondisk, &pos);
+	pe.ce_stat_data.sd_size  = read_be32(ondisk, &pos);
+
+	oidread(&pe.oid, ondisk + pos);
+	pos += hashsz;
+
+	flags = read_be16(ondisk, &pos);
 	len = flags & CE_NAMEMASK;
 
 	if (flags & CE_EXTENDED) {
 		int extended_flags;
-		extended_flags = get_be16(flagsp + sizeof(uint16_t)) << 16;
+		extended_flags = read_be16(ondisk, &pos) << 16;
 		/* We do not yet understand any bit out of CE_EXTENDED_FLAGS */
 		if (extended_flags & ~CE_EXTENDED_FLAGS)
 			die(_("unknown index entry format 0x%08x"), extended_flags);
 		flags |= extended_flags;
-		name = (const char *)(flagsp + 2 * sizeof(uint16_t));
 	}
-	else
-		name = (const char *)(flagsp + sizeof(uint16_t));
+	pe.ce_flags = flags & ~CE_NAMEMASK;
 
 	if (expand_name_field) {
-		const unsigned char *cp = (const unsigned char *)name;
+		const unsigned char *cp = ondisk + pos;
 		size_t strip_len, previous_len;
 
 		/* If we're at the beginning of a block, ignore the previous name */
@@ -1924,43 +1937,29 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
 					previous_ce->name);
 			copy_len = previous_len - strip_len;
 		}
-		name = (const char *)cp;
+		pos = cp - ondisk;
 	}
 
 	if (len == CE_NAMEMASK) {
-		len = strlen(name);
+		len = strlen((const char *)ondisk + pos);
 		if (expand_name_field)
 			len += copy_len;
 	}
 
 	ce = mem_pool__ce_alloc(ce_mem_pool, len);
-
-	ce->ce_stat_data.sd_ctime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)
-							+ offsetof(struct cache_time, sec));
-	ce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)
-							+ offsetof(struct cache_time, sec));
-	ce->ce_stat_data.sd_ctime.nsec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)
-							 + offsetof(struct cache_time, nsec));
-	ce->ce_stat_data.sd_mtime.nsec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)
-							 + offsetof(struct cache_time, nsec));
-	ce->ce_stat_data.sd_dev   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, dev));
-	ce->ce_stat_data.sd_ino   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ino));
-	ce->ce_mode  = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mode));
-	ce->ce_stat_data.sd_uid   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, uid));
-	ce->ce_stat_data.sd_gid   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, gid));
-	ce->ce_stat_data.sd_size  = get_be32(ondisk + offsetof(struct ondisk_cache_entry, size));
-	ce->ce_flags = flags & ~CE_NAMEMASK;
-	ce->ce_namelen = len;
+	memcpy(ce, &pe, sizeof(pe));
 	ce->index = 0;
-	oidread(&ce->oid, (const unsigned char *)ondisk + offsetof(struct ondisk_cache_entry, data));
+	ce->ce_namelen = len;
+	/* yuck, our memcpy overwrote this */
+	ce->mem_pool_allocated = 1;
 
 	if (expand_name_field) {
 		if (copy_len)
 			memcpy(ce->name, previous_ce->name, copy_len);
-		memcpy(ce->name + copy_len, name, len + 1 - copy_len);
-		*ent_size = (name - ((char *)ondisk)) + len + 1 - copy_len;
+		memcpy(ce->name + copy_len, ondisk + pos, len + 1 - copy_len);
+		*ent_size = pos + len + 1 - copy_len;
 	} else {
-		memcpy(ce->name, name, len + 1);
+		memcpy(ce->name, ondisk + pos, len + 1);
 		*ent_size = ondisk_ce_size(ce);
 	}
 	return ce;