I have no idea why I'm receiving segmentation fault errors

Question

#include <stdio.h>
#include <cs50.h> //stdlib.h is included in cs50.h so I don't need it
#include <string.h>
#include <ctype.h>
#include <math.h>

int main(int argc, string argv[]) // command line input
{
    if(argc != 2) // check if there is only one input
    {
        printf("error\n");
        return 1;
    }

    int commandlength = strlen(argv[1]); // find string length of command string

    string key[commandlength + 1]; // taking the key from the input and putting it in something that will take less typing later

    for(int i = 0; i < commandlength; i++) // check if every char in the string is a letter
    {
        if(isalpha(argv[1][i]))
            continue;

        else
        {
            printf("error\n");
            return 1;
        }
    }

    strcpy(key[commandlength], argv[1]); // copy key from command line into a string called key
    string input = GetString();
    int inputlength = strlen(input); // length of string typed in when prompted
    int k = 0; // this will be used to iterate the key separately from i, since the key only iterates when applied to an alpha

    for(int i = 0; i < inputlength; i++)
    {
        if(ispunct(input[i]))
            printf("%c", input[i]);

        if(isspace(input[i]))
            printf("%c", input[i]);

        if(isupper(input[i]))
        {
            printf("%c", (input[i] + atoi(key[k]) % commandlength - 65) % 26 + 65);
            k++;
        }

        if(islower(input[i]))
        {
            printf("%c", (input[i] + atoi(key[k]) % commandlength - 97) % 26 + 97);
            k++;
        }        
    }

    printf("\n");
    return 0;
}

Before reading, please keep in mind that I am a student. It is much more beneficial to receive a detailed explanation than simply a line of code saying "here you go I fixed it". I will be linking this post in my submission, and we do have very strict rules about academic integrity and what is copying vs helping, and I'd really appreciate if that were kept in mind.

The purpose of this project is to create a vigenere cipher. Below are the instructions my teacher provided:

Your final challenge this week is to write, in vigenere.c, a program that encrypts messages using Vigenère’s cipher. This program must accept a single command-line argument: a keyword, k, composed entirely of alphabetical characters. If your program is executed without any command-line arguments, with more than one command-line argument, or with one command-line argument that contains any non-alphabetical character, your program should complain and exit immediately, with main returning 1 (thereby signifying an error that our own tests can detect). Otherwise, your program must proceed to prompt the user for a string of plaintext, p, which it must then encrypt according to Vigenère’s cipher with k, ultimately printing the result and exiting, with main returning 0.

As for the characters in k, you must treat A and a as 0, B and b as 1, … , and Z and z as 25. In addition, your program must only apply Vigenère’s cipher to a character in p if that character is a letter. All other characters (numbers, symbols, spaces, punctuation marks, etc.) must be outputted unchanged. Moreover, if your code is about to apply the jth character of k to the ith character of p, but the latter proves to be a non-alphabetical character, you must wait to apply that jth character of k to the next alphabetical character in p; you must not yet advance to the next character in k. Finally, your program must preserve the case of each letter in p.

I'm pretty sure I hit the requirements for iterating through the code, making sure the key loops if it's shorter than the phrase being encrypted, and making sure that I'm not applying the key to non-alpha characters. However, I get a segmentation default before prompted to enter a string to encrypt, so I'm pretty sure the problem lies in the top half of the code. I've looked over the code quite a bit and I still can't figure out why it keeps segfaulting.

Answer 1

Change this statement

strcpy(key[commandlength], argv[1]); 

to

strcpy( key, argv[1] ); 

key[commandlength] is an object of type char value of which is used as an address of a string if you use this invalid construction

strcpy(key[commandlength], argv[1]); 

Also it is not a good idea to use continue in your loop. Instead of

for(int i = 0; i < commandlength; i++) // check if every char in the string is a letter
{
    if(isalpha(argv[1][i]))
        continue;

    else
    {
        printf("error\n");
        return 1;
    }
}

I would write simply

for(int i = 0; i < commandlength; i++) // check if every char in the string is a letter
{
    if( !isalpha(argv[1][i] ) )
    {
        printf("error\n");
        return 1;
    }
}

And as BLUEPIXY noted instead of

string key[commandlength + 1]; 

there shall be

char key[commandlength + 1]; 

provided that you have no some typedef similar to

typedef char string;

However if you indeed has such a typedef then the following statement

string input = GetString();

will be invalid because you are trying to call function strlen for input:

int inputlength = strlen(input); 

And do not use magic numbers in your code as for example in this statement

printf("%c", (input[i] + atoi(key[k]) % commandlength - 65) % 26 + 65);

I suppose that 65 is 'A'. So it would be better to use explicitly character literal 'A' instead of 65.

Answer 2

I am not sure what the string type is supposed to be (according to the main(int argc string argv[]) line it looks like a char*. If that is so then you get an array of char pointers with the line string key[commandlength + 1];

With the line strcpy(key[commandlength], argv[1]);

You copy the string from argv[i] to the last pointer in the array you created. But since this pointer does not (yet) have any memory assigned to. Furthermore it has random content and points to a random memory location. Therefore you get the segfault.

Probably what you want is one pointer which has the stringlength of the argv[1] string as memory associated.

Answer 3

The answer given by user johanneshau is somewhat right. To add to his/her answer, you shoud declare a variable key like below

string key;

instead of

string key[commandlength + 1];

Then you should assign memory to the above pointer variable because string is a typedef for char * and hence it can not store any string until some memory is allocated to it.

key = (string)malloc(sizeof(char) * (commandlength + 1));

The above line will allocate memory to hold input string.

Then you should copy the input string into it using following code

strcpy(key,argv[1]);

So now you have actual string copied into the key variable.

Before returning from the function (semantically program) you should free the memory using free function as we have allocated it dynamically using malloc function on heap.

free(key);

Answer 4

if you have not idea, then compile your source with following: "gcc -Wall -g source.c" ; run your program, after program crashes, run "dgb ./prog core"

into the gdb type "bt" and you will see the number of string where seg fault comes

if core dump cannot create in working directory, run following: "ulimit -c unlimited" (in bash environment)