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