4
\$\begingroup\$

The following program is an updated version of this question, an implementation of the Vigenere cipher in C. It has been updated to take input from stdin and pipes, amongst other suggestions made by users answering the previous question, as well as having been significantly cleaned up. The program has been through several revisions since my original post, but I still feel that there is room for improvement.

It feels like the code is unnecessarily lengthy and could probably be cut down in size. Are there any redundancies in my code or things that could be done more simply? Are there any parts that could be written in a cleaner or more concise way? This is my first time using stdin so please inform me if I've made any mistakes with it. Despite fairly extensive testing, there could be an edge-case bug or two I missed.

// encryption.c - encrypts a message from stdin and encrypts/decrypts it against a given password and a user specified encryption method (vigenere, etc.)

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

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <ctype.h>
#include <unistd.h>
#include <stdbool.h>

enum EncryptionMethod {
    EM_Vigenere
};

typedef struct block {
    size_t size;
    char *data;
} block_t;

typedef struct options {
    char                    *password;
    enum EncryptionMethod   crypt_method;
    bool                    encrypt;
} options;

block_t read_data_block(FILE *fp, size_t block_size);
bool    isvalid(char input);
char    *vigenere_enc(char plain[], char key[]);
char    *vigenere_dec(char key_text[], char key[]);
options parse_opts(int argc, char *argv[]);
void    alphatonum(char *input);
void    numtoalpha(char *input);
void    string_clean(char *source);
void    usage(char *prog_name);

int main(int argc, char *argv[])
{
    options args;
    block_t block;
    int     chunk_s;

    args = parse_opts(argc, argv);
    string_clean(args.password);
    chunk_s = strlen(args.password);

    do {
        block = read_data_block(stdin, chunk_s);

        if(block.size == 0)
            break;

        if(args.encrypt == true)
            block.data = vigenere_enc(block.data, args.password);
        else
            block.data = vigenere_dec(block.data, args.password);

        if( block.data    == NULL   ||
            block.data[0] == EOF    || 
            block.data[0] == NULL   )
            break;

        printf("%s", block.data);
    } while(block.size == chunk_s);

    fclose(stdin);

    return 0;
}

block_t read_data_block(FILE *fp, size_t block_size) {
    block_t input;
    input.data = NULL;
    input.size = 0;
    unsigned int pos = 0;

    if( block_size < 1 ) {  
        fprintf(stderr, "Block size error!\n");
        return input;
    }   

    if( !(input.data = malloc(block_size)) ) {
        fprintf(stderr, "Malloc error!\n");
        return input;
    }

    while(  pos < block_size &&
            (input.data[pos] = fgetc(fp)) != EOF )
        if(isvalid(input.data[pos]) == true)
            ++pos;

    input.data[pos] = '\0';

    if( pos < block_size) {
        char *tmp;
        if((tmp = realloc(input.data, pos)) != NULL)
            input.data = tmp;
    }

    input.size = (size_t) pos;

    return input;
}

bool isvalid(char input)
{
    if(isupper(input) || islower(input))
        return true;
    else
        return false;
}

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

    string_clean(plain);

    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)) )
        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';
    }

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

    enc_text[plain_len] = '\0';

    return enc_text;
}

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

    string_clean(enc_text);

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

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

    if( !(dec_text = malloc(dec_text_len + 1)) )
        return NULL;

    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';
    }

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

    dec_text[dec_text_len] = '\0';

    return dec_text;
}

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

    args.encrypt        = true; //Enable encrypt mode by default
    args.crypt_method   = EM_Vigenere; //Enable vigenere encryption by default
    args.password       = NULL;
    opterr              = 0;

    while((c = getopt(argc, argv, "dehvp:")) != -1) {
        switch(c) {
            case 'd':
                args.encrypt        = false;
                break;
            case 'e':
                args.encrypt        = true;
                break;
            case 'h':
                usage(argv[0]);
                exit(0);
            case 'v':
                args.crypt_method   = EM_Vigenere;
                break;
            case 'p':
                args.password       = optarg;
                break;
            case '?':
                if(optopt == 'p')
                    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) {
        usage(argv[0]);
        exit(-1);
    }

    return args;
}

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

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 numtoalpha(char *input)
{
    if(*input >= 0 && *input <= 26)
        *input += 'A';
}

void usage(char *prog_name)
{
    fprintf(stderr, "Usage: %s -p [password]\n"
                    "Optional: -e(ncrypt) -d(ecrypt) -v(igenere cipher) -h(elp)\n"
                    "Now updated to take input from pipes and stdin!\n", prog_name);
}
\$\endgroup\$
2
\$\begingroup\$

Merge common functions

Your vigenere_enc() and vigenere_dec() functions are almost entirely identical. You could merge both functions into one quite easily, like this:

char *vigenere(char src_text[], char key[], bool encrypt)
{
    char *out_text;

    string_clean(src_text);

    int out_text_len    = strlen(src_text);
    int key_len         = strlen(key);

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

    if( !(out_text = malloc(out_text_len + 1)) )
        return NULL;

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

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

        alphatonum(&src_text[i]);
        if (encrypt) {
            out_text[i] = ((src_text[i] + key[j]) % 26);
        } else {
            out_text[i] = ((src_text[i] + 26 - key[j]) % 26);
        }

        out_text[i] += 'A';
    }

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

    out_text[out_text_len] = '\0';

    return out_text;
}

Then you would call it like this, which also merges two cases together nicely:

    block.data = vigenere(block.data, args.password, args.encrypt);

Confusing if statement

This if statement puzzled me:

    if( block.data    == NULL   ||
        block.data[0] == EOF    ||
        block.data[0] == NULL   )
        break;
  1. First of all, I got a compiler warning "comparison between pointer and integer". The last check should be block.data[0] == '\0'.
  2. There is no way that block.data[0] could ever be EOF, because your encrypt/decrypt functions can never generate an EOF.
  3. There is no way that block.data[0] could ever be '\0' either, if you check all of the program logic.

So essentially, I believe that code could be reduced to simply:

    if (block.data == NULL)
        break;

Memory leak

You never free block.data anywhere, so you keep leaking memory every time you read a new block and every time you encrypt/decrypt it. I would actually recommend changing your read_data_block function to take an existing block as an argument instead of allocating a new buffer every time. That way, you can allocate one block in main and reuse it. I would also suggest modifying your encrypt/decrypt function to write over the original contents instead of allocating a new block.

Simplification

I would rewrite this function:

bool isvalid(char input)
{
    if(isupper(input) || islower(input))
        return true;
    else
        return false;
}

as this:

bool isvalid(char input)
{
    return isupper(input) || islower(input);
}

Unnecessary fclose()

This line is unnecessary:

fclose(stdin);

Unnecessary function call

In your encrypt/decrypt function, you have this:

string_clean(plain);

Your read_data_block() function already cleaned the string, so you don't need to call this again. More importantly, it is required that the string already be cleaned. If it weren't, then string_clean() here could make the input be an empty string, which would cause your encrypt/decrypt function to return NULL and abort the process, even though there could be more data.

My recommendation is that the string_clean() call be removed, and a comment should be added which states that the input string must already be cleaned.

\$\endgroup\$
0
\$\begingroup\$

Change type of pos from unsigned int to size_t

That would make the cast from unsigned int to size_t unneccesary, while also ensuring portability if a very large array index is used. Also, your struct block has the member size, which is of type size_t. You should try to avoid casts when practical, as they can cause nasty problems in the future.

Simplification

JS1's answer suggested changing the isvalid function to:

bool isvalid(char input)
{
    return isupper(input) || islower(input);
}

Looking at the manual page for isalpha(3), I found that it is equivalent to (isupper(c) || islower(c)), so you can omit the isvalid function completely and use isalpha instead. (Remember to remove the declaration as well.)

Pass pointers to structures instead of the structure itself

On most systems, it is definitely better to pass a pointer to a struct instead of the whole thing if your struct is sufficiently large, which I think is true for both of them. See this question for more details.

\$\endgroup\$

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.