4
\$\begingroup\$

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 <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <ctype.h>
#include <unistd.h>

#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;              //Stores all user inputs; file name, text string, encrypt or decrypt, encryption method, etc.

char    *ftostr(char *file_name);                   //Converts a given file to a string, returns char* on success and NULL on fail
char    *vigenere_enc(char plain[], char key[]);    //Encryption function, returns char* on success or NULL on fail
char    *vigenere_dec(char key_text[], char key[]); //Decryption function, returns char* on success or NULL on fail
options parse_opts(int argc, char *argv[]);         //Commandline argument parsing function, returns an options struct based or exits with a -1 code if inputs are invalid (no password or input given)
void    enc_print(char *enc_text);                  //Print the text in the 3 char blocks
void    string_clean(char *source);                 //Remove non alpha chars from string
void    usage(char *prog_name);                     //Print the program usage
void    alphatonum(char *input);                    //Takes an alphabetic char of either case and converts it to it's numeric position in the alphabet

int main(int argc, char *argv[])
{
    options args;
    char    *output_text = NULL;

    args = parse_opts(argc, argv);

    if(args.input_mode == ARG_IN) {     //Lots of ugly and duplicated code ahead...
        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);
        }
    } else if(args.input_mode == FILE_IN) {
        if( (args.input_string = ftostr(args.file_name)) == NULL ) {
            fprintf(stderr, "Error accessing file!\n");
            exit(-2);
        }
        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);
        }
        free(args.input_string);
    }

    if(output_text != NULL) {
        if(args.enc_dec == ENC)
            enc_print(output_text);
        else if(args.enc_dec == DEC)
            puts(output_text);
        free(output_text);
    }

    return 0;
}

char *ftostr(char *file_name)
{
    FILE        *file;
    long int    fsize;
    char        *fcontents;

    if( !(file = fopen(file_name, "r")) ) {
        fprintf(stderr, "Error opening file \"%s\"!\n", file_name);
        return NULL;
    }

    fseek(file, 0, SEEK_END);
    fsize = ftell(file);
    rewind(file);

    if( !(fcontents = malloc((fsize + 1) * sizeof(char))) ) {
        fclose(file);
        fprintf(stderr, "Error allocating memory!");
        return NULL;
    }

    if( fread(fcontents, sizeof(char), fsize, file) != fsize ) {
        fclose(file);
        free(fcontents);
        fprintf(stderr, "Error copying file to memory!\n");
        return NULL;
    }

    fclose(file);
    return fcontents;
}

char *vigenere_enc(char plain[], char key[])
{
    char *enc_text = NULL;

    string_clean(plain);
    string_clean(key);

    int plain_len   = strlen(plain);
    int key_len     = strlen(key);

    if(plain_len == 0 || key_len == 0)
        return NULL;

    if( !(enc_text = malloc((plain_len + 1) * sizeof(char))) )
        return NULL;

    for(int i = 0; i < key_len; i++)
        alphatonum(&key[i]);

    for(int i = 0, j = 0; i < plain_len; i++, j++) {
        if(j == key_len)
            j = 0;

        alphatonum(&plain[i]);
        enc_text[i] = ((plain[i] + key[j]) % 26) + 'A';
    }

    enc_text[plain_len] = '\0';
    return enc_text;
}

char *vigenere_dec(char enc_text[], char key[])
{
    char *dec_text;

    string_clean(enc_text);
    string_clean(key);

    int dec_text_len    = strlen(enc_text);
    int key_len         = strlen(key);

    if( !(dec_text = malloc((dec_text_len + 1) * sizeof(char))) )
        return 0;

    for(int i = 0; i < key_len; i++)
        alphatonum(&key[i]);

    for(int i = 0, j = 0; i < dec_text_len; i++, j++) {
        if(j == key_len)
            j = 0;

        alphatonum(&enc_text[i]);
        dec_text[i] = ((enc_text[i] - key[j]) % 26);

        if(dec_text[i] < 0)
            dec_text[i] += 26;
        dec_text[i] += 'A';
    }

    dec_text[dec_text_len] = '\0';

    return dec_text;
}


options parse_opts(int argc, char *argv[])
{
    int     c;
    options args;

    args.enc_dec        = ENC; //Enable encrypt mode by default
    args.crypt_meth     = VIG; //Enable vigenere encryption by default
    args.file_name      = NULL;
    args.password       = NULL;
    args.input_string   = NULL;
    opterr              = 0;

    while((c = getopt(argc, argv, "dehvf:p:s:")) != -1) {
        switch(c) {
            case 'd':
                args.enc_dec        = DEC;
                break;
            case 'e':
                args.enc_dec        = ENC;
                break;
            case 'h':
                usage(argv[0]);
                exit(0);
            case 'v':
                args.crypt_meth     = VIG;
                break;
            case 'p':
                args.password       = optarg;
                break;
            case 's':
                args.input_string   = optarg;
                args.input_mode     = ARG_IN;
                break;
            case 'f':
                args.file_name      = optarg;
                args.input_mode     = FILE_IN;
                break;
            case '?':
                if(optopt == 'f' || optopt == 'p' || optopt == 's')
                    fprintf(stderr, "Option -%c requires an argument.\n", optopt);
                else if(isprint(optopt))
                    fprintf(stderr, "Unknown option `-%c'.\n", optopt);
                else
                    fprintf(stderr, "Unknown option character `\\x%x'.\n", optopt);

                usage(argv[0]);
                exit(-1);
        }
    }

    if(args.password == NULL || (args.input_string == NULL && args.file_name == NULL)) {
        usage(argv[0]);
        exit(-1);
    }

    return args;
}

void alphatonum(char *input)
{
    if(isupper(*input))
        *input -= 'A';
    else if(islower(*input))
        *input -= 'a';
}

void enc_print(char *enc_text)
{
    for(int i = 0; i < strlen(enc_text); i++) {
        if(i % 3 == 0 && i != 0)
            putchar(' ');
        putchar(enc_text[i]);
    }
    putchar('\n');
}

void string_clean(char *source)
{
    char *i = source;
    char *j = source;

    while(*j != '\0') {
        *i = *j++;
        if(isupper(*i) || islower(*i))
            i++;
    }

    *i = '\0';
}

void usage(char *prog_name)
{
    fprintf(stderr, "Usage: %s (-f [file name] || -s [input text]) -p [password]\n"
                    "Optional: -e(ncrypt) -d(ecrypt) -h(elp)\n", prog_name);
}

If anyone has the time,

I'd like to know what I did well in this program and what I did poorly, as well as an explanation of how I could go about correcting these mistakes. I feel like the program is fairly bloated and could probably be done in far fewer lines. All feedback is greatly appreciated.

\$\endgroup\$
6
\$\begingroup\$

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/mybigfile > /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);
    print_block(output, stdout);
    destroy_block(input);
    destroy_block(output);
}

Hope this helps!

\$\endgroup\$
  • \$\begingroup\$ I like a lot of what you're saying here, but I'm yet to see a good reason why to use enum instead of #define and I'm sorry but "It's obselete" isn't much of an explanation. Other than that, a very well thought through and thorough (try saying that 5 times quickly) answer! \$\endgroup\$ – psychedelic_alex Sep 7 '15 at 23:35
  • \$\begingroup\$ Also, that last piece of code doesn't seem to make much sense to me. What's an "operation_t" type for? I've never seen anything like "(*encrypt)(input, key)", what does that do? I feel like the whole last part of this answer is beyond me as is and I could really do with some more explanation. \$\endgroup\$ – psychedelic_alex Sep 8 '15 at 0:45
  • \$\begingroup\$ Googling "enum versus define" should get you there. As for operation_t, that's a function pointer type, which should be covered in K&R somewhere (certainly in the part where they teach you how to parse declarations and write cdecl, but maybe somewhere else too). My first version of the answer was written a bit in haste and didn't compile, so you might find the last part a bit more comprehensible now. :) \$\endgroup\$ – Quuxplusone Sep 8 '15 at 1:25
  • 1
    \$\begingroup\$ You don't need to dereference a function pointer to call it (out = encrypt(input, key); should work just fine). \$\endgroup\$ – Mat Sep 8 '15 at 4:37
  • \$\begingroup\$ @Mat True, but at least in C, I prefer to be explicit. It helps (me) when skimming through code: if I want to know what this particular line code does, should I look (elsewhere) for the definition of a function encrypt, or should I look (locally) for the definition of a pointer variable encrypt? In C++ I probably wouldn't bother with the distinction, because there are so many other possibilities: encrypt could be a functor object, or a lambda, or a typename,... but in C I find the distinction useful enough to keep. YMMV, to each his own, etc. etc. \$\endgroup\$ – Quuxplusone Sep 8 '15 at 5:43

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.