Re: Unaligned accesses in sha1dc

On 1 June 2017 at 12:33, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Thu, Jun 1, 2017 at 12:26 PM, Martin Ågren [off-list ref] wrote:

quoted

On 1 June 2017 at 12:08, Andreas Schwab [off-list ref] wrote:

quoted

Even if the architecture implements unaligned accesses in hardware, it
is still undefined behaviour, and the compiler will (eventually) take
advantage of it.

I tried to optically follow the macros and ended up on line 87/89 in
lib/sha1.c of the sha1dc-library, where there is undefined behavior if
the address is unaligned, which it seems it could be. Maybe Git uses
some particular combination of macro-definitions and I went down the
wrong path... There might also be other spots; I haven't thrown UBSan
at the code.

Using memcpy on those lines should not be a performance problem on
platforms where unaligned access is ok, of course assuming the
compiler sees the opportunity.

This is what the upstream version of sha1dc now in the next branch
does, i.e. just does a memcpy() on platforms which aren't on a
whitelist of CPUs that allow unaligned access.

Ok, now I get it. Undefined behavior can occur on line 1772 in
sha1dc/sha1.c on "next", but only if SHA1DC_ALLOW_UNALIGNED_ACCESS is
defined. I don't think the macro does what its name suggests, though.
To me, it behaves more like
SHA1DC_RELY_ON_UNDEFINED_BEHAVIOR_TO_DO_THE_RIGHT_THING_BY_CHANCE...

So it seems the call chain of commands and macros could be redesigned
to work with "char*" instead of "uint32_t*"... Then the lines I
mentioned earlier could be converted to memcpy and "should" be
compiled to efficient loads where possible. Yes, I know, patches speak
for themselves, and no, this mail is not a patch.