git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Cree <mcree@orcon.net.nz>,
	git@vger.kernel.org, Nicolas Pitre <nico@fluxnic.net>
Subject: [PATCH maint-1.6.5 v2] block-sha1: avoid pointer conversion that violates alignment constraints
Date: Sat, 14 Jul 2012 15:50:49 -0500	[thread overview]
Message-ID: <20120714205049.GA28502@burratino> (raw)
In-Reply-To: <CA+55aFy+y=TCoJUQarinaduibt4i-46TAuvpp7fsAmjDZj_+3w@mail.gmail.com>

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 converting the get_be32
calls back to a load and bswap and producing 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-and-tested-by: Michael Cree <mcree@orcon.net.nz>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Linus Torvalds wrote:

> Anyway, the whole "noticed on alpha" makes no sense, since alpha isn't
> even big-endian.

Thanks again for catching that.  Here's a reroll.

 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 d8934757..10fd94d1 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);
-- 
1.7.10.4

  parent reply	other threads:[~2012-07-14 20:51 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20120713233957.6928.87541.reportbug@electro.phys.waikato.ac.nz>
     [not found] ` <20120714002950.GA3159@burratino>
     [not found]   ` <5000CBCA.8020607@orcon.net.nz>
     [not found]     ` <20120714021856.GA3062@burratino>
     [not found]       ` <50010B84.5030606@orcon.net.nz>
2012-07-14  7:59         ` [PATCH maint-1.6.5] block-sha1: avoid unaligned accesses on some big-endian systems Jonathan Nieder
2012-07-14 19:11           ` Linus Torvalds
2012-07-14 19:50             ` Jonathan Nieder
2012-07-14 19:55               ` Linus Torvalds
2012-07-14 20:50             ` Jonathan Nieder [this message]
2012-07-14 20:56               ` [PATCH maint-1.6.5 v2] block-sha1: avoid pointer conversion that violates alignment constraints Linus Torvalds
2012-07-14 21:57                 ` [PATCH] block-sha1: put macro arguments in parentheses Jonathan Nieder
2012-07-15 20:58               ` [PATCH maint-1.6.5 v2] block-sha1: avoid pointer conversion that violates alignment constraints Michael Cree
2012-07-15 21:27                 ` Jonathan Nieder
2012-07-16  9:53                   ` Michael Cree

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120714205049.GA28502@burratino \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=mcree@orcon.net.nz \
    --cc=nico@fluxnic.net \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).