Based on this code found on github and the advice given to me on stackoverflow I made this code for the xor in c.

    #include <stdio.h>
    #include <string.h>

    void encryptDecrypt(char *input, char *output)
        char key[3] = {'K', 'E', 'Y'};

        for (int i = 0; i < strlen(input); ++i)     

            output[i] = input [i] ^ key [i % sizeof(key)]; 

    int main()
        char baseStr[] = "Test";    

        char encrypted[strlen(baseStr) + 1];
        memset(encrypted, '\0', sizeof(encrypted));
        encryptDecrypt(baseStr, encrypted);
        printf("Encrypted: %s\n", encrypted);

        char decrypted[strlen(baseStr) + 1];
        memset(decrypted, '\0', sizeof(decrypted));
        encryptDecrypt(encrypted, decrypted);
        printf("Decrypted: %s\n", decrypted);

        return 0;


How can I improve the code?

  • \$\begingroup\$ The first time I broke XOR encrypted text it took me two years after I broke the encryption to realize it was even using encryption. I thought it just a weird encoding. \$\endgroup\$
    – hildred
    May 1, 2020 at 0:54

2 Answers 2


Don't deal in NUL terminated strings. The problem is that there are characters that will "encrypt" to NUL. (In this case, notably, 'K', 'E', and 'Y'.) This means you need to pass in the length of the text.

If you want to allow for future expansion, allow for the possibility that the output text may be longer than the input text. Dynamic allocation may be appropriate, or maybe you have an auxiliary function that says "for a text of length x, what is the longest length of output text." It may also then be appropriate to pass in the output buffer length, and pass out the output used length.

Also allow for the specifying the operation (encrypt/decrypt). This "encryption" doesn't need it, but most symmetric encryptions do.

  • 3
    \$\begingroup\$ Symmetric ciphers normally have output length = input length; I don't think doing dynamic allocation inside the enc/dec function sounds like a great idea. I'd suggest having main call it with output=input to encrypt and then decrypt in place so it doesn't need any extra allocation of C99 VLAs. \$\endgroup\$ Apr 30, 2020 at 10:05
  • \$\begingroup\$ @PeterCordes Symmetric ciphers normally have a block size. Feeding 3 bytes in will not get you three bytes out. OP's "encryption" is unusual in having a block size of 1 byte. Also, it is not unusual to add a header, to validate that the decryption is using the right key. Adding a magic number header is also reasonable. \$\endgroup\$
    – David G.
    Apr 30, 2020 at 14:17
  • \$\begingroup\$ Sure, I left out that detail. For a block cipher the buffers should be explicit-length, so just passing the input length implies the output length is that rounded up. If the caller wants to alloc a buffer, it's not unreasonable to expect it to know whatever rule applies for rounding the length up to a multiple of the block size. Baking in allocation would make it impossible to encrypt in-place, overwriting the input with encrypted output, so I hope you're not suggesting that. \$\endgroup\$ Apr 30, 2020 at 14:31

First, about the existing code:

  • key can be initialized with a string literal, "KEY", rather than an array literal - with caveats in the comments
  • encryptDecrypt should be static
  • It should accept a const char *input as a promise not to modify it
  • Since your baseStr is a local array, you do not need to call strlen on it; you can use sizeof

Now, about what the code could be doing:

  • Accept the input from stdin or a file instead of having it be hard-coded; similar for the key
  • Learn about encryption algorithms that are stronger than this; and learn about the cryptographic weaknesses of xor encryption
  • When you print the encrypted string to stdout, do not print its raw string which will be full of unprintable characters. Instead print a hex string.
  • 4
    \$\begingroup\$ Note that initializing key with a string literal can be fraught with trouble. GCC for instance gives you warnings. The issue is that a string literal includes a trailing NUL. If you don't use sizeof(key), this can be cleaner. And maybe key should be static either way. \$\endgroup\$
    – David G.
    Apr 30, 2020 at 0:09
  • 4
    \$\begingroup\$ You can and should also hoist strlen(input) out of the loop! With a char assignment inside the loop body, the compiler will probably re-run strlen every iteration in case that store to output[i] affected input[i+1]. \$\endgroup\$ Apr 30, 2020 at 9:58
  • 1
    \$\begingroup\$ I'd also recommend taking a command-line arg as the string to encrypt with a fallback string literal if there are no args. Easier to test with than echo foo | ./xor if you only support stdin. \$\endgroup\$ Apr 30, 2020 at 10:01
  • \$\begingroup\$ Weaknesses of xor encryption, including but not limited to the encryption function is the decryption function. \$\endgroup\$
    – S.S. Anne
    Apr 30, 2020 at 19:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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