From: Jeff King <peff@peff.net>
To: Yann Droneaud <ydroneaud@opteya.com>
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: Wed, 12 Sep 2012 17:04:56 -0400 [thread overview]
Message-ID: <20120912210455.GA30679@sigill.intra.peff.net> (raw)
In-Reply-To: <1347482230.1961.4.camel@test.quest-ce.net>
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
next prev parent reply other threads:[~2012-09-12 21:05 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 [this message]
2012-09-13 6:11 ` Yann Droneaud
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=20120912210455.GA30679@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=ydroneaud@opteya.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).