0

I am having trouble debugging my implementation of Vigenere's cipher in C. The error arises when using file input (-f flag) where the file contains fewer than 6 chars (+ 1 EOF), it spits out some number of random characters as well as the expected input and I cannot figure out why this is, although I suspect it has something to do with the second part of my question which is, when using fread(), I noticed that this

if( fread(fcontents, fsize, sizeof(char), file) != 1 ) {...}

will run with no issues, whereas this

if( fread(fcontents, sizeof(char), fsize, file) != 1 ) {...}

doesn't work (i.e. causes fread() to return 1 and trigger the error handling code beneath it), which I would expect to be the other way around according to answers from here, but I may just be misinterpreting something.

My complete code is as follows:

#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 INP 0 //Commandline input mode
#define FLE 1 //File input mode

typedef struct {
    char *password;
    char *file_name;
    char *input;
    int edmode;
    int ifmode;
} options;

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

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

    *i = 0;
}

char *ftostr(char *file_name) //Takes a file name as input and returns a char* to the files contents, returns NULL pointer on faliure. Allocated string must be freed after use by parent function to prevent memory leaks.
{
    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, 0L, 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, fsize, sizeof(char), file) != 1 ) { //suspected buggy line
        fclose(file);
        free(fcontents);
        fprintf(stderr, "Error copying file to memory!\n");
        return NULL;
    }

    fclose(file);
    return fcontents;
}

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

    args.edmode     = ENC; //enable encrypt mode by default
    args.ifmode     = INP; //enable commandline input mode by default
    args.file_name  = NULL;
    args.password   = NULL;
    args.input      = NULL;
    opterr          = 0;

    while((c = getopt(argc, argv, "dep:i:f:")) != -1) {
        switch(c) {
            case 'e':
                args.edmode     = ENC;
                break;
            case 'd':
                args.edmode     = DEC;
                break;
            case 'p':
                args.password   = optarg;
                break;
            case 'i':
                args.input      = optarg;
                args.ifmode     = INP;
                break;
            case 'f':
                args.file_name  = optarg;
                args.ifmode     = FLE;
                break;
            case '?':
                if(optopt == 'f' || optopt == 'p' || optopt == 'i')
                    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);

                fprintf(stderr, "Usage: %s (-f file_name || -i input) -p password [options]\n"
                                "Optional: -e -d\n", argv[0]);
                exit(-1);
        }
    }

    return args;
}

char *vigenere_dec(char cipher_text[], char cipher[])
{
    char *plain_text;

    string_clean(cipher_text);
    string_clean(cipher);

    int plain_text_len = strlen(cipher_text);
    int cipher_len = strlen(cipher);

    if( !(plain_text = malloc((plain_text_len + 1) * sizeof(char))) )
        return 0;

    for(int i = 0; i < cipher_len; i++) {
        if(isupper(cipher[i]))
            cipher[i] -= 'A';
        else if(islower(cipher[i]))
            cipher[i] -= 'a';
    }

    for(int i = 0, j = 0; i < plain_text_len; i++, j++) {
        if(j == cipher_len)
            j = 0;

        if(isupper(cipher_text[i]))
            cipher_text[i] -= 'A';
        else if(islower(cipher_text[i]))
            cipher_text[i] -= 'a';

        plain_text[i] = ((cipher_text[i] - cipher[j]) % 26);
        if(plain_text[i] < 0)
            plain_text[i] += 26;
        plain_text[i] += 'A';
    }
    return plain_text;
}

char *vigenere_enc(char plain[], char cipher[])
{
    char *cipher_text;

    string_clean(plain);
    string_clean(cipher);

    int plain_len   = strlen(plain);
    int cipher_len  = strlen(cipher);

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

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

    for(int i = 0; i < cipher_len; i++) {
        if(isupper(cipher[i]))
            cipher[i] -= 'A';
        else if(islower(cipher[i]))
            cipher[i] -= 'a';
    }

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

        if(isupper(plain[i]))
            plain[i] -= 'A';
        else if(islower(plain[i]))
            plain[i] -= 'a';

        cipher_text[i] = ((plain[i] + cipher[j]) % 26) + 'A';
    }
    return cipher_text;
}

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

    args = parse_opts(argc, argv);

    if(args.password == NULL) {
        fprintf(stderr, "Password uninitialised!\n");
        exit(-1);
    }

    if(args.input == NULL && args.file_name == NULL) {
        fprintf(stderr, "Input stream uninitialised!\n");
        exit(-1);
    }

    if(args.ifmode == INP) {
        if(args.edmode == ENC)
            output_text = vigenere_enc(args.input, args.password);
        else if(args.edmode == DEC)
            output_text = vigenere_dec(args.input, args.password);
    } else if(args.ifmode == FLE) {
        if( !(args.input = ftostr(args.file_name)) )
            return -1;
        if(args.edmode == ENC)
            output_text = vigenere_enc(args.input, args.password);
        else if(args.edmode == DEC)
            output_text = vigenere_dec(args.input, args.password);
        free(args.input);
    }

    puts(output_text);
    free(output_text);

    return 0;
}
Community
  • 1
  • 1
jess
  • 222
  • 2
  • 18
  • 1
    `EOF` is no `char`. Please provide a [mcve] (Notice the **minimal**!) And "doesn't work" is no error description. – too honest for this site Sep 06 '15 at 20:39
  • What does "enabling file mode" mean? And in addition to an MCVE as requested please also provide sample input, expected output and actual output. – kaylum Sep 06 '15 at 20:42
  • 1
    You quoted another question, but failed to see that `fread()` returns the number of **items** read. So if you swap the `size` and `count` arguments, expect a different result (unless `fsize == 1`). – Weather Vane Sep 06 '15 at 21:01
  • Ah, you basically answered the whole second part of my question with that, thanks! – jess Sep 06 '15 at 21:07

1 Answers1

1

The fault is unterminated strings. You allowed room for the termination char with

if( !(plain_text = malloc(plain_text_len + 1)) )  // (simplified)

but after you have set, for example

plain_text[i] += 'A';

you need to end the string with

plain_text[i+1] = '\0';

or when the string is complete.

For the second part, you quoted another question, but failed to see that fread() returns the number of items read. So if you swap its size and count arguments, expect a different result (unless fsize == 1).

So you can either use

if( fread(fcontents, fsize, 1, file) != 1 ) {...}

or this

if( fread(fcontents, 1, fsize, file) != fsize ) {...}

Note I changed sizeof(char) to 1 since by definition, it is.

Weather Vane
  • 33,872
  • 7
  • 36
  • 56
  • Thanks, it would have taken ages for me to spot that! The 'sizeof(char)' bit was just for clarity and because I don't really like magic numbers. – jess Sep 06 '15 at 22:06