{"thread":{"id":"31510","subject":"[PATCH 0/3] Janitor minor fixes on SHA1","startedAt":"2012-09-12T10:01:24Z","lastAt":"2012-09-13T06:11:02Z","messageCount":14,"participants":["Yann Droneaud","Junio C Hamano","Jeff King","Bruce Korb"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"198823","messageId":"cover.1347442430.git.ydroneaud@opteya.com","threadId":"31510","inReplyTo":null,"subject":"[PATCH 0/3] Janitor minor fixes on SHA1","fromName":"Yann Droneaud","fromEmail":"ydroneaud@opteya.com","sentAt":"2012-09-12T10:01:24Z","receivedAt":"2012-09-12T10:01:24Z","isPatch":true,"sender":{"key":"ydroneaud@opteya.com","avatar":"https://avatars.githubusercontent.com/u/881377?v=4"},"body":"While looking to use the git SHA1 code, I've found some small oddities.\nPlease find little cosmetics fixes for them.\n\nThe patches are against 'next' and can be merged in one single patch\nif needed.\n\nYann Droneaud (3):\n sha1: update pointer and remaining length after subfunction call\n sha1: clean pointer arithmetic\n sha1: use char type for temporary work buffer\n\n block-sha1/sha1.c | 6 +++---\n block-sha1/sha1.h | 2 +-\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\n-- \n1.7.11.4\n"},{"id":"198824","messageId":"e6f0051409811fc57385ae712d3256772044bf09.1347442430.git.ydroneaud@opteya.com","threadId":"31510","inReplyTo":"cover.1347442430.git.ydroneaud@opteya.com","subject":"[PATCH 1/3] sha1: update pointer and remaining length after subfunction call","fromName":"Yann Droneaud","fromEmail":"ydroneaud@opteya.com","sentAt":"2012-09-12T10:01:25Z","receivedAt":"2012-09-12T10:01:25Z","isPatch":true,"sender":{"key":"ydroneaud@opteya.com","avatar":"https://avatars.githubusercontent.com/u/881377?v=4"},"body":"There's no need to update the pointer and remaining length before\nleaving or calling the SHA1 sub function.\n\nAdditionnaly, the partial block code could be looking more like\nthe full block handling branch.\n\nSigned-off-by: Yann Droneaud \n---\n block-sha1/sha1.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex a8d4bf9..c1af112 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -248,11 +248,11 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len)\n \t\t\tleft = len;\n \t\tmemcpy(lenW + (char *)ctx->W, data, left);\n \t\tlenW = (lenW + left) & 63;\n-\t\tlen -= left;\n-\t\tdata = ((const char *)data + left);\n \t\tif (lenW)\n \t\t\treturn;\n \t\tblk_SHA1_Block(ctx, ctx->W);\n+\t\tdata = ((const char *)data + left);\n+\t\tlen -= left;\n \t}\n \twhile (len >= 64) {\n \t\tblk_SHA1_Block(ctx, data);\n-- \n1.7.11.4\n"},{"id":"198828","messageId":"20dce012a57900b61e51c0e0d8dfb5573693010e.1347442430.git.ydroneaud@opteya.com","threadId":"31510","inReplyTo":"cover.1347442430.git.ydroneaud@opteya.com","subject":"[PATCH 2/3] sha1: clean pointer arithmetic","fromName":"Yann Droneaud","fromEmail":"ydroneaud@opteya.com","sentAt":"2012-09-12T10:21:13Z","receivedAt":"2012-09-12T10:21:13Z","isPatch":true,"sender":{"key":"ydroneaud@opteya.com","avatar":"https://avatars.githubusercontent.com/u/881377?v=4"},"body":"One memcpy() argument is computed from a pointer added to an integer:\neg. int + pointer. It's unusal.\nLet's write it in the correct order: pointer + offset.\n\nSigned-off-by: Yann Droneaud \n---\n block-sha1/sha1.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex c1af112..a7c4470 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -246,7 +246,7 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len)\n \t\tunsigned int left = 64 - lenW;\n \t\tif (len < left)\n \t\t\tleft = len;\n-\t\tmemcpy(lenW + (char *)ctx->W, data, left);\n+\t\tmemcpy((char *)ctx->W + lenW, data, left);\n \t\tlenW = (lenW + left) & 63;\n \t\tif (lenW)\n \t\t\treturn;\n-- \n1.7.11.4\n"},{"id":"198827","messageId":"a8c30a998cad6a7b38bd983e7689a628567a8176.1347442430.git.ydroneaud@opteya.com","threadId":"31510","inReplyTo":"cover.1347442430.git.ydroneaud@opteya.com","subject":"[PATCH 3/3] sha1: use char type for temporary work buffer","fromName":"Yann Droneaud","fromEmail":"ydroneaud@opteya.com","sentAt":"2012-09-12T10:30:45Z","receivedAt":"2012-09-12T10:30:45Z","isPatch":true,"sender":{"key":"ydroneaud@opteya.com","avatar":"https://avatars.githubusercontent.com/u/881377?v=4"},"body":"The SHA context is holding a temporary buffer for partial block.\n\nThis block must 64 bytes long. It is currently described as\nan array of 16 integers.\n\nSigned-off-by: Yann Droneaud \n---\n block-sha1/sha1.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/block-sha1/sha1.h b/block-sha1/sha1.h\nindex b864df6..d29ff6a 100644\n--- a/block-sha1/sha1.h\n+++ b/block-sha1/sha1.h\n@@ -9,7 +9,7 @@\n typedef struct {\n \tunsigned long long size;\n \tunsigned int H[5];\n-\tunsigned int W[16];\n+\tunsigned char W[64];\n } blk_SHA_CTX;\n \n void blk_SHA1_Init(blk_SHA_CTX *ctx);\n-- \n1.7.11.4\n"},{"id":"198867","messageId":"7vhar3ccmr.fsf@alter.siamese.dyndns.org","threadId":"31510","inReplyTo":"e6f0051409811fc57385ae712d3256772044bf09.1347442430.git.ydroneaud@opteya.com","subject":"Re: [PATCH 1/3] sha1: update pointer and remaining length after subfunction call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-12T18:35:56Z","receivedAt":"2012-09-12T18:35:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Droneaud writes:\n\n> There's no need to update the pointer and remaining length before\n> leaving or calling the SHA1 sub function.\n>\n> Additionnaly, the partial block code could be looking more like\n> the full block handling branch.\n>\n> Signed-off-by: Yann Droneaud \n> ---\n> block-sha1/sha1.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\n> index a8d4bf9..c1af112 100644\n> --- a/block-sha1/sha1.c\n> +++ b/block-sha1/sha1.c\n> @@ -248,11 +248,11 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len)\n> \t\t\tleft = len;\n> \t\tmemcpy(lenW + (char *)ctx->W, data, left);\n> \t\tlenW = (lenW + left) & 63;\n> -\t\tlen -= left;\n> -\t\tdata = ((const char *)data + left);\n> \t\tif (lenW)\n> \t\t\treturn;\n> \t\tblk_SHA1_Block(ctx, ctx->W);\n> +\t\tdata = ((const char *)data + left);\n> +\t\tlen -= left;\n> \t}\n> \twhile (len >= 64) {\n> \t\tblk_SHA1_Block(ctx, data);\n\nIt is not wrong per-se, but doesn't the compiler optimize it out if\nthis is worth doing? Just being curious.\n"},{"id":"198868","messageId":"7vd31rcck4.fsf@alter.siamese.dyndns.org","threadId":"31510","inReplyTo":"20dce012a57900b61e51c0e0d8dfb5573693010e.1347442430.git.ydroneaud@opteya.com","subject":"Re: [PATCH 2/3] sha1: clean pointer arithmetic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-12T18:37:31Z","receivedAt":"2012-09-12T18:37:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Droneaud writes:\n\n> One memcpy() argument is computed from a pointer added to an integer:\n> eg. int + pointer. It's unusal.\n> Let's write it in the correct order: pointer + offset.\n\nMeh.\n\nBoth are correct. Aren't ctx->w[lenW] and lenW[ctx-w] both correct,\neven?\n\n\n>\n> Signed-off-by: Yann Droneaud \n> ---\n> block-sha1/sha1.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\n> index c1af112..a7c4470 100644\n> --- a/block-sha1/sha1.c\n> +++ b/block-sha1/sha1.c\n> @@ -246,7 +246,7 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len)\n> \t\tunsigned int left = 64 - lenW;\n> \t\tif (len < left)\n> \t\t\tleft = len;\n> -\t\tmemcpy(lenW + (char *)ctx->W, data, left);\n> +\t\tmemcpy((char *)ctx->W + lenW, data, left);\n> \t\tlenW = (lenW + left) & 63;\n> \t\tif (lenW)\n> \t\t\treturn;\n"},{"id":"198869","messageId":"20120912183833.GA20795@sigill.intra.peff.net","threadId":"31510","inReplyTo":"a8c30a998cad6a7b38bd983e7689a628567a8176.1347442430.git.ydroneaud@opteya.com","subject":"Re: [PATCH 3/3] sha1: use char type for temporary work buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-12T18:38:33Z","receivedAt":"2012-09-12T18:38:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 12, 2012 at 12:30:45PM +0200, Yann Droneaud wrote:\n\n> The SHA context is holding a temporary buffer for partial block.\n> \n> This block must 64 bytes long. It is currently described as\n> an array of 16 integers.\n> \n> Signed-off-by: Yann Droneaud \n> ---\n> block-sha1/sha1.h | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h\n> index b864df6..d29ff6a 100644\n> --- a/block-sha1/sha1.h\n> +++ b/block-sha1/sha1.h\n> @@ -9,7 +9,7 @@\n> typedef struct {\n> \tunsigned long long size;\n> \tunsigned int H[5];\n> -\tunsigned int W[16];\n> +\tunsigned char W[64];\n> } blk_SHA_CTX;\n\nWouldn't this break all of the code that is planning to index \"W\" by\n32-bit words (see the definitions of setW in block-sha1/sha1.c)?\n\nYou do not describe an actual problem in the commit message, but reading\nbetween the lines it would be \"system X would like to use block-sha1,\nbut has an \"unsigned int\" that is not 32 bits\". IOW, an ILP64 type of\narchitecture. Do you have some specific platform in mind?\n\nIf that is indeed the problem, wouldn't the simplest fix be using\nuint32_t instead of \"unsigned int\"?\n\nMoreover, would that be sufficient to run on such a platform? At the\nvery least, \"H\" above would want the same treatment. And I would not be\nsurprised if some of the actual code in block-sha1/sha1.c needed\nupdating, as well.\n\n-Peff\n"},{"id":"198870","messageId":"7v8vcfccbn.fsf@alter.siamese.dyndns.org","threadId":"31510","inReplyTo":"a8c30a998cad6a7b38bd983e7689a628567a8176.1347442430.git.ydroneaud@opteya.com","subject":"Re: [PATCH 3/3] sha1: use char type for temporary work buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-12T18:42:36Z","receivedAt":"2012-09-12T18:42:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Droneaud writes:\n\n> The SHA context is holding a temporary buffer for partial block.\n>\n> This block must 64 bytes long. It is currently described as\n> an array of 16 integers.\n>\n> Signed-off-by: Yann Droneaud \n> ---\n\nAs we do not work with 16-bit integers anyway, 16 integers occupy 64\nbytes anyway.\n\nWhat problem does this series fix?\n\n> block-sha1/sha1.h | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h\n> index b864df6..d29ff6a 100644\n> --- a/block-sha1/sha1.h\n> +++ b/block-sha1/sha1.h\n> @@ -9,7 +9,7 @@\n> typedef struct {\n> \tunsigned long long size;\n> \tunsigned int H[5];\n> -\tunsigned int W[16];\n> +\tunsigned char W[64];\n> } blk_SHA_CTX;\n> \n> void blk_SHA1_Init(blk_SHA_CTX *ctx);\n"},{"id":"198882","messageId":"1347482230.1961.4.camel@test.quest-ce.net","threadId":"31510","inReplyTo":"20120912183833.GA20795@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] sha1: use char type for temporary work buffer","fromName":"Yann Droneaud","fromEmail":"ydroneaud@opteya.com","sentAt":"2012-09-12T20:37:10Z","receivedAt":"2012-09-12T20:37:10Z","isPatch":true,"sender":{"key":"ydroneaud@opteya.com","avatar":"https://avatars.githubusercontent.com/u/881377?v=4"},"body":"Le mercredi 12 septembre 2012 à 14:38 -0400, Jeff King a écrit :\n> On Wed, Sep 12, 2012 at 12:30:45PM +0200, Yann Droneaud wrote:\n> \n> > The SHA context is holding a temporary buffer for partial block.\n> > \n> > This block must 64 bytes long. It is currently described as\n> > an array of 16 integers.\n> > \n> > Signed-off-by: Yann Droneaud \n> > ---\n> > block-sha1/sha1.h | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > \n> > diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h\n> > index b864df6..d29ff6a 100644\n> > --- a/block-sha1/sha1.h\n> > +++ b/block-sha1/sha1.h\n> > @@ -9,7 +9,7 @@\n> > typedef struct {\n> > \tunsigned long long size;\n> > \tunsigned int H[5];\n> > -\tunsigned int W[16];\n> > +\tunsigned char W[64];\n> > } blk_SHA_CTX;\n> \n> Wouldn't this break all of the code that is planning to index \"W\" by\n> 32-bit words (see the definitions of setW in block-sha1/sha1.c)?\n> \n\nThat's not the same \"W\" ... This part of the code is indeed unclear.\n\n> You do not describe an actual problem in the commit message, but reading\n> between the lines it would be \"system X would like to use block-sha1,\n> but has an \"unsigned int\" that is not 32 bits\". IOW, an ILP64 type of\n> architecture. Do you have some specific platform in mind?\n> \n> If that is indeed the problem, wouldn't the simplest fix be using\n> uint32_t instead of \"unsigned int\"?\n> \n\nIt's another way to fix this oddity, but not simpler.\n\n\n> Moreover, would that be sufficient to run on such a platform? At the\n> very least, \"H\" above would want the same treatment. And I would not be\n> surprised if some of the actual code in block-sha1/sha1.c needed\n> updating, as well.\n> \n\nctx->H is actually used as an array of integer, so it would benefits of\nbeing declared uint32_t for an ILP64 system. This fix would also be\nrequired for blk_SHA1_Block() function.\n\nRegards.\n\n-- \nYann Droneaud\nOPTEYA\n"},{"id":"198879","messageId":"1347482549.1961.9.camel@test.quest-ce.net","threadId":"31510","inReplyTo":"7v8vcfccbn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] sha1: use char type for temporary work buffer","fromName":"Yann Droneaud","fromEmail":"ydroneaud@opteya.com","sentAt":"2012-09-12T20:42:29Z","receivedAt":"2012-09-12T20:42:29Z","isPatch":true,"sender":{"key":"ydroneaud@opteya.com","avatar":"https://avatars.githubusercontent.com/u/881377?v=4"},"body":"Le mercredi 12 septembre 2012 à 11:42 -0700, Junio C Hamano a écrit :\n> Yann Droneaud writes:\n> \n> > The SHA context is holding a temporary buffer for partial block.\n> >\n> > This block must 64 bytes long. It is currently described as\n> > an array of 16 integers.\n> >\n> > Signed-off-by: Yann Droneaud \n> > ---\n> \n> As we do not work with 16-bit integers anyway, 16 integers occupy 64\n> bytes anyway.\n> \n\nIt's unclear why this array is declared as 'int' but used as 'char'.\n\n> What problem does this series fix?\n> \n\nThe question I was hoping to not see asked :)\nThis is mostly some cosmetic fixes to improve readability and\nportability.\n\nRegards\n\n-- \nYann Droneaud\nOPTEYA\n"},{"id":"198881","messageId":"1347483030.1961.13.camel@test.quest-ce.net","threadId":"31510","inReplyTo":"7vd31rcck4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] sha1: clean pointer arithmetic","fromName":"Yann Droneaud","fromEmail":"ydroneaud@opteya.com","sentAt":"2012-09-12T20:50:30Z","receivedAt":"2012-09-12T20:50:30Z","isPatch":true,"sender":{"key":"ydroneaud@opteya.com","avatar":"https://avatars.githubusercontent.com/u/881377?v=4"},"body":"Le mercredi 12 septembre 2012 à 11:37 -0700, Junio C Hamano a écrit :\n> Yann Droneaud writes:\n> \n> > One memcpy() argument is computed from a pointer added to an integer:\n> > eg. int + pointer. It's unusal.\n> > Let's write it in the correct order: pointer + offset.\n> \n> Meh.\n> \n> Both are correct. Aren't ctx->w[lenW] and lenW[ctx-w] both correct,\n> even?\n> \n\n\"correct\" in my commit log message should be read as \"the way it's used\nby most C developer\".\n\nIt's again a cosmetic fix.\n\n-- \nYann Droneaud\n"},{"id":"198884","messageId":"20120912210455.GA30679@sigill.intra.peff.net","threadId":"31510","inReplyTo":"1347482230.1961.4.camel@test.quest-ce.net","subject":"Re: [PATCH 3/3] sha1: use char type for temporary work buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-12T21:04:56Z","receivedAt":"2012-09-12T21:04:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 12, 2012 at 10:37:10PM +0200, Yann Droneaud wrote:\n\n> > > diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h\n> > > index b864df6..d29ff6a 100644\n> > > --- a/block-sha1/sha1.h\n> > > +++ b/block-sha1/sha1.h\n> > > @@ -9,7 +9,7 @@\n> > > typedef struct {\n> > > \tunsigned long long size;\n> > > \tunsigned int H[5];\n> > > -\tunsigned int W[16];\n> > > +\tunsigned char W[64];\n> > > } blk_SHA_CTX;\n> > \n> > Wouldn't this break all of the code that is planning to index \"W\" by\n> > 32-bit words (see the definitions of setW in block-sha1/sha1.c)?\n> > \n> That's not the same \"W\" ... This part of the code is indeed unclear.\n\nSorry, you're right, that's a different work array (though it has the\nidentical issue, no?). But the point still stands. Did you audit the\nblock-sha1 code to make sure nobody is ever indexing the W array? If you\ndidn't, then your change is not safe. If you did, then you should really\nmention that in the commit message.\n\n> > If that is indeed the problem, wouldn't the simplest fix be using\n> > uint32_t instead of \"unsigned int\"?\n> \n> It's another way to fix this oddity, but not simpler.\n\nIt is simpler in the sense that it does not have any side effects (like\nchanging how every user of the data structure needs to index it).\n\n> > Moreover, would that be sufficient to run on such a platform? At the\n> > very least, \"H\" above would want the same treatment. And I would not be\n> > surprised if some of the actual code in block-sha1/sha1.c needed\n> > updating, as well.\n> \n> ctx->H is actually used as an array of integer, so it would benefits of\n> being declared uint32_t for an ILP64 system. This fix would also be\n> required for blk_SHA1_Block() function.\n\nSo...if we are not ready to run on an ILP system after this change, then\nwhat is the purpose?\n\n-Peff\n"},{"id":"198886","messageId":"CAKRnqNJaG4Nw2AVMmSsweBzeNbkPKYQ+TUuGc_B2M464y6jbHA@mail.gmail.com","threadId":"31510","inReplyTo":"1347483030.1961.13.camel@test.quest-ce.net","subject":"Re: [PATCH 2/3] sha1: clean pointer arithmetic","fromName":"Bruce Korb","fromEmail":"bruce.korb@gmail.com","sentAt":"2012-09-12T21:25:24Z","receivedAt":"2012-09-12T21:25:24Z","isPatch":true,"sender":{"key":"bruce.korb@gmail.com","avatar":"https://gravatar.com/avatar/86d91467dc7cc8466a9d133a7b93a5d21233052017c144c9a6f6f7e5110344d0?d=mp&s=160"},"body":"On Wed, Sep 12, 2012 at 1:50 PM, Yann Droneaud wrote:\n>> Both are correct. Aren't ctx->w[lenW] and lenW[ctx-w] both correct,\n>> even?\n>>\n>\n> \"correct\" in my commit log message should be read as \"the way it's used\n> by most C developer\".\n>\n> It's again a cosmetic fix.\n\nIt's a maintenance fix. The fewer distractions, the easier it is to understand.\n\"lenW[ctx-w]\" is distracting.\n"},{"id":"198899","messageId":"1347516662.1961.23.camel@test.quest-ce.net","threadId":"31510","inReplyTo":"20120912210455.GA30679@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] sha1: use char type for temporary work buffer","fromName":"Yann Droneaud","fromEmail":"ydroneaud@opteya.com","sentAt":"2012-09-13T06:11:02Z","receivedAt":"2012-09-13T06:11:02Z","isPatch":true,"sender":{"key":"ydroneaud@opteya.com","avatar":"https://avatars.githubusercontent.com/u/881377?v=4"},"body":"Le mercredi 12 septembre 2012 à 17:04 -0400, Jeff King a écrit :\n\n> > > Wouldn't this break all of the code that is planning to index \"W\" by\n> > > 32-bit words (see the definitions of setW in block-sha1/sha1.c)?\n> > > \n> > That's not the same \"W\" ... This part of the code is indeed unclear.\n> \n> Sorry, you're right, that's a different work array (though it has the\n> identical issue, no?).\n\nNo, this one is really accessed as int. But would probably benefit being\ndeclared as uint32_t.\n\n> But the point still stands. Did you audit the\n> block-sha1 code to make sure nobody is ever indexing the W array? \n\nYes. It was the first thing to do before changing its definition\n(for alignment purpose especially).\n\n> If you didn't, then your change is not safe. If you did, then you should really\n> mention that in the commit message.\n> \n\nSorry about this.\nI thought having the test suite OK was enough to prove this.\n\n> > > If that is indeed the problem, wouldn't the simplest fix be using\n> > > uint32_t instead of \"unsigned int\"?\n> > \n> > It's another way to fix this oddity, but not simpler.\n> \n> It is simpler in the sense that it does not have any side effects (like\n> changing how every user of the data structure needs to index it).\n> \n\nThere's no other user than blk_SHA1_Update()\n\n> > > Moreover, would that be sufficient to run on such a platform? At the\n> > > very least, \"H\" above would want the same treatment. And I would not be\n> > > surprised if some of the actual code in block-sha1/sha1.c needed\n> > > updating, as well.\n> > \n> > ctx->H is actually used as an array of integer, so it would benefits of\n> > being declared uint32_t for an ILP64 system. This fix would also be\n> > required for blk_SHA1_Block() function.\n> \n> So...if we are not ready to run on an ILP system after this change, then\n> what is the purpose?\n> \n\nReadility: in blk_SHA1_Block(), the ctx->W array is used a 64 bytes len\narray, so, AFAIK, there's no point of having it defined as a 16 int len.\nIt's disturbing while reading the code.\n\nThis could allows us to change the memcpy() call further:\n\n@@ -246,7 +246,7 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void\n*data, unsigned long len)\n unsigned int left = 64 - lenW;\n if (len < left)\n left = len;\n- memcpy((char *)ctx->W + lenW, data, left);\n+ memcpy(ctx->W + lenW, data, left);\n lenW = (lenW + left) & 63;\n if (lenW)\n\nRegards.\n\n-- \nYann Droneaud\nOPTEYA\n"}]}