Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 24 Sep 2014 01:31:16 +0530
From: Sayantan Datta <std2048@...il.com>
To: john-dev <john-dev@...ts.openwall.com>
Subject: Re: Restart work on mask mode

Hi magnum,

On Mon, Sep 22, 2014 at 2:09 AM, Solar Designer <solar@...nwall.com> wrote:

> Sayantan,
>
> On Mon, Sep 22, 2014 at 12:22:22AM +0400, Solar Designer wrote:
> > Just one detail I am actually able to communicate to you easily: please
> > don't use nested functions.
>
> Oh, here's some more:
>
> Please don't forget to add yourself as a copyright holder for this
> source file.
>
> This (commented out) line is weird:
>
> //#include <string.h> /* for isalpha(), isxdigit() */
>
> Those macros actually come from <ctype.h>, not <string.h>.
>
> These lines should have spaces added:
>
> #include<string.h>
> #include<stdlib.h>
>
> The indentation levels are excessive - ideally, you'd re-arrange the
> code such that you would not need to nest blocks this deeply.
>
> Closely related to the above: many lines are overly long.  In the rest
> of JtR, I tried to keep lines in under 80 chars, and that's with 8 char
> tabs.  Sure, this is often difficult to do, but it provides a reminder
> to restructure the code (usually to split out more portions of it into
> functions) before it becomes too complex.
>
> Some curly braces are mis-indented, most commonly before "else", e.g.:
>
>                 end_c[k] = strtol(str, NULL, 16);
>                 }
>         else {
>                 dash_loc_beg[k] = j - 1;
>
> would be corrected to:
>
>                 end_c[k] = strtol(str, NULL, 16);
>         } else {
>                 dash_loc_beg[k] = j - 1;
>
> You pass potentially negative numbers into ctype macros:
>
> static void init_cpu_mask(char *mask, [...]
> [...]
>         isxdigit((int)mask[j - 2]) [...]
>
> This causes sign extension.  You need to cast to (int)(unsigned char)
> (yes, two casts in a row), or avoid ctype macros there (but don't make
> the same error with your own array lookups!)
>
> In many places (but not in all), you have no whitespace between "if" or
> "for" and the following opening brace.  Elsewhere in JtR (and in the
> rest of instances in this same source file), we use "if (" and "for (",
> etc. with the whitespace.
>
> The commented out portions of code need to be fully dropped, or at least
> turned into #if 0 ... #endif blocks.  They are not comments.
>
> Alexander
>

Please test the new mask mode cracker and apply it to bleeding-jumbo, if
you are comfortable.

Regards,
Sayantan

[ CONTENT OF TYPE text/html SKIPPED ]

[ CONTENT OF TYPE application/x-gzip SKIPPED ]

Powered by blists - more mailing lists

Your e-mail address:

Powered by Openwall GNU/*/Linux - Powered by OpenVZ