Re: [PATCH 3/3] sha1: use char type for temporary work buffer
Le mercredi 12 septembre 2012 à 17:04 -0400, Jeff King a écrit : Wouldn't this break all of the code that is planning to index W by 32-bit words (see the definitions of setW in block-sha1/sha1.c)? That's not the same W ... This part of the code is indeed unclear. Sorry, you're right, that's a different work array (though it has the identical issue, no?). No, this one is really accessed as int. But would probably benefit being declared as uint32_t. But the point still stands. Did you audit the block-sha1 code to make sure nobody is ever indexing the W array? Yes. It was the first thing to do before changing its definition (for alignment purpose especially). If you didn't, then your change is not safe. If you did, then you should really mention that in the commit message. Sorry about this. I thought having the test suite OK was enough to prove this. If that is indeed the problem, wouldn't the simplest fix be using uint32_t instead of unsigned int? It's another way to fix this oddity, but not simpler. It is simpler in the sense that it does not have any side effects (like changing how every user of the data structure needs to index it). There's no other user than blk_SHA1_Update() Moreover, would that be sufficient to run on such a platform? At the very least, H above would want the same treatment. And I would not be surprised if some of the actual code in block-sha1/sha1.c needed updating, as well. ctx-H is actually used as an array of integer, so it would benefits of being declared uint32_t for an ILP64 system. This fix would also be required for blk_SHA1_Block() function. So...if we are not ready to run on an ILP system after this change, then what is the purpose? Readility: in blk_SHA1_Block(), the ctx-W array is used a 64 bytes len array, so, AFAIK, there's no point of having it defined as a 16 int len. It's disturbing while reading the code. This could allows us to change the memcpy() call further: @@ -246,7 +246,7 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len) unsigned int left = 64 - lenW; if (len left) left = len; - memcpy((char *)ctx-W + lenW, data, left); + memcpy(ctx-W + lenW, data, left); lenW = (lenW + left) 63; if (lenW) Regards. -- Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] sha1: use char type for temporary work buffer
The SHA context is holding a temporary buffer for partial block.
This block must 64 bytes long. It is currently described as
an array of 16 integers.
Signed-off-by: Yann Droneaud ydrone...@opteya.com
---
block-sha1/sha1.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h
index b864df6..d29ff6a 100644
--- a/block-sha1/sha1.h
+++ b/block-sha1/sha1.h
@@ -9,7 +9,7 @@
typedef struct {
unsigned long long size;
unsigned int H[5];
- unsigned int W[16];
+ unsigned char W[64];
} blk_SHA_CTX;
void blk_SHA1_Init(blk_SHA_CTX *ctx);
--
1.7.11.4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] sha1: use char type for temporary work buffer
On Wed, Sep 12, 2012 at 12:30:45PM +0200, Yann Droneaud wrote:
The SHA context is holding a temporary buffer for partial block.
This block must 64 bytes long. It is currently described as
an array of 16 integers.
Signed-off-by: Yann Droneaud ydrone...@opteya.com
---
block-sha1/sha1.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h
index b864df6..d29ff6a 100644
--- a/block-sha1/sha1.h
+++ b/block-sha1/sha1.h
@@ -9,7 +9,7 @@
typedef struct {
unsigned long long size;
unsigned int H[5];
- unsigned int W[16];
+ unsigned char W[64];
} blk_SHA_CTX;
Wouldn't this break all of the code that is planning to index W by
32-bit words (see the definitions of setW in block-sha1/sha1.c)?
You do not describe an actual problem in the commit message, but reading
between the lines it would be system X would like to use block-sha1,
but has an unsigned int that is not 32 bits. IOW, an ILP64 type of
architecture. Do you have some specific platform in mind?
If that is indeed the problem, wouldn't the simplest fix be using
uint32_t instead of unsigned int?
Moreover, would that be sufficient to run on such a platform? At the
very least, H above would want the same treatment. And I would not be
surprised if some of the actual code in block-sha1/sha1.c needed
updating, as well.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] sha1: use char type for temporary work buffer
Yann Droneaud ydrone...@opteya.com writes:
The SHA context is holding a temporary buffer for partial block.
This block must 64 bytes long. It is currently described as
an array of 16 integers.
Signed-off-by: Yann Droneaud ydrone...@opteya.com
---
As we do not work with 16-bit integers anyway, 16 integers occupy 64
bytes anyway.
What problem does this series fix?
block-sha1/sha1.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h
index b864df6..d29ff6a 100644
--- a/block-sha1/sha1.h
+++ b/block-sha1/sha1.h
@@ -9,7 +9,7 @@
typedef struct {
unsigned long long size;
unsigned int H[5];
- unsigned int W[16];
+ unsigned char W[64];
} blk_SHA_CTX;
void blk_SHA1_Init(blk_SHA_CTX *ctx);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] sha1: use char type for temporary work buffer
Le mercredi 12 septembre 2012 à 11:42 -0700, Junio C Hamano a écrit : Yann Droneaud ydrone...@opteya.com writes: The SHA context is holding a temporary buffer for partial block. This block must 64 bytes long. It is currently described as an array of 16 integers. Signed-off-by: Yann Droneaud ydrone...@opteya.com --- As we do not work with 16-bit integers anyway, 16 integers occupy 64 bytes anyway. It's unclear why this array is declared as 'int' but used as 'char'. What problem does this series fix? The question I was hoping to not see asked :) This is mostly some cosmetic fixes to improve readability and portability. Regards -- Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] sha1: use char type for temporary work buffer
Le mercredi 12 septembre 2012 à 14:38 -0400, Jeff King a écrit :
On Wed, Sep 12, 2012 at 12:30:45PM +0200, Yann Droneaud wrote:
The SHA context is holding a temporary buffer for partial block.
This block must 64 bytes long. It is currently described as
an array of 16 integers.
Signed-off-by: Yann Droneaud ydrone...@opteya.com
---
block-sha1/sha1.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h
index b864df6..d29ff6a 100644
--- a/block-sha1/sha1.h
+++ b/block-sha1/sha1.h
@@ -9,7 +9,7 @@
typedef struct {
unsigned long long size;
unsigned int H[5];
- unsigned int W[16];
+ unsigned char W[64];
} blk_SHA_CTX;
Wouldn't this break all of the code that is planning to index W by
32-bit words (see the definitions of setW in block-sha1/sha1.c)?
That's not the same W ... This part of the code is indeed unclear.
You do not describe an actual problem in the commit message, but reading
between the lines it would be system X would like to use block-sha1,
but has an unsigned int that is not 32 bits. IOW, an ILP64 type of
architecture. Do you have some specific platform in mind?
If that is indeed the problem, wouldn't the simplest fix be using
uint32_t instead of unsigned int?
It's another way to fix this oddity, but not simpler.
Moreover, would that be sufficient to run on such a platform? At the
very least, H above would want the same treatment. And I would not be
surprised if some of the actual code in block-sha1/sha1.c needed
updating, as well.
ctx-H is actually used as an array of integer, so it would benefits of
being declared uint32_t for an ILP64 system. This fix would also be
required for blk_SHA1_Block() function.
Regards.
--
Yann Droneaud
OPTEYA
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
