C - Buffer overflow with strcpy() and strncpy()

Question

I'm taking the CS50 course on edx and I'm supposed to write a code in C that encrypts a message using the Vigenere Cipher. And I did. But the problem is I keep getting buffer overflow.

If I use a short string, like "Meet", it works okay. But if I use a longer string, like "Meet me at the park at eleven am", I get buffer overflow.

If I use strcpy() to copy both the key passed in argv[1] to another variable (let's say k) and the original message (msg) to the variable of the encrypted message (encMsg), then it erases what I had in k. If I use strcpy() again to copy the value of argv[1] to k one more time, I get buffer overflow in encMsg concatenating both strings.

If I use strccpy(), it won't get the content of k erased, but it will overflow and concatenate both strings on encMsg.

This all happens when I use GetSting() (from cs50.h);

If I use fgets(), then I will get '\n' at the end of msg and then have to add an if to put a '\0' where that '\n' was, which solves my problem, but I wanted to know if anyone could explain me why all of this happens.

Here's my code:

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


int main(int argc, char *argv[])
{
    if (argc != 2) //Checks if number of command-line arguments is valid
    {
        printf ("usage: ./caesar + key \n");
        return 1;    
    }

    char k[strlen(argv[1])];
    //strcpy(k, argv[1]);         //Saves argument into a string
    strncpy(k, argv[1], strlen(argv[1]));

    for (int x = 0; x < strlen(k); x++) 
    {
        if (k[x] < 65 || (k[x] > 90 && k[x] < 97) || k[x] > 122) //Checks if there is any non-alphabetical character in key
        {
            printf ("key must contain only alphabetical characters \n");
            return 1;    
        }
    }

    //printf("Inform the message you want to encrypt: \n");
    string msg = GetString();
    //char msg[255];
    //fgets(msg, 255, stdin);

    char encMsg[strlen(msg)];
    //strcpy(encMsg, msg);
    strncpy(encMsg, msg, strlen(msg));

    //strcpy(k, argv[1]); 

    int y = 0;
    for (int x = 0; x < strlen(msg); x++) 
    {
        //if(msg[x] == '\n') msg[x] = '\0';
        if (msg[x] < 65 || (msg[x] > 90 && msg[x] < 97) || msg[x] > 122) encMsg[x] = msg[x];
        else if ((msg[x] + (k[y] - 97 ) > 90 && msg[x] + (k[y] - 97 ) < 97) || msg[x] + (k[y] - 97 ) > 122)
        {
            encMsg[x] = msg[x] + (k[y] - 97) - 26;
            y++;
        }
        else 
        {
            encMsg[x] = msg[x] + (k[y] - 97);
            y++;
        }
        if (y >= strlen(k)) y = 0;
    }

    printf("key     =    %s\n", k);
    printf("msg     =    %s\n", msg);
    printf("encMsg  =    %s\n", encMsg);


    return 0;
}

Answer 1

This style of code:

char k[strlen(argv[1])];

leaves no space for a terminating '\0' character. It should be

char k[strlen(argv[1]) + 1 ];

And as pointed out in the comments, code such as

strncpy(k, argv[1], strlen(argv[1]));

will not properly terminate the target string. Per the standard for strncpy():

If the array pointed to by s2 is a string that is shorter than n bytes, NUL characters shall be appended to the copy in the array pointed to by s1, until n bytes in all are written.

Since the target string isn't shorter than n, only the non-NUL bytes in the source string are copied as there are n of them.

Proper code could be

strncpy(k, argv[1], 1 + strlen(argv[1]));