RC4 implementation in C

#include 
#include 
#include 

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 S[i] = i;
        T[i] = K[i % keylen];
    }

    //Initial permutation of S
    byte *S = state->S;
    int j = 0;
    for(i = 0; i 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.

From top to bottom…

• #include 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]);
    }
}

StackExchange Code Review Q#41148, answer score: 8