From: Jonathan Nieder <jrnieder@gmail.com>
Date: Thu, 23 Jan 2014 23:34:16 GMT
Subject: Re: [PATCH 2/3] read-cache: use get_be32 instead of hand-rolled ntoh_l
Message-ID: <20140123233416.GE18964@google.com>
In-Reply-To: <20140123212642.GB21705@sigill.intra.peff.net>

Jeff King wrote:

> This _might_ still suffer from the issue fixed in 5f6a112 (block-sha1:
> avoid pointer conversion that violates alignment constraints,
> 2012-07-22), as we are taking the pointer of a uint32 in a struct.

No conversion, so no issue there.

Line 1484 looks more problematic:

		disk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset);

In v4 indexes, src_offset doesn't have any particular alignment so
this conversion has undefined behavior.

Do you know if any tests exercise this code with paths that don't
have convenient length?

[...]
> I'm inclined to leave it for now, as we haven't made anything worse, and
> nobody has reported a problem.

Yeah, agreed.

Probably the simplest fix would be to take a char *, memcpy into a
new (aligned) buffer and then byteswap in place, but that's
orthogonal to this series.

Thanks,
Jonathan