Re: [PATCH 2/3] read-cache: use get_be32 instead of hand-rolled ntoh_l

[Jonathan Nieder]

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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] read-cache: use get_be32 instead of hand-rolled ntoh_l

[Jeff King]

On Thu, Jan 23, 2014 at 03:34:16PM -0800, Jonathan Nieder wrote:

 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?

My impression was that we are not testing v4 index at all (and grepping
for `--index-version`, which I think is the only way to write it,
supports that).

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html