Segmentation Fault when using strncpy in c

Question

This code is supposed to work as a vigenere cipher. When run, however, no matter what input you put in, it segmentation faults. I'm writing this for the online CS50 course on edx. Isn't strncpy supposed to stop segmentation faults from happening if I tell it to copy over the right amount of characters?

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

int main(int argc, string argv[]) {
    int result;

    if (argc != 2) {
        printf("Shame on you!\n");
        return 1;
    }

    string key = argv[1];
    string text = GetString();
    string cpy_key = NULL;

    //strncpy(cpy_key, key, strlen(key));
    for (int i = 0, n = strlen(text); i < n; i++) {
        strcat(cpy_key, key);
    }
    cpy_key[strlen(text)] = '\0';
    // Main loop starts here
    for (int i = 0, n = strlen(text); i < n; i++) {
        result = text[i] + cpy_key[i];

        if (isupper(text[i]) && (result > 'Z')) {
            result = result - 26;
        }
        if (islower(text[i]) && (result > 'z')) {
            result = result - 26;
        }
        if (isalpha(text[i])) {
            printf("%c", result);
        } else {
            printf("%c", text[i]);
        }
    }
    printf("\n");
    return 0;
}

Answer 1

The cs50.h header defines typedef char *string;.

The core dump occurs because you're copying to a null pointer:

string cpy_key = NULL;

//strncpy(cpy_key, key, strlen(key));
for (int i = 0, n = strlen(text); i < n; i++) {
    strcat(cpy_key, key);

Whether it is strcat() or strncpy(), you need to allocate storage space for cpy_key. With the loop shown, if the string entered is 50 characters, you are copying the string 50 times over, so you'd need to allocate more than 2500 characters to be safe. Using strncpy() would do the job correctly — as long as you allocate enough space.

Note that strncpy() not a good function to use. If you have a 20 KiB buffer and copy a 10 byte string into it, it writes 20470 null bytes after the string. If you have a 50 byte buffer and you copy 75 bytes into it, it copies the 50 bytes and does not leave the buffer null terminated. Neither is obvious; the lack of guaranteed null termination makes it dangerous. There are worse interfaces (strncat() is the prime candidate — what does the length parameter represent?) but not many.

You have some work to do on your encryption algorithm.

You could look at Vigenere cipher only works up to until dealing with a space in C — why? to see how else it could be done.

Answer 2

One problem is memory ie

string cpy_key = NULL;

does not allocate memory ie it's just a name with no size. Knowing this then this must fail

strcat(cpy_key, key);

In that call you tried to concatenate a thing that has a size to a thing with no size.

Answer 3

You do not need to make a copy of key, you can just index into key modulo its length.

By the way, you should NEVER EVER USE strncpy, it does not do what you think it does, its semantics are error prone, it is never the right tool for the job. The last argument is supposed to be the size of the destination array, but if the source is too large, the destination will not be null terminated and if the size is small and the destination large, this function will waste time padding the whole destination array will '\0' bytes. Not using strncpy will save you from many errors.

Also it is unfortunate that cs50.h defines string with typedef char *string;. Using such a type is misleading and error prone, especially for people who know C++. Just use char *, it makes your code much easier to read by C programmers. I hope you are not required to use this type, hiding pointers behind typedefs is generally not a good idea.

Here is a simpler version:

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

int main(int argc, char *argv[]) {
    int result;

    if (argc != 2) {
        printf("Shame on you!\n");  // you might want to be more explicit ;-)
        return 1;
    }

    char *key = argv[1];
    int klen = strlen(key);
    char *text = GetString();
    if (!text) {
        /* end of file or read error */
        return 1;
    }

    // Main loop starts here
    for (int i = 0, n = strlen(text); i < n; i++) {
        result = text[i] + key[i % len];

        if (isupper((unsigned char)text[i]) && result > 'Z') {
            result = result - 26;
        }
        if (islower((unsigned char)text[i]) && result > 'z') {
            result = result - 26;
        }
        if (isalpha((unsigned char)text[i])) {
            printf("%c", result);
        } else {
            printf("%c", text[i]);
        }
    }
    printf("\n");
    return 0;
}

HINT: your vigenere is incorrect, you should use result = text[i] + key[i % len] - 'A'; if the key is all uppercase.

Answer 4

Is this C or C++? In C you need to allocate space on your own for char arrays (strings):

char *cpy_key = 0 ;
...
size_t key_len = strlen (key) + 1 ; // adding space for terminating NULL
...
cpy_key = malloc ( key_len ) ; // add error management here
...
strncpy ( cpy_key , key , key_len ) ; // add error management here
...
free ( cpy_key ) ; // when you don't need it anymore