Vigenère cipher 2

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

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(

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;

• First of all, I got a compiler warning "comparison between pointer and integer". The last check should be block.data[0] == '\0'.

• There is no way that block.data[0] could ever be EOF, because your encrypt/decrypt functions can never generate an EOF.

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

StackExchange Code Review Q#104469, answer score: 2