On Fri, Jun 2, 2017 at 4:46 PM, Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
> * ?var Arnfj?r? Bjarmason <avarab@xxxxxxxxx> [170602 04:53]:
>> On Fri, Jun 2, 2017 at 2:15 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> > Martin Ågren <martin.agren@xxxxxxxxx> writes:
>> >
>> >> 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?
>>
>> I think we're mixing up several things here, which need to be untangled:
>>
>> 1) The sha1dc works just fine on most platforms even with undefined
>> behavior, as evidenced by 2.13.0 working.
>>
>> 2) There was a bug in practice with unaligned access on SPARC. It's
>> not clear to me whether anyone (Andreas, Liam?) still has any issues
>> in practice on any platform without specifying compile flags like what
>> Martin Ågren suggested above.
>>
>> Andreas: Is your initial report of unaligned access here fixed in the
>> next branch with my "sha1dc: update from upstream" commit? You didn't
>> say what platform you were on.
>>
>> Liam: How about your issue on SPARC?
>
> 2.13.0 is very much broken for me on SPARC.
> {maint//git} $ make -j120
> [...]
> {maint//git} $ ./git log
> [1] 1004506 bus error (core dumped) ./git log
>
> This is with b06d36431 (maint).
>
> The same thing happens on v2.13.0-384-g826c06412 (master).
>
> v2.13.0-539-g4b9c06c7d (next) works for me, as did following the
> instructions on upgrading the sha1dc code myself.
Thanks a lot. So that works as I suspect on SPARC, hopefully it'll be
in master (and 2.13.1) soon.
>>
>> 3) Now we have another issue reported by Martin Ågren here, which is
>> that while the code works in practice on most platforms it's using
>> undefined behavior. On my GCC 7.1.1 it's sufficient to:
>
> My platforms gcc is older than 7.1.1.
Right, shouldn't matter. Just thought I'd note the version for context
to note what version was producing that warning.
>>
>> make -j8 CFLAGS="-fsanitize=undefined
>> -fsanitize-recover=undefined" LDFLAGS="-fsanitize=undefined
>> -fsanitize-recover=undefined" all
>>
>> And then run e.g.:
>>
>> ./t0020-crlf.sh -v
>
> These tests pass With my older gcc - which those flags are not
> recognized.
>
> # passed all 35 test(s)
>
>
>>
>> To get spiel like:
>>
>> sha1dc/sha1.c:346:2: runtime error: load of misaligned address
>> 0x5610bf16d005 for type 'const uint32_t', which requires 4 byte
>> alignment
>> 0x5610bf16d005: note: pointer points here
>> 65 6e 74 20 66 30 34 66 61 39 37 36 36 64 62 62 38 65 34 63 37
>> 33 38 34 37 30 61 31 36 63 61 62
>>
>> I think that this is definitely something worth looking into /
>> coordinating with upstream, but I haven't seen anything to suggest
>> that we need to be rushing to get a patch in to fix this given 1) and
>> nobody saying yet that 2) doesn't solve their issue as long as they're
>> not supplying some -fsanitize=* flags.
>>
>> Now, stepping a bit back from this whole thing: I didn't read the
>> entire discussion back in February when sha1dc was integrated, but I
>> really don't see given all this churn / bug reporting we're getting
>> now why another acceptable solution wouldn't be to just revert
>> e6b07da278 ("Makefile: make DC_SHA1 the default", 2017-03-17) &
>> release 2.13.1 with that.
>>
>> Clearly there are outstanding issues with it, and needing to do a
>> memcpy() as my `next` patch does will harm performance on some
>> platforms, and something like Martin's patch on top will slow it down
>> even more.
>>
>> It seems to me that we should give it more time to cook, and better
>> understand the various trade-offs involved. The shattered attack is
>> very unlikely to impact anything in practice, and users who are
>> paranoid about it can opt-in to this extra protection.
>
> I have not seen issues with DC_SAH1 with the newer code base on SPARC.
Great, thanks!