vd/fix-unaligned-read-index-v4, was Re: What's cooking in git.git (Sep 2022, #08; Tue, 27)

From: Jeff King <peff@peff.net>
Date: Sep 28, 2022, 04:18 UTC
Message-ID: <YzPLBN09zzlTdNgc@coredump.intra.peff.net>
In-Reply-To: <be8f11f2-c4ad-0542-066b-3bbc99a16dae@github.com>

On Tue, Sep 27, 2022 at 06:52:02PM -0700, Victoria Dye wrote:

Show 21 quoted lines

> Junio C Hamano wrote:
> > * vd/fix-unaligned-read-index-v4 (2022-09-23) 1 commit
> >  - read-cache: avoid misaligned reads in index v4
> > 
> >  The codepath that reads from the index v4 had unaligned memory
> >  accesses, which has been corrected.
> > 
> >  Expecting a reroll.
> >  cf. <Yy4nkEnhuzt2iH+R@coredump.intra.peff.net>
> >  cf. <bb3a2470-7ff5-e4a6-040a-96e0e3833978@gmail.com>
> >  source: <pull.1366.git.1663962236069.gitgitgadget@gmail.com>
> > 
> 
> How drastic an update were you expecting for this re-roll? To keep the fix
> minimal (that is, focused on 'create_from_disk()'), I was planning to just
> add some comments explaining the implementation (in response to [1]). If the
> goal is to get this merged quickly, I'd want to avoid a larger refactor
> (suggested in [2] & [3]), since doing so would either make the
> implementations of "read from disk" ('create_from_disk()') and "write to
> disk" ('copy_cache_entry_to_ondisk()') different/difficult to compare, or
> would involve a more invasive refactor to update both functions [4].

The first "cf" there is my initial request for a v2, but I since retracted that. I have no objection to adding more comments, but I am happy enough without them (like Junio, it may be that I'm overly familiar with how I expect our get_be() functions to handle alignment).

I think even if we want to go further in the near term, it is still best to build it on top of your basic fix. And we can take that fix and make further refactoring (if any) its own topic.

I don't think even that basic fix needs to happen before the release, though. While it is a slight regression that SANITIZE=undefined does not pass in the 2.38 release candidates:

  - the bug really was there all along. It's just that a new test
    triggers it.

  - I don't think it's a sign of actual problems; we are forming an
    unaligned pointer via a funny cast, which is technically UB, but I
    don't think any real-world platforms actually care, since we
    dereference it only via get_be16().

So it's mostly just a minor annoyance for running the tests; we're probably better not to change any code, even trivially, this late in the release cycle.

I was going to point to my branch with commits that are slightly less rough than what I posted, for somebody to work on it later if interested. But after applying a minimal amount of polish, I think they are pretty reasonable, so I'll actually share them here.

IMHO it would be OK to proceed with them even if we don't redo the writing side. But I can see the argument that they should come together.

I'm also OK if we just drop it. It was a fun puzzle to make it all work, and I do like the result. But arguably it's just churn, and in particular I haven't measured the re-ordering patch to see if it produces a performance difference. I doubt it does, but it's important enough that we should get a clear answer.

So here are my refactoring patches for the read side, built on top of your earlier patch. They resolve the "yuck" comment by storing and copying individual fields (which is much less verbose than you'd think because of struct assignment). I left the pointer type as signed; there are still come casts, but the same ones that were necessary before my series.

  [1/3]: pack-bitmap: make read_be32() public
  [2/3]: read-cache: read on-disk entries in byte order
  [3/3]: read-cache: use read_be32() in create_from_disk()

 compat/bswap.h | 22 ++++++++++++++++++
 pack-bitmap.c  | 12 ----------
 read-cache.c   | 60 +++++++++++++++++++++++++-------------------------
 3 files changed, 52 insertions(+), 42 deletions(-)

-Peff