Please make unaligned access configurable

[noloader]

Per a discussion on Git mailing list at One failed self test on Fedora 29 and assorted follow-ups. The discussion concerns itself with a failed audit due to unaligned accesses in sha1dc/sha1.c (using CFLAGS += -fsanitize=undefined).

And in particular, from Jeff King at disabling sha1dc unaligned access:

Unfortunately, I don't think sha1dc currently supports #defines in that direction. The only logic is "if we are on intel, do unaligned loads" and "even if we are not on intel, do it anyway". There is no "even if we are on intel, do not do unaligned loads".

I think you'd need something like this:

diff --git a/Makefile b/Makefile index 148668368b..705c54dcd8 100644 --- a/Makefile +++ b/Makefile @@ -1194,6 +1194,7 @@ BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE) BASIC_CFLAGS += -fno-omit-frame-pointer ifneq ($(filter undefined,$(SANITIZERS)),) BASIC_CFLAGS += -DNO_UNALIGNED_LOADS +BASIC_CFLAGS += -DSHA1DC_DISALLOW_UNALIGNED_ACCESS endif ifneq ($(filter leak,$(SANITIZERS)),) BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c index df0630bc6d..0bdf80d778 100644 --- a/sha1dc/sha1.c +++ b/sha1dc/sha1.c @@ -124,9 +124,11 @@ #endif /ENDIANNESS SELECTION/

+#ifndef SHA1DC_DISALLOW_UNALIGNED_ACCESS #if defined(SHA1DC_FORCE_UNALIGNED_ACCESS) || defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR) #define SHA1DC_ALLOW_UNALIGNED_ACCESS #endif /UNALIGNMENT DETECTION/ +#endif

#define rotate_right(x,n) (((x)>>(n))|((x)<<(32-(n))))

but of course we cannot touch sha1dc/*, because we might actually be using the submodule copy instead. And AFAIK there is no good way to modify the submodule-provided content as part of the build. Why do we even have the submodule again? ;P

I guess the same would be true for DC_SHA1_EXTERNAL, too, though.

So anyway, I think this needs a patch to the upstream sha1dc project.

Please provide an option to disable unaligned accesses.

[cr-marcstevens]

Does branch 'force_aligned' solve your issues? https://github.com/cr-marcstevens/sha1collisiondetection/tree/force_aligned

You will need to add -DSHA1DC_FORCE_ALIGNED_ACCESS to your compiler options.

[peff]

@cr-marcstevens Yep, that would fine (and I agree sticking with the FORCE naming scheme is much clearer than what I showed in the patch above).

[shumow]

We should merge this change back into master. @peff are you ready to take these changes back into Git now?

[peff]

@shumow Yeah, as soon as this is merged, I can take care of the Git side of things.

[shumow]

@cr-marcstevens I see that there watson checks haven't been run on these changes yet? Has the watson integration been turned off for this branch/project?

Marc, if you're happy with the change, I'm happy to merge it back into master. Of course, pending figuring out what's up with the watson checks.

[cr-marcstevens]

@shumow I have received emails from TravisCI that both commits in the branch have passed, so TravisCI is still working. But indeed, I also don't see those same confirmations visibly in GitHub.

If everyone is happy then I'll merge it now.

[shumow]

Ahh, yeah -- Travis, not watson. (Can't keep all these proper names straight.) As long as you can confirm it, that's fine.

[peff]

Thanks for the quick turnaround! The Git patch is at https://public-inbox.org/git/[email protected]/.