From 5f6a11259ab0045a9f79bd789393de7a77e3c5d6 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sun, 22 Jul 2012 18:39:54 -0500 Subject: block-sha1: avoid pointer conversion that violates alignment constraints With 660231aa (block-sha1: support for architectures with memory alignment restrictions, 2009-08-12), blk_SHA1_Update was modified to access 32-bit chunks of memory one byte at a time on arches that prefer that: #define get_be32(p) ( \ (*((unsigned char *)(p) + 0) << 24) | \ (*((unsigned char *)(p) + 1) << 16) | \ (*((unsigned char *)(p) + 2) << 8) | \ (*((unsigned char *)(p) + 3) << 0) ) The code previously accessed these values by just using htonl(*p). Unfortunately, Michael noticed on an Alpha machine that git was using plain 32-bit reads anyway. As soon as we convert a pointer to int *, the compiler can assume that the object pointed to is correctly aligned as an int (C99 section 6.3.2.3 "pointer conversions" paragraph 7), and gcc takes full advantage by using a single 32-bit load, resulting in a whole bunch of unaligned access traps. So we need to obey the alignment constraints even when only dealing with pointers instead of actual values. Do so by changing the type of 'data' to void *. This patch renames 'data' to 'block' at the same time to make sure all references are updated to reflect the new type. Reported-tested-and-explained-by: Michael Cree Signed-off-by: Jonathan Nieder Acked-by: Linus Torvalds Signed-off-by: Junio C Hamano --- block-sha1/sha1.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c index d8934757a5..10fd94d179 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((unsigned char *) 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 void *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); -- cgit v1.2.1 From 23119ffb4ea91cdf30016254df60e1adc64b478c Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sun, 22 Jul 2012 18:40:54 -0500 Subject: block-sha1: put expanded macro parameters in parentheses 't' is currently always a numeric constant, but it can't hurt to prepare for the day that it becomes useful for a caller to pass in a more complex expression. Suggested-by: Linus Torvalds Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- block-sha1/sha1.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c index 10fd94d179..6f885c4333 100644 --- a/block-sha1/sha1.c +++ b/block-sha1/sha1.c @@ -100,8 +100,8 @@ * 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((unsigned char *) block + t*4) -#define SHA_MIX(t) SHA_ROL(W(t+13) ^ W(t+8) ^ W(t+2) ^ W(t), 1) +#define SHA_SRC(t) get_be32((unsigned char *) 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 { \ unsigned int TEMP = input(t); setW(t, TEMP); \ -- cgit v1.2.1 From f200197c39d9181c02cac06c26433edaa9d31219 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Mon, 23 Jul 2012 01:29:14 -0500 Subject: Makefile: BLK_SHA1 does not require fast htonl() and unaligned loads block-sha1/ is fast on most known platforms. Clarify the Makefile to be less misleading about that. Early versions of block-sha1/ explicitly relied on fast htonl() and fast 32-bit loads with arbitrary alignment. Now it uses those on some arches but the default behavior is byte-at-a-time access for the sake of arches like ARM, Alpha, and their kin and it is still pretty fast on these arches (fast enough to supersede the mozilla SHA1 implementation and the hand-written ARM assembler implementation that were bundled before). Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- Makefile | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 134606b9b7..eadcc70ace 100644 --- a/Makefile +++ b/Makefile @@ -84,9 +84,8 @@ all:: # specify your own (or DarwinPort's) include directories and # library directories by defining CFLAGS and LDFLAGS appropriately. # -# Define BLK_SHA1 environment variable if you want the C version -# of the SHA1 that assumes you can do unaligned 32-bit loads and -# have a fast htonl() function. +# Define BLK_SHA1 environment variable to make use of the bundled +# optimized C SHA1 routine. # # Define PPC_SHA1 environment variable when running make to make use of # a bundled SHA1 routine optimized for PowerPC. -- cgit v1.2.1