Re: [PATCH] read-cache: avoid misaligned reads in index v4
- From
Victoria Dye <vdye@github.com>
- Date
- Sep 26, 2022, 15:39 UTC
- Message-ID
- <e5954e90-6b5c-46a6-0842-b3d7d1e06b33@github.com>
- In-Reply-To
- <Yy4nkEnhuzt2iH+R@coredump.intra.peff.net>
Jeff King wrote:
Show 20 quoted lines
> On Fri, Sep 23, 2022 at 07:43:55PM +0000, Victoria Dye via GitGitGadget wrote: >> @@ -1883,7 +1883,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool, >> size_t len; >> const char *name; >> const unsigned hashsz = the_hash_algo->rawsz; >> - const uint16_t *flagsp = (const uint16_t *)(ondisk->data + hashsz); >> + const char *flagsp = ondisk + offsetof(struct ondisk_cache_entry, data) + hashsz; > > Now we use the "const char *" pointer instead of the cast to the > ondisk_cache_entry struct, which is good, and is what fixes the > alignment question. > > But we also convert flagsp from being a uint16_t into a byte pointer. > I'm not sure if that's strictly necessary from an alignment perspective, > as we'd dereference it only via get_be16(), which handles alignment and > type conversion itself. > > I'd imagine the standard probably says that even forming such a pointer > is illegal, so in that sense, it probably is undefined behavior. But I > think it's one of those things that's OK in practice.
Yep, per the C standard §6.3.2.3 #7 [1]:
A pointer to an object or incomplete type may be converted to a pointer to a different object or incomplete type. If the resulting pointer is not correctly aligned for the pointed-to type, the behavior is undefined.
To your point, it is probably fine in practice, but I'd lean towards sticking with a 'char *' to play it safe.
[1] https://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf
Show 19 quoted lines
>> @@ -1935,20 +1935,24 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool, >> >> ce = mem_pool__ce_alloc(ce_mem_pool, len); >> >> - ce->ce_stat_data.sd_ctime.sec = get_be32(&ondisk->ctime.sec); >> [...] >> + ce->ce_stat_data.sd_ctime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime) >> + + offsetof(struct cache_time, sec)); > > I had figured we'd be able to drop ondisk_cache_entry entirely. But here > you're using it essentially as a template for a set of constants > retrieved via offsetof(). > > That's OK from an alignment perspective. It does mean we'd be in trouble > if a compiler ever decided to introduce padding into the struct. That's > probably unlikely. We don't use __attribute__((packed)) because it's not > portable, and our existing uses have generally been OK, because our > data structures are organized around 8-byte alignment. We might have > problems on a theoretical 128-bit processor or something.
In addition to portability, using '__attribute__((packed))' could hurt performance (and, in a large index, that might have a noticeable effect).
As for dropping 'ondisk_cache_entry()', I didn't want to drop it only from the "read" operation (and use something like the "parse left-to-right" strategy below) while leaving it in "write." And, as you mentioned later, changing 'ce_write_entry()' is a lot more invasive than what's already in this patch and possibly out-of-scope.
Show 14 quoted lines
> Another strategy is to just parse left-to-right, advancing the byte > pointer. Like: > > ce->ce_state_data.sd_ctime.sec = get_be32(ondisk); > ondisk += sizeof(uint32_t); > ce->ce_state_data.sd_mtime.sec = get_be32(ondisk); > ondisk += sizeof(uint32_t); > ...etc... > > You can even stick that in a helper function that does the get_b32() and > advances, so you know they're always done in sync. See pack-bitmap.c's > read_be32(), etc. IMHO this produces a nice result because the reading > code itself becomes the source of truth for the format. >
...
Show 23 quoted lines
> One final note, though: > >> + ce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime) >> + + offsetof(struct cache_time, sec)); > > Here (and elsewhere), you can assume that the offsetof() "sec" in > cache_time is 0, for two reasons: > > - I didn't look up chapter and verse, but I'm pretty sure the standard > does guarantee that the first field of a struct is at the beginning. > > - If there's any padding, this whole scheme is hosed anyway, because > it means sizeof(cache_time) is bigger than we expect, which messes > up the offsetof() the entry after us (in this case sd_dev). > > So this can just be: > > ce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)); > > which is mercifully shorter. > > Assuming we dismiss the rest of what I said as not worth it for a > minimal fix, I do think that simplification is worth rolling a v2.
That makes sense from a technical perspective, but I included the starting entry offset for readability reasons. It might be confusing to someone unfamiliar with C struct memory alignment to see every other 'get_be32' refer to the exact entry it's reading via the 'offsetof()', but have that information absent only for a few entries. And, the double 'offsetof()' would still be used by the 'mtime.nsec'/'ctime.nsec' fields anyway.
In any case, if this patch is intended to be a short-lived change on the way to a more complete refactor and/or I'm being overzealous on the readability, I'd be happy to change it. :)
Thanks!
Show 9 quoted lines
> > -Peff > > PS BTW, I mentioned earlier "can we just get rid of ondisk_cache_entry". > We also use it for the writing side, of course. That doesn't have > alignment issues, but it does have the same "I hope there's never any > padding" question. In an ideal world, it would be using the > equivalent put_be32(), but again, that's getting out of the "minimal > fix" territory.