[PATCH 0/3] Janitor minor fixes on SHA1

[PATCH 0/3] Janitor minor fixes on SHA1

[Yann Droneaud]

While looking to use the git SHA1 code, I've found some small oddities.
Please find little cosmetics fixes for them.

The patches are against 'next' and can be merged in one single patch
if needed.

Yann Droneaud (3):
  sha1: update pointer and remaining length after subfunction call
  sha1: clean pointer arithmetic
  sha1: use char type for temporary work buffer

 block-sha1/sha1.c | 6 +++---
 block-sha1/sha1.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
1.7.11.4

[PATCH 1/3] sha1: update pointer and remaining length after subfunction call

[Yann Droneaud]

There's no need to update the pointer and remaining length before
leaving or calling the SHA1 sub function.

Additionnaly, the partial block code could be looking more like
the full block handling branch.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 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 a8d4bf9..c1af112 100644
--- a/block-sha1/sha1.c
+++ b/block-sha1/sha1.c
@@ -248,11 +248,11 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len)
 			left = len;
 		memcpy(lenW + (char *)ctx->W, data, left);
 		lenW = (lenW + left) & 63;
-		len -= left;
-		data = ((const char *)data + left);
 		if (lenW)
 			return;
 		blk_SHA1_Block(ctx, ctx->W);
+		data = ((const char *)data + left);
+		len -= left;
 	}
 	while (len >= 64) {
 		blk_SHA1_Block(ctx, data);
-- 
1.7.11.4

[PATCH 2/3] sha1: clean pointer arithmetic

[Yann Droneaud]

One memcpy() argument is computed from a pointer added to an integer:
eg. int + pointer. It's unusal.
Let's write it in the correct order: pointer + offset.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 block-sha1/sha1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
index c1af112..a7c4470 100644
--- a/block-sha1/sha1.c
+++ b/block-sha1/sha1.c
@@ -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(lenW + (char *)ctx->W, data, left);
+		memcpy((char *)ctx->W + lenW, data, left);
 		lenW = (lenW + left) & 63;
 		if (lenW)
 			return;
-- 
1.7.11.4

[PATCH 3/3] sha1: use char type for temporary work buffer

[Yann Droneaud]

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 <ydroneaud@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

Re: [PATCH 1/3] sha1: update pointer and remaining length after subfunction call

[Junio C Hamano]

Yann Droneaud <ydroneaud@opteya.com> writes:

> There's no need to update the pointer and remaining length before
> leaving or calling the SHA1 sub function.
>
> Additionnaly, the partial block code could be looking more like
> the full block handling branch.
>
> Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
> ---
>  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 a8d4bf9..c1af112 100644
> --- a/block-sha1/sha1.c
> +++ b/block-sha1/sha1.c
> @@ -248,11 +248,11 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len)
>  			left = len;
>  		memcpy(lenW + (char *)ctx->W, data, left);
>  		lenW = (lenW + left) & 63;
> -		len -= left;
> -		data = ((const char *)data + left);
>  		if (lenW)
>  			return;
>  		blk_SHA1_Block(ctx, ctx->W);
> +		data = ((const char *)data + left);
> +		len -= left;
>  	}
>  	while (len >= 64) {
>  		blk_SHA1_Block(ctx, data);

It is not wrong per-se, but doesn't the compiler optimize it out if
this is worth doing?  Just being curious.

Re: [PATCH 2/3] sha1: clean pointer arithmetic

[Junio C Hamano]

Yann Droneaud <ydroneaud@opteya.com> writes:

> One memcpy() argument is computed from a pointer added to an integer:
> eg. int + pointer. It's unusal.
> Let's write it in the correct order: pointer + offset.

Meh.

Both are correct.  Aren't ctx->w[lenW] and lenW[ctx-w] both correct,
even?


>
> Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
> ---
>  block-sha1/sha1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
> index c1af112..a7c4470 100644
> --- a/block-sha1/sha1.c
> +++ b/block-sha1/sha1.c
> @@ -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(lenW + (char *)ctx->W, data, left);
> +		memcpy((char *)ctx->W + lenW, data, left);
>  		lenW = (lenW + left) & 63;
>  		if (lenW)
>  			return;

Re: [PATCH 3/3] sha1: use char type for temporary work buffer

[Jeff King]

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 <ydroneaud@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

Re: [PATCH 3/3] sha1: use char type for temporary work buffer

[Junio C Hamano]

Yann Droneaud <ydroneaud@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 <ydroneaud@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);

Re: [PATCH 3/3] sha1: use char type for temporary work buffer

[Yann Droneaud]

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 <ydroneaud@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

Re: [PATCH 3/3] sha1: use char type for temporary work buffer

[Yann Droneaud]

Le mercredi 12 septembre 2012 à 11:42 -0700, Junio C Hamano a écrit :
> Yann Droneaud <ydroneaud@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 <ydroneaud@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

Re: [PATCH 2/3] sha1: clean pointer arithmetic

[Yann Droneaud]

Le mercredi 12 septembre 2012 à 11:37 -0700, Junio C Hamano a écrit :
> Yann Droneaud <ydroneaud@opteya.com> writes:
> 
> > One memcpy() argument is computed from a pointer added to an integer:
> > eg. int + pointer. It's unusal.
> > Let's write it in the correct order: pointer + offset.
> 
> Meh.
> 
> Both are correct.  Aren't ctx->w[lenW] and lenW[ctx-w] both correct,
> even?
> 

"correct" in my commit log message should be read as "the way it's used
by most C developer".

It's again a cosmetic fix.

-- 
Yann Droneaud

Re: [PATCH 3/3] sha1: use char type for temporary work buffer

[Jeff King]

On Wed, Sep 12, 2012 at 10:37:10PM +0200, Yann Droneaud wrote:

> > > 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.

Sorry, you're right, that's a different work array (though it has the
identical issue, no?). But the point still stands.  Did you audit the
block-sha1 code to make sure nobody is ever indexing the W array? If you
didn't, then your change is not safe. If you did, then you should really
mention that in the commit message.

> > 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).

> > 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?

-Peff

Re: [PATCH 2/3] sha1: clean pointer arithmetic

[Bruce Korb]

On Wed, Sep 12, 2012 at 1:50 PM, Yann Droneaud <ydroneaud@opteya.com> wrote:
>> Both are correct.  Aren't ctx->w[lenW] and lenW[ctx-w] both correct,
>> even?
>>
>
> "correct" in my commit log message should be read as "the way it's used
> by most C developer".
>
> It's again a cosmetic fix.

It's a maintenance fix.  The fewer distractions, the easier it is to understand.
"lenW[ctx-w]" is distracting.

Re: [PATCH 3/3] sha1: use char type for temporary work buffer

[Yann Droneaud]

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