Vigenère cipher in C

For my second major project in C, I decided to write an implementation of the Vigenère cipher in C.


My program uses command line options (optarg) and can read from both a file or from a string specified with the -s option. It does both encryption as well as decryption of text and also has error handling and I've listed the meaning of the main return codes above the code. I also made my best efforts to efficiently manage the memory.

To summarise,


you feed it a string (or file which it converts to a string), it removes any non alphabetic characters, it either encrypts or decrypts the text depending on what mode was specified (encrypt is the default) using the cipher that was specified (Vigenère is the default and currently the only one), and then prints the string in blocks of 3 chars if it's encrypted it, or just as a plain string if it's decrypting.

I wrote the program with the intention of adding other encryption/decryption methods later (other than Vigenère), so I've made it so that it should be quite easy to add more encrypt/decrypt functions.

I have heavily commented the function prototypes to give a fairly thorough explanation of how they work.

```
// encryption.c - encrypts a message (file or argv) and encrypts it against a given password and a user specified encryption method (vigenere, etc.)

// Return/exit codes:
// 0 - successful execution
// -1 - argument error
// -2 - File access error

#include
#include
#include
#include
#include

#define ENC 0 //Encrypt mode
#define DEC 1 //Decrypt mode

#define ARG_IN 0 //Commandline input mode
#define FILE_IN 1 //File input mode

#define VIG 0 //Vigenere cipher mode

typedef struct {
char *password;
char *file_name;
char *input_string;
int crypt_meth; //The encryption method to use (currently only one method, but I plan to add others)
int enc_dec; //Encrypt or decrypt the string
int input_mode; //File or argv
} options;

I see from your other questions that you're learning C from K&R. This is great — K&R is the best way to learn C, in my opinion — but their terse style is a bit "out of date" these days. So your code has a lot of Unixy brevity going on, which I'm going to be quietly undoing throughout this review.

For example,

int crypt_meth;  // Encryption method

could be written much more readably as

int encryption_method;

If you ever find yourself writing the same thing twice — once unreadably in code and again in English in the comments — you should always stop yourself and try to write it once, readably, in the code.

Okay, no more about naming. I'll just fix things up as I go along.

You use #define for constants. That's obsolete; you should be using enum instead.

enum InputMethod {
    IM_CommandLineInput,
    IM_FileInput,
};

enum EncryptionMethod {
    EM_Vigenere,
};

Notice the use of IM_ and EM_ prefixes for namespacing, since C doesn't have namespaces built into the language.

Particularly, your define DEC 1 steps on a lot of hypothetical toes — DEC could plausibly mean "decrement", or "decimal", or... and you're just declaring that in your program, it means the integer 1!

Pedantic aside: Your #define ENC 0 is actually invalid under POSIX; all-caps names beginning with the letter E (more or less) are reserved for the implementation's use only. Some real examples: EADV, EIO,... You're just lucky that ENC isn't one of the ones being used by your system!

However, I don't think you should use an enum for "encrypt or decrypt"; that sounds like a job for bool!

struct options {
    const char *password;
    const char *file_name;
    const char *input_string;
    EncryptionMethod encryption_method;
    bool encrypt;
    InputMethod input_method;
};

Here's where your code departs drastically from K&R/Unix-style simplicity: Your ftostr relies on fseek to compute a "file size", so it won't work on a pipe; for example

./vigenere -p GOD -e -f <(echo "hello")  # fails to work!

Pipes and streams are the powerhouse of Unix operating systems, and Vigenère encryption can be streamed really easily, so you ought to try to make your program work with stdin.

Also, suppose I want to encrypt a whole 10GB file — are you really going to tell me that I need 10GB of memory to encrypt it? I should be able to use your program like this:

./vigenere -p GOD  /tmp/mybigencryptedfile

Once you've figured out how to make that work, it'll be easy to come back and make the -f and -s options use the same system.

if(args.enc_dec == ENC) {
        if(args.crypt_meth == VIG)
            output_text = vigenere_enc(args.input_string, args.password);
    } else if(args.enc_dec == DEC) {
        if(args.crypt_meth == VIG)
            output_text = vigenere_dec(args.input_string, args.password);
    }

I suggest two improvements to this control flow.
First, in order to support streaming from a pipe, you should observe that the Vigenère cipher is a block cipher: it operates on "blocks" of size strlen(password). So your top-level control flow ought to be something like

while (get_next_block_from_input_stream(...)) {
    encrypt_block(...);
    print_block_to_stdout(...);
}

In fact, it seems pretty obvious that every step of that control flow is concerned with manipulating blocks, so maybe you ought to make Block a first-class concept in your program's structure.

struct Block {
    char *data;
    size_t size;  // will always be <= strlen(password)
};

Block get_block_from_stream(FILE *in);
Block vigenere_encrypt_block(Block input, const char *key);
void print_block(Block block, FILE *out);
void destroy_block(Block block);

while (true) {
    Block input = get_block_from_stream(stdin);
    if (input.size == 0) {
        break;  // we've reached the end of the input
    }
    Block output = vigenere_encrypt_block(input, key);
    print_block(output, stdout);
    destroy_block(input);   // i.e. free(input.data);
    destroy_block(output);  // i.e. free(output.data);
}

The second improvement I'd make is to the way you select the encryption method and whether you're encrypting or decrypting. Instead of encoding the logic into control flow with a bunch of nested ifs, try whenever possible to encode it into data with a table lookup.

```
typedef Block (operation_t)(Block, const char );
struct {
const char *name;
operation_t function_pointers[2];
} encryption_methods[] = {
{ "Vigenère", { vigenere_encrypt_block, vigenere_decrypt_block } },
{ "Caesar", { caesar_encrypt_block, caesar_decrypt_block } },
// ...
};

...
operation_t encrypt = encryption_methods[options.encryption_method].function_pointers[options.encrypt];
while (true) {
Block input = get_block_from_stream(stdin);
if (input.size == 0) {
break; // we've reached the end of the input
}
Block output = (*encrypt)(input, key);
p

StackExchange Code Review Q#104083, answer score: 7