From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/1366 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: Re: crypt* files in crypt directory Date: Wed, 25 Jul 2012 03:57:02 -0400 Message-ID: <20120725075702.GO544@brightrain.aerifal.cx> References: Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Trace: dough.gmane.org 1343206727 27529 80.91.229.3 (25 Jul 2012 08:58:47 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Wed, 25 Jul 2012 08:58:47 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-1369-gllmg-musl=m.gmane.org@lists.openwall.com Wed Jul 25 10:58:46 2012 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1StxQb-00027h-1E for gllmg-musl@plane.gmane.org; Wed, 25 Jul 2012 10:58:45 +0200 Original-Received: (qmail 12170 invoked by uid 550); 25 Jul 2012 08:58:43 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 32558 invoked from network); 25 Jul 2012 07:57:19 -0000 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Xref: news.gmane.org gmane.linux.lib.musl.general:1366 Archived-At: 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 > +#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 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