current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite 
#include <stdio.h>
#include <stdint.h>
#include <assert.h>


typedef uint8_t byte;
typedef struct
{
    byte i, j;
    byte S[256];
} Rc4State;

void swap(byte *a, byte *b)
{
    byte temp = *a;
    *a = *b;
    *b = temp;
}

/*Initialization & initial permutation
also initialize i&j counters in state for stream generation*/
void initState(const byte K[256], int keylen, Rc4State *state)
{
    byte T[256];
    assert(keylen>=1 && keylen<=256);
    int i;
    for(i = 0; i < 256; i++) {
        state-> S[i] = i;
        T[i] = K[i % keylen];
    }

    //Initial permutation of S
    byte *S = state->S;
    int j = 0;
    for(i = 0; i < 256; i++) {
        j = (j + S[i] + T[i]) % 256;
        swap(&S[i], &S[j]);
    }

    //Initialize counters in state
    state->i = state->j = 0;
}

/*Encrypt/Decrypt text by XORing with next byte of keystream*/
byte crypt(byte text, Rc4State *state)
{
    byte t, k;
    byte *i = &(state->i), *j = &(state->j);
    byte *S = state->S;
    *i = (*i+1) % 256;
    *j = (*j+S[*i]) % 256;
    swap(&S[*i], &S[*j]);
    t = (S[*i] + S[*j]) % 256;
    k = S[t];

    return text ^ k;
}

I considered removing the %256 since it's implied in unsigned 8 bit arithmetic but left it in for clarity.

I asked this on SO.

Any comments would be appreciated.

share|improve this question

1 Answer 1

up vote 5 down vote accepted

From top to bottom…

  • #include <stdio.h> appears to be left over from development, and can be removed.
  • swap() is not part of your library's interface, and should be declared static.
  • To give your library a sense of unity, I'd rename initState() → rc4InitState() and crypt() → rc4Crypt(). The latter renaming is also important so as not to clash with the traditional Unix crypt(3) function. Also for consistency, I'd make Rc4State *state the first parameter of each function.

  • Assertions are not really an appropriate mechanism for parameter validation. The use of assert() in initState() is marginal. You should assert conditions that you know to be true, rather than conditions that you hope to be true. Alternatively stated, assertion failures should arise from programmer errors, not user errors.

    Change int keylen to uint8_t keylen, and most of the validation problem goes away anyway. (In C, i % 0 has undefined behaviour.)

  • On the other hand, you could add an assert((byte)(S[i] + 256) == S[i]) to put to rest your concerns about overflow.
  • Calling crypt() to encrypt a byte at a time is inefficient. The function should accept a byte array and length. A compiler might inline a function, but if you're writing this as a library, the linker can't optimize away a function call.

  • *i and *j are a bit unconventional for my taste. Here's how I would express it:

    static byte rc4CryptByte(Rc4State *state, byte plainText)
{
    byte *S = state->S;
    byte i = ++(state->i);
    byte j = (state->j += S[i]);

    swap(&S[i], &S[j]);
    byte t = S[i] + S[j];
    byte k = S[t];

    return plainText ^ k;
}

void rc4Crypt(Rc4State *state, byte text[], size_t len)
{
    for (size_t i = 0; i < len; i++)
    {
        text[i] = rc4CryptByte(state, text[i]);
    }
}
share|improve this answer
    
Actually, I wasn't implementing a library and I just copy pasted everything except the main() from my program. –  v3ga Feb 7 '14 at 20:23
    
It would still be a good habit to write your code as if you were designing a library. The best practices I suggested cost you nothing. –  200_success♦ Feb 7 '14 at 20:28
1  
Of course, I was simply clarifying. The main motive in posting to CR was to make a habit of following best practices.Thanks for the part on assert & static esp. Also, I don't think I can make keylen as uint8_t as it won't be able to take the value 256. –  v3ga Feb 7 '14 at 20:35

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.