Re: [PATCH] sha2: new header <sha2.h>

[Mike Frysinger <vapier at gentoo dot org>]

On 28 Mar 2015 05:55, Shawn Landden wrote:
> On Fri, Mar 27, 2015 at 11:46:28PM -0400, Mike Frysinger wrote:
> > On 27 Mar 2015 14:39, Shawn Landden wrote:
> > > --- /dev/null
> > > +++ b/crypt/align.h
> > >
> > > +#define put_be32(p, v)	do { *(uint32_t *)(p) = be32toh(v); } while (0)
> > > +#define put_be64(p, v)	do { *(uint64_t *)(p) = be64toh(v); } while (0)
> > > +#define put_be32(p, v)	do { \
> > > +	unsigned int __v = (v); \
> > > +	*((unsigned char *)(p) + 0) = __v >> 24; \
> > > +	*((unsigned char *)(p) + 1) = __v >> 16; \
> > > +	*((unsigned char *)(p) + 2) = __v >>  8; \
> > > +	*((unsigned char *)(p) + 3) = __v >>  0; } while (0)
> > > +#define put_be64(p, v)	do { \
> > > +	unsigned int __v = (v); \
> > > +	*((unsigned char *)(p) + 0) = __v >> 56; \
> > > +	*((unsigned char *)(p) + 1) = __v >> 48; \
> > > +	*((unsigned char *)(p) + 2) = __v >> 40; \
> > > +	*((unsigned char *)(p) + 3) = __v >> 32; \
> > > +	*((unsigned char *)(p) + 4) = __v >> 24; \
> > > +	*((unsigned char *)(p) + 5) = __v >> 16; \
> > > +	*((unsigned char *)(p) + 6) = __v >>  8; \
> > > +	*((unsigned char *)(p) + 7) = __v >>  0; } while (0)
> > 
> > these are like ... really bad.  they violate strict aliasing so hard.  just use 
> > the macros already available in endian.h.  like htobe32 and htobe64.  which you 
> > seem to use elsewhere in this patch already ;).
>
> Those don't provide unaligned stores, or have anything to do with the alignment
> issue I am dealing with here. And they are writing to a passed void * in
> public function so the compiler can't do strict aliasing optimizations, so that is moot.

i was thinking the pointer type was higher than void*.

my concern wasn't related to alignment.  it's that this code is ugly as sin, and 
broken.  sizeof(int) is not guaranteed to be >=sizeof(int64) which is what this 
assumes, and you expand (p) multiple times.

the endian swaps won't help as they only take one arg ... was thinking the API 
was different.  for big endian hosts, this is a simple call to memcpy (and gcc 
can handle all the various arch details like direct stores on unaligned arches).

this API isn't specific to crypto.  if you're going to make a header out of it, 
should be merged into an existing endian.h imo.  include/endian.h has the 
advantage of not being exported.

> > you should also update the manual to document these.
>
> Do people actually use info? Anyway yes, and when it goes it I will submit a patch to man-pages.

people use HTML everyday
https://www.gnu.org/software/libc/manual/
-mike

Attachment: signature.asc
Description: Digital signature