Vigenere's cipher in C

Question

I have written a program in C that takes plain-text and a password and produces a cipher-text using vigenere's cipher. While the code produces the correct output most of the time, I have found an example where it produces an unexpected output and I cannot find the issue myself. The output is as such:

jess@laptop:~/Desktop/programming/C/current/vigenere$ ./vigenere lemon attackatdawn
LXF OPV EFR NHR [0002]

That box at the end doesn't appear as it should, it is meant to represent when bash tries to display ascii character 2, but copy and paste doesn't show it correctly. This is the example text from Wikipedia for the cipher, and it's the only text I've found that breaks my program (I don't know what the cause is, so I cannot replicate it) but I'm sure there are more strings that will produce similar results. I suspect that I have done something that produces undefined behaviour but I'm not sure. What have I done wrong here? My code:

// vigenere.c - Takes a plaintext and a cipher key from argv[1] and argv[2] and produces the cipher text according to Vigenere's cipher

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

void string_clean(char *source) //Courtesy (mostly) of user 'Aaron' of StackOverflow
{
    char *i = source;
    char *j = source;
    
    while(*j != 0) {
        *i = *j++;
        if( *i != ' ' && (isupper(*i) || islower(*i)) )
            i++;
    }
    
    *i = 0;
}

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( !(cipher_text = calloc(plain_len, 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';
    }
    
    int j = 0;
    
    for(int i = 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[])
{
    if(argc != 3)
        return 1;
    char *cipher = vigenere_enc(argv[2], argv[1]);

    for(int i = 0; i < strlen(cipher); i++) {
        if(i % 3 == 0 && i != 0)
            putchar(' ');
        putchar(cipher[i]);
    }
    
    putchar('\n');
    
    return 0;
}

All and any help/suggestions are greatly appreciated!

score 3 · Accepted Answer · answered Sep 05 '15 at 19:42

3

You need to NUL-terminate your output string. :-( That also means your call to calloc (you really should just use malloc, though) should specify plain_len + 1, not just plain_len.

answered Sep 05 '15 at 19:42

C. K. Young

219,335
46
382
435

1

OP is also writing to the input strings in `string_clean()` but arguments passed to `main()` may not be writeable. – Weather Vane Sep 05 '15 at 19:47
Thank you very much for this answer as it has solved my issue. Two questions though, why use malloc over calloc, and why plain_len + 1 and not plain_len? Not my best day for off-by-one's. :/ – jess Sep 05 '15 at 19:51
1

`calloc` zeros the memory but you wriet to it all anyway, without needing it to be zero. And `+1` because you need room for the string terminator mentioned. `strlen()` does not include this. – Weather Vane Sep 05 '15 at 19:52
@psychedelic_alex `malloc` doesn't clear the memory first. This is usually faster than `calloc`, and since your program doesn't rely on cleared memory, it works just fine. The + 1 is because you need to allocate the extra character for the NUL terminator. Or, you know, what Weather Vane just said. ;-) – C. K. Young Sep 05 '15 at 19:52
Thanks a lot guys, you've saved me loads of time here :-). One more question though for @WeatherVane when and why would argv not be writeable? I've actually heard that it's always writeable but I'm happy to be wrong with that. – jess Sep 05 '15 at 19:59
@psychedelic_alex you could be right, there is a discussion about modifying args here http://stackoverflow.com/questions/25737434/is-argvn-writable I wrote *"may not be"* out of caution. Of course, arguments that are on the stack, like `argc` are writeable. – Weather Vane Sep 05 '15 at 20:07

Vigenere's cipher in C

1 Answers1