Re: Unaligned accesses in sha1dc

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 2, 2017, 00:15 UTC
Message-ID: <xmqq37bj454a.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAN0heSp9DpW4_0QL57_oAHGu+os8k6yd=Z5+0MJnaL6iXTa-qQ@mail.gmail.com>

Martin Ågren <martin.agren@gmail.com> writes:

Show 21 quoted lines

> I looked into this some more. It turns out it is possible to trigger
> undefined behavior on "next". Here's what I did:
> ...
>
> This "fixes" the problem:
> ...
> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
> index 3dff80a..d6f4c44 100644
> --- a/sha1dc/sha1.c
> +++ b/sha1dc/sha1.c
> @@ -66,9 +66,9 @@
> ...
> With this diff, various tests which seem relevant for SHA-1 pass,
> including t0013, and the UBSan-error is gone. The second diff is just
> a monkey-patch. I have no reason to believe I will be able to come up
> with a proper and complete patch for sha1dc. And I guess such a thing
> would not really be Git's patch to carry, either. But at least Git
> could consider whether to keep relying on undefined behavior or not.
>
> There's a fair chance I've mangled the whitespace. I'm using gmail's
> web interface... Sorry about that.

Thanks. I see Marc Stevens is CC'ed in the thread, so I'd expect that the final "fix" would come from his sha1collisiondetection repository via Ævar.

In the meantime, I am wondering if it makes sense to merge the earlier update with #ifdef ALLOW_UNALIGNED_ACCESS and #ifdef SHA1DC_FORCE_LITTLEENDIAN for the v2.13.x maintenance track, which would at least unblock those on platforms v2.13.0 did not work correctly at all.

Ævar, thoughts?