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

[Shawn Landden <shawn at churchofgit dot com>]

On Fri, Mar 27, 2015 at 11:46:28PM -0400, Mike Frysinger wrote:
> On 27 Mar 2015 14:39, Shawn Landden wrote:
> > --- a/configure.ac
> > +++ b/configure.ac
> >
> > -  nss_includes=-I$(nss-config --includedir 2>/dev/null)
> > +  nss_includes=$(pkg-config --cflags-only-I nss)
> 
> you cannot hardcode `pkg-config`.  you must respect $PKG_CONFIG.  normally 
> you'd use PKG_PROG_PKG_CONFIG, but that'd require an external pkg.m4 to be 
> available, so do this instead earlier in the file:
> 	AC_PATH_TOOL([PKG_CONFIG], [pkg-config])
We should actually just drop libnss3 support (netscape securiycurity services, libfreebl3)
as it doesn't make much sense to export these symbols and then use NSS's versions.
> 
> > --- /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.

It would be poor API design to only allow writing to aligned addresses when the write
is short and minor compared to the overhead of the hash itsself.
 
> 
> > --- /dev/null
> > +++ b/crypt/sha2.h
> >
> > +typedef struct {
> > +  char __internal_state[176];
> > +} sha256_ctx __attribute__((aligned(16)));
> 
> this wasn't a problem before because you weren't installing the header, but now 
> that you are, you have to use __aligned__.
> 
> applies below too.
> 
> > +/* sha256() writes 32 bytes to md and returns md.
> > + * sha256_finish() writes 32 bytes to md and returns md.
> > + * sha224_finish() writes 28 bytes to md and returns md.
> > + * sha256_update() returns d.*/
> > +void *sha256(const void *__restrict d, size_t n, void *__restrict md);
> > +void sha256_init(sha256_ctx *s);
> > +const void *sha256_update(sha256_ctx *__restrict s, const void *__restrict d, size_t n);
> > +void *sha256_finish(sha256_ctx *__restrict s, void *__restrict md);
> > +void *sha224_finish(sha256_ctx *__restrict s, void *__restrict md);
> 
> comments about the api should be split before each function rather than one big 
> block.  you should also omit the names (like "d" or "n") as those cause 
> namespace issues.
> 
> applies below too.
> 
> 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.
> 
> i did see some clean ups we might want to split out & merge regardless of 
> exporting the sha2 API (like deleting the SWAP ugliness).  although, considering 
> glibc already has this code in it, i don't have a problem with exporting them as 
> proper symbols (assuming the API is reasonable).
> -mike