[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 25 Jul 2012 03:57:02 -0400
From: Rich Felker <dalias@...ifal.cx>
To: musl@...ts.openwall.com
Subject: Re: Re: crypt* files in crypt directory
Hi. Finally got around to reviewing this a bit.. :-)
On Sat, Jul 21, 2012 at 05:23:24PM +0200, Łukasz Sowa wrote:
> --- /dev/null
> +++ b/src/misc/crypt_blowfish.c
> [...]
> +#include <errno.h>
> +#ifndef __set_errno
> +#define __set_errno(val) errno = (val)
> +#endif
Is there a reason for this __set_errno stuff? IMO the code should just
directly assign to errno. This looks like some silly cargo-culting
from glibc...
> +/* Just to make sure the prototypes match the actual definitions */
> +#include <crypt.h>
I'm not sure this is useful; this file does not define any public
interfaces.
> +#ifdef __i386__
> +#define BF_ASM 1
> +#define BF_SCALE 1
> +#elif defined(__x86_64__) || defined(__alpha__) || defined(__hppa__)
> +#define BF_ASM 0
> +#define BF_SCALE 1
> +#else
> +#define BF_ASM 0
> +#define BF_SCALE 0
> +#endif
Is this used/needed for anything? I'm generally opposed to having
different versions of code conditionally compiled for different archs
unless there's a very good reason. It makes it so testing on multiple
archs is required to ensure that there are not bugs.
> +typedef unsigned int BF_word;
> +typedef signed int BF_word_signed;
While it's okay for musl where all targets have 32-bit int, if you
want a type that's 32-bit you should probably be using [u]int32_t, or
if you want the system wordsize then int is wrong and you should be
using long or size_t or something...
> +#if BF_ASM
> +#define BF_body() \
> + _BF_body_r(&data.ctx);
> +#else
This does not seem to exist in the submitted code..
> +char *_crypt_gensalt_blowfish_rn(const char *prefix, unsigned long count,
> + const char *input, int size, char *output, int output_size)
This belongs in the gensalt file, not here. There's no reason every
program that merely wants to authenticate against passwords should
pull in salt-generation code.
Aside from that, the code looks a bit long as-is, but I haven't tried
building it yet and it might just look that way from all the comments.
I'll take a look at size stuff soon..
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
