Re: Unaligned accesses in sha1dc

On 2 June 2017 at 22:14, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Fri, Jun 2, 2017 at 10:11 PM, Martin Ågren [off-list ref] wrote:

quoted

On 2 June 2017 at 21:32, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

On Fri, Jun 2, 2017 at 11:49 AM, Martin Ågren [off-list ref] wrote:

quoted

On 2 June 2017 at 10:51, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

On Fri, Jun 2, 2017 at 2:15 AM, Junio C Hamano [off-list ref] wrote:

quoted

Martin Ågren [off-list ref] writes:

quoted

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.

Right, with "platform" meaning "combination of hardware-architecture
and compiler". Nothing can be said about how the current code behaves
on "x86". Such statements can only be made with regard to "x86 and
this or that compiler". Even then, short of studying the compiler
implementation/documentation in detail, one cannot be certain that
seemingly unrelated changes in Git don't make the code do something
else entirely.

I think you're veering into a theoretical discussion here that has
little to no bearing on the practicalities involved here.

Yes if something is undefined behavior in C the compiler &
architecture is free to do anything they want with it. In practice
lots of undefined behavior is de-facto standardized across various
platforms.

As far as I can tell unaligned access is one of those things. I don't
think there's ever been an x86 chip / compiler that would run this
code with any semantic differences when it comes to unaligned access,
and such a chip / compiler is unlikely to ever exist.

I'm not advocating that we rely on undefined behavior willy-nilly,
just that we should consider the real situation is (i.e. what actual
architectures / compilers are doing or are likely to do) as opposed to
the purely theoretical (if you gave a bunch of aliens who'd never
heard of our technology the ANSI C standard to implement from
scratch).

Yeah, that's an argument. I just thought I'd provide whatever input I
could, albeit in text form. The only thing that matters in the end is
that you (the Git project) feel that you make the correct decision,
possibly going beyond "theoretical" reasoning into engineering-land.

I forgot to note, I think it would be very useful if you could submit
that patch of yours in cleaned up form to the upstream sha1dc project:
https://github.com/cr-marcstevens/sha1collisiondetection

They might be interested in taking it, even if it's guarded by some
macro "don't do unaligned access even on archs that seem OK with it".

My comments are just focusing on this in the context of whether we
should be hotfixing our copy due to an issue in the wild, like e.g.
the SPARC issue.

A good way to get the sha1dc project properly tests on all platforms
would be to wrap it in a cpan distribution and let cpants (cpan
testers) test it for you on all the platforms under the sun.

In the Sereal project we found and fixed many portability issues with
the csnappy code simply because there are people testing modules in
the cpan world on every platform you can think of, and a few you might
be surprised to find out people still use.

Yves

Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"