bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: arcfour


From: Ralf Wildenhues
Subject: Re: arcfour
Date: Fri, 14 Oct 2005 09:10:24 +0200
User-agent: Mutt/1.5.11

Hi Simon,

* Simon Josefsson wrote on Fri, Oct 14, 2005 at 01:12:59AM CEST:
> How about this?

Some style comments, if I may..

> Index: lib/arcfour.c
> ===================================================================
> RCS file: lib/arcfour.c
> diff -N lib/arcfour.c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ lib/arcfour.c     13 Oct 2005 23:12:01 -0000
*snip*
> +
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +
> +#include "arcfour.h"
> +
> +void
> +arcfour_stream (arcfour_context * context, const char *inbuf, char *outbuf,
> +             size_t length)
> +{
> +  register size_t i = context->idx_i;
> +  register size_t j = context->idx_j;
> +  register unsigned char *sbox = context->sbox;
> +  register unsigned char t;
> +
> +  while (length--)
> +    {
> +      i++;
> +      i = i & 255;   /* The and-op seems to be faster than the mod-op. */

Yes (for less-than-commonly-optimizing compilers), but you could also be
consistent in notation here, and two lines down the road.

Furthermore, if I may suggest not to sprinkle hard-coded numbers all
over the place.  How about something like this
  #define ARCFOUR_BLOCKBITS 8
  #define ARCFOUR_BLOCKSIZE (1 << ARCFOUR_BLOCKBITS)
  #define ARCFOUR_BLOCKMASK (ARCFOUR_BLOCKSIZE - 1)

in the header, and then use the latter two consistently?
If you don't like long names, maybe s/BLOCK//g.

> +      j += sbox[i];
> +      j &= 255;
> +      t = sbox[i];
> +      sbox[i] = sbox[j];
> +      sbox[j] = t;
> +      *outbuf++ = *inbuf++ ^ sbox[(sbox[i] + sbox[j]) & 255];
> +    }
> +
> +  context->idx_i = i;
> +  context->idx_j = j;
> +}
> +
> +void
> +arcfour_setkey (arcfour_context * context, const char *key, size_t keylen)
> +{
> +  int i, j;
> +  unsigned char karr[256];
> +
> +  context->idx_i = context->idx_j = 0;
> +  for (i = 0; i < 256; i++)
> +    context->sbox[i] = i;
> +  for (i = 0; i < 256; i++)
> +    karr[i] = key[i % keylen];

If this were time-critical, you could hand-maintain another index k and
omit the modulo.  I also wonder whether merging the karr loop into the
next one (and thus eliding karr completely) isn't beneficial.  Like
this (untested):

for (i = j = k = 0; i < ARCFOUR_BLOCKSIZE; i++)
  {
    int t;
    j = (j + context->sbox[i] + key[k]) & ARCFOUR_BLOCKMASK;
    t = context->sbox[i];
    context->sbox[i] = context->sbox[j];
    context->sbox[j] = t;
    if (++k == keylen)
      k = 0;
  }
}

Furthermore, I guess it would be beneficial if you would test this and
the other modules you recently added with test strings that are longer
than respective block sizes.  :)

> +  for (i = j = 0; i < 256; i++)
> +    {
> +      int t;
> +      j = (j + context->sbox[i] + karr[i]) % 256;
> +      t = context->sbox[i];
> +      context->sbox[i] = context->sbox[j];
> +      context->sbox[j] = t;
> +    }
> +}
*snip*

Cheers,
Ralf




reply via email to

[Prev in Thread] Current Thread [Next in Thread]