[PATCH maint-1.6.5] block-sha1: avoid unaligned accesses on some big-endian systems

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 14, 2012, 07:59 UTC
Message-ID: <20120714075906.GD3693@burratino>
In-Reply-To: <50010B84.5030606@orcon.net.nz>

With 660231aa (block-sha1: support for architectures with memory alignment restrictions, 2009-08-12), blk_SHA1_Update was modified to work reasonably on RISC-y architectures by accessing 32-bit values one byte at a time on machines that prefer that:

	#define get_unaligned_be32(p)    ( \
		(*((unsigned char *)(p) + 0) << 24) | \
		(*((unsigned char *)(p) + 1) << 16) | \
		(*((unsigned char *)(p) + 2) <<  8) | \
		(*((unsigned char *)(p) + 3) <<  0) )

Unfortunately, on big-endian architectures, if p is a pointer to unsigned int then current gcc assumes it is properly aligned and converts this construct to a 32-bit load. The result is the same alignment fault we want to avoid. Fix it by passing around 'data' as an unsigned char * instead of unsigned int * to more clearly document the intended alignment for compilers and humans.

The patch renames 'data' to 'block' to make sure all references are updated to reflect its new type.

Noticed on an alpha. Typical ARM systems and other little-endian arches don't hit this because with the endianness conversion gcc (rightly) prefers reading one byte at a time even if the pointer is aligned.

Reported-and-tested-by: Michael Cree <mcree@orcon.net.nz>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi,

Michael Cree wrote[1]:

> On 14/07/12 14:18, Jonathan Nieder wrote:

>> Does this patch help?
>
> Yes, it does!  Unaligned accesses are no longer reported and git is
> running noticeably faster.

Thanks!  Nico, Linus, what do you think?

As Michael mentioned, in the long term it might make sense to tweak git to always pass aligned pointers and lengths in the 'data' and 'len' arguments to SHA1_Update, but that's an experiment for another day.

[1] http://bugs.debian.org/681532

 block-sha1/sha1.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
index d8934757..4d72e4bc 100644
--- a/block-sha1/sha1.c
+++ b/block-sha1/sha1.c
@@ -100,7 +100,7 @@
  * Where do we get the source from? The first 16 iterations get it from
  * the input data, the next mix it from the 512-bit array.
  */
-#define SHA_SRC(t) get_be32(data + t)
+#define SHA_SRC(t) get_be32(block + t*4)
 #define SHA_MIX(t) SHA_ROL(W(t+13) ^ W(t+8) ^ W(t+2) ^ W(t), 1)
 
 #define SHA_ROUND(t, input, fn, constant, A, B, C, D, E) do { \
@@ -114,7 +114,7 @@
 #define T_40_59(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B&C)+(D&(B^C))) , 0x8f1bbcdc, A, B, C, D, E )
 #define T_60_79(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (B^C^D) ,  0xca62c1d6, A, B, C, D, E )
 
-static void blk_SHA1_Block(blk_SHA_CTX *ctx, const unsigned int *data)
+static void blk_SHA1_Block(blk_SHA_CTX *ctx, const unsigned char *block)
 {
 	unsigned int A,B,C,D,E;
 	unsigned int array[16];
@@ -125,7 +125,7 @@ static void blk_SHA1_Block(blk_SHA_CTX *ctx, const unsigned int *data)
 	D = ctx->H[3];
 	E = ctx->H[4];
 
-	/* Round 1 - iterations 0-16 take their input from 'data' */
+	/* Round 1 - iterations 0-16 take their input from 'block' */
 	T_0_15( 0, A, B, C, D, E);
 	T_0_15( 1, E, A, B, C, D);
 	T_0_15( 2, D, E, A, B, C);
@@ -251,7 +251,7 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len)
 		data = ((const char *)data + left);
 		if (lenW)
 			return;
-		blk_SHA1_Block(ctx, ctx->W);
+		blk_SHA1_Block(ctx, (const unsigned char *) ctx->W);
 	}
 	while (len >= 64) {
 		blk_SHA1_Block(ctx, data);

-- 
1.7.10.4