Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list] 
Message-ID: <20130731223454.GB31179@openwall.com>
Date: Thu, 1 Aug 2013 02:34:54 +0400
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: Parallella: bcrypt

Hi Katja,

On Wed, Jul 31, 2013 at 11:25:21PM +0200, Katja Malvoni wrote:
> Alexander, I'm currently working on everything you mentioned so far.

Here's some more work for you:

Your source code indentation is currently a bit inconsistent - both
within your own code and against the rest of JtR.  We're using 8 char
tabs in JtR, whereas your parallella_e_bcrypt.S might have been written
under a different assumption (judging by how some of the #define's look
misaligned against nearby ones when I view this file with 8 char tabs).
Also, your assembly instructions are indented by two tabs, whereas we
use just one tab for that in the rest of *.S files in JtR.

In parallella_e_bcrypt.c: BF_crypt(), in this loop:

	do {
		int done;
		for(i = 0; i < n; i++)
		{
			current[i].ctx.s.P[0] ^= current[i].expanded_key[0];
[...]
			current[i].ctx.s.P[17] ^= current[i].expanded_key[17];
		}

	#ifdef interleave
			BF_encrypt2(&current[0].ctx, &current[1].ctx);

The "done" variable is unused and the code following the for loop (that
is, the line BF_encrypt2(...) above and what follows it) are incorrectly
indented (should be one tab to the left).

Also, in the rest of JtR we place opening curly braces on the same line
with "if", "while", "for", "do" (as you can see with "do" above),
"struct", "union".  The placement on a new line is only for function
declarations, as well as when a new block is used inside a function in
order to limit variable scope.

And we separate "if", "while", "for" from the following opening brace
with a space character.

In C version of BF_encrypt2() you have this piece of code:

		L0 = tmpa4 ^ P0[17];
		L1 = tmpb4 ^ P1[17];
		*ptr = L0; \
		*(ptr + 1) = R0; \
		*(ptr + (ctx1->s.P - ctx0->s.P)) = L1; \
		*(ptr + (ctx1->s.P - ctx0->s.P) + 1) = R1; \
		ptr += 2; \

In it, the trailing " \" on some of the lines are not needed - perhaps
you moved these lines from a #define and forgot to remove these trailing
chars.

There are also some empty lines that don't appear to be there on purpose
(do not appear to make the code more readable) - although indeed some
other empty lines do help.

Overall, what I am saying is that some source code cleanup would be nice.

Thanks,

Alexander

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.