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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html