From: Yann Droneaud <ydroneaud@opteya•com>
To: Jeff King <peff@peff•net>
Cc: git@vger•kernel.org, Junio C Hamano <gitster@pobox•com>
Subject: Re: [PATCH 3/3] sha1: use char type for temporary work buffer
Date: Thu, 13 Sep 2012 08:11:02 +0200 [thread overview]
Message-ID: <1347516662.1961.23.camel@test.quest-ce.net> (raw)
In-Reply-To: <20120912210455.GA30679@sigill.intra.peff.net>
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
next prev parent reply other threads:[~2012-09-13 6:11 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-12 10:01 [PATCH 0/3] Janitor minor fixes on SHA1 Yann Droneaud
2012-09-12 10:01 ` [PATCH 1/3] sha1: update pointer and remaining length after subfunction call Yann Droneaud
2012-09-12 18:35 ` Junio C Hamano
2012-09-12 10:21 ` [PATCH 2/3] sha1: clean pointer arithmetic Yann Droneaud
2012-09-12 18:37 ` Junio C Hamano
2012-09-12 20:50 ` Yann Droneaud
2012-09-12 21:25 ` Bruce Korb
2012-09-12 10:30 ` [PATCH 3/3] sha1: use char type for temporary work buffer Yann Droneaud
2012-09-12 18:38 ` Jeff King
2012-09-12 20:37 ` Yann Droneaud
2012-09-12 21:04 ` Jeff King
2012-09-13 6:11 ` Yann Droneaud [this message]
2012-09-12 18:42 ` Junio C Hamano
2012-09-12 20:42 ` Yann Droneaud
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1347516662.1961.23.camel@test.quest-ce.net \
--to=ydroneaud@opteya$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox