#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.


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]);
| improve this answer | |
  • \$\begingroup\$ Actually, I wasn't implementing a library and I just copy pasted everything except the main() from my program. \$\endgroup\$ – avmohan Feb 7 '14 at 20:23
  • \$\begingroup\$ 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. \$\endgroup\$ – 200_success Feb 7 '14 at 20:28
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – avmohan Feb 7 '14 at 20:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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