Re: [PATCH 3/3] sha1: use char type for temporary work buffer
- From
Jeff King <peff@peff.net>
- Date
- Sep 12, 2012, 21:04 UTC
- Message-ID
- <20120912210455.GA30679@sigill.intra.peff.net>
- In-Reply-To
- <1347482230.1961.4.camel@test.quest-ce.net>
On Wed, Sep 12, 2012 at 10:37:10PM +0200, Yann Droneaud wrote:
Show 16 quoted lines
> > > 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).
Show 8 quoted lines
> > 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