Why does this C code give me a segmentation fault?

Question

Using the CS50 library, this is my code but it keeps giving me a segmentation fault. I have tried to figure out where it keeps doing a segmentation fault, and for some reason it has to do with my function. Why does it keep doing this?

#import <cs50.h>
#import <stdio.h>
#import <ctype.h>
#import <string.h>

string ciphertext(string plaintext[1], string key[1]);

int main(int argc, string argv[])
{
    // Variables
    string plaintext2[1];

    if (argc == 2)
    {
        if(strlen(argv[1]) == 26)
        {
            string text = get_string("plaintext: ");
            plaintext2[0] = text;
        }
        else
        {
            printf("Key must contain 26 characters.\n");
            return 1;
        }
    }
    else
    {
        printf("Usage: ./substitution key\n");
        return 1;
    }

    string ciphertext2 = ciphertext(&plaintext2[0], &argv[1]);
    printf("ciphertext: %s\n", ciphertext2);
}

string ciphertext(string plaintext[1], string key[1])
{
    // Variables
    string ciphertext[1];

    for (int i = 0; i < strlen(plaintext[0]); i++)
    {
        if (plaintext[0][i] == 'a')
        {
            ciphertext[0][i] = tolower(key[0][0]);
        }
        else if (plaintext[0][i] == 'A')
        {
            ciphertext[0][i] = toupper(key[0][0]);
        }
        else if (plaintext[0][i] == 'b')
        {
            ciphertext[0][i] = tolower(key[0][1]);
        }
        else if (plaintext[0][i] == 'B')
        {
            ciphertext[0][i] = toupper(key[0][1]);
        }
        else if (plaintext[0][i] == 'c')
        {
            ciphertext[0][i] = tolower(key[0][2]);
        }
        else if (plaintext[0][i] == 'C')
        {
            ciphertext[0][i] = toupper(key[0][2]);
        }
        else if (plaintext[0][i] == 'd')
        {
            ciphertext[0][i] = tolower(key[0][3]);
        }
        else if (plaintext[0][i] == 'D')
        {
            ciphertext[0][i] = toupper(key[0][3]);
        }
        else if (plaintext[0][i] == 'e')
        {
            ciphertext[0][i] = tolower(key[0][4]);
        }
        else if (plaintext[0][i] == 'E')
        {
            ciphertext[0][i] = toupper(key[0][4]);
        }
        else if (plaintext[0][i] == 'f')
        {
            ciphertext[0][i] = tolower(key[0][5]);
        }
        else if (plaintext[0][i] == 'F')
        {
            ciphertext[0][i] = toupper(key[0][5]);
        }
        else if (plaintext[0][i] == 'g')
        {
            ciphertext[0][i] = tolower(key[0][6]);
        }
        else if (plaintext[0][i] == 'G')
        {
            ciphertext[0][i] = toupper(key[0][6]);
        }
        else if (plaintext[0][i] == 'h')
        {
            ciphertext[0][i] = tolower(key[0][7]);
        }
        else if (plaintext[0][i] == 'H')
        {
            ciphertext[0][i] = toupper(key[0][7]);
        }
        else if (plaintext[0][i] == 'i')
        {
            ciphertext[0][i] = tolower(key[0][8]);
        }
        else if (plaintext[0][i] == 'I')
        {
            ciphertext[0][i] = toupper(key[0][8]);
        }
        else if (plaintext[0][i] == 'j')
        {
            ciphertext[0][i] = tolower(key[0][9]);
        }
        else if (plaintext[0][i] == 'J')
        {
            ciphertext[0][i] = toupper(key[0][9]);
        }
        else if (plaintext[0][i] == 'k')
        {
            ciphertext[0][i] = tolower(key[0][10]);
        }
        else if (plaintext[0][i] == 'K')
        {
            ciphertext[0][i] = toupper(key[0][10]);
        }
        else if (plaintext[0][i] == 'l')
        {
            ciphertext[0][i] = tolower(key[0][11]);
        }
        else if (plaintext[0][i] == 'L')
        {
            ciphertext[0][i] = toupper(key[0][11]);
        }
        else if (plaintext[0][i] == 'm')
        {
            ciphertext[0][i] = tolower(key[0][12]);
        }
        else if (plaintext[0][i] == 'M')
        {
            ciphertext[0][i] = toupper(key[0][12]);
        }
        else if (plaintext[0][i] == 'n')
        {
            ciphertext[0][i] = tolower(key[0][13]);
        }
        else if (plaintext[0][i] == 'N')
        {
            ciphertext[0][i] = toupper(key[0][13]);
        }
        else if (plaintext[0][i] == 'o')
        {
            ciphertext[0][i] = tolower(key[0][14]);
        }
        else if (plaintext[0][i] == 'O')
        {
            ciphertext[0][i] = toupper(key[0][14]);
        }
        else if (plaintext[0][i] == 'p')
        {
            ciphertext[0][i] = tolower(key[0][15]);
        }
        else if (plaintext[0][i] == 'P')
        {
            ciphertext[0][i] = toupper(key[0][15]);
        }
        else if (plaintext[0][i] == 'q')
        {
            ciphertext[0][i] = tolower(key[0][16]);
        }
        else if (plaintext[0][i] == 'Q')
        {
            ciphertext[0][i] = toupper(key[0][16]);
        }
        else if (plaintext[0][i] == 'r')
        {
            ciphertext[0][i] = tolower(key[0][17]);
        }
        else if (plaintext[0][i] == 'R')
        {
            ciphertext[0][i] = toupper(key[0][17]);
        }
        else if (plaintext[0][i] == 's')
        {
            ciphertext[0][i] = tolower(key[0][18]);
        }
        else if (plaintext[0][i] == 'S')
        {
            ciphertext[0][i] = toupper(key[0][18]);
        }
        else if (plaintext[0][i] == 't')
        {
            ciphertext[0][i] = tolower(key[0][19]);
        }
        else if (plaintext[0][i] == 'T')
        {
            ciphertext[0][i] = toupper(key[0][19]);
        }
        else if (plaintext[0][i] == 'u')
        {
            ciphertext[0][i] = tolower(key[0][20]);
        }
        else if (plaintext[0][i] == 'U')
        {
            ciphertext[0][i] = toupper(key[0][20]);
        }
        else if (plaintext[0][i] == 'v')
        {
            ciphertext[0][i] = tolower(key[0][21]);
        }
        else if (plaintext[0][i] == 'V')
        {
            ciphertext[0][i] = toupper(key[0][21]);
        }
        else if (plaintext[0][i] == 'w')
        {
            ciphertext[0][i] = tolower(key[0][22]);
        }
        else if (plaintext[0][i] == 'W')
        {
            ciphertext[0][i] = toupper(key[0][22]);
        }
        else if (plaintext[0][i] == 'x')
        {
            ciphertext[0][i] = tolower(key[0][23]);
        }
        else if (plaintext[0][i] == 'X')
        {
            ciphertext[0][i] = toupper(key[0][23]);
        }
        else if (plaintext[0][i] == 'y')
        {
            ciphertext[0][i] = tolower(key[0][24]);
        }
        else if (plaintext[0][i] == 'Y')
        {
            ciphertext[0][i] = toupper(key[0][24]);
        }
        else if (plaintext[0][i] == 'z')
        {
            ciphertext[0][i] = tolower(key[0][25]);
        }
        else if (plaintext[0][i] == 'Z')
        {
            ciphertext[0][i] = toupper(key[0][25]);
        }
        else
        {
            ciphertext[0][i] = plaintext[0][i];
        }
    }
    return ciphertext[0];
}

I really need some help, and I don't know why it keeps giving a fault.

Please provide more information how to run the program and which line do you get signal? — Farhad Sarvari, Jul 23 '20 at 21:29
Why are you making arrays with only 1 element? It just means you have to write `[0]` all over the place unnecessarily. — Barmar, Jul 23 '20 at 21:39
Based on the dozens of similar questions, it's clear that CS50 doesn't teach you what you need to know about strings in C. You'll need to find an online tutorial that actually explains what a C string is. Once you understand what a C string is, you need to realize that `string` in CS50 is just a `char *`, i.e. a pointer to a `char`. — user3386109, Jul 23 '20 at 21:40
There's got to be a better way to replace all the letters than 52 `else if` statements. — Barmar, Jul 23 '20 at 21:42
@Deegan This array string ciphertext[1]; is uninitialized. So accessing it like this ciphertext[0][i] = tolower(key[0][0]); results in undefined behavior. — Vlad from Moscow, Jul 23 '20 at 21:44
In the function `ciphertext`, for starters, change `string plaintext[1]` into `string plaintext` (i.e. you want a single string and not an array of string pointers). That is, you have: `char *plaintext[1]` (i.e. `char **plaintext` and you want: `char *plaintext` Then, adjust the code accordingly. Also you don't need a big `switch/case` statement. The entire function can be just a few lines. And, `ciphertext` is the name of the function. You need a separate output array. This won't even compile because you can't do: `func[...] = whatever;` inside a function named `func` — Craig Estey, Jul 23 '20 at 21:47
@user3386109: Apparently it's the [future of education](https://www.newyorker.com/news/our-local-correspondents/how-harvards-star-computer-science-professor-built-a-distance-learning-empire), though. — rici, Jul 23 '20 at 22:33

Vlad from Moscow · Answer 1 · 2020-07-23T22:02:39.597

The array

string ciphertext[1];

is uninitialized and has indeterminate value.

Accessing such an array the way like this

ciphertext[0][i] = tolower(key[0][0]);

results in undefined behavior.

You need to allocate a character array with the size equal to the value of the expression

strlen( *plaintext ) + 1

Pay attention to that instead the numerous if-else statement you could define a string literal like for example

string letters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

and then you can use the standard string function strchr like

char *p = strchr( letters, toupper( ( unsigned char)plaintext[0][i] ) );
if ( p != NULL )
{
    if ( plaintext[0][i] == *p )
    {
        ciphertext[0][i] = key[0][p - letters]);
    }
    else
    {
        ciphertext[0][i] = tolower(key[0][p - letters]);
    }
}

score 0 · Answer 2 · answered Jul 23 '20 at 21:51

You already have the answer (@Vlad from Moscow), but I would like to point out a few things:

Duplicating is a very bad practice, it is unreadable and more important seriously time-consuming. If you see "100" else if() statements one below the other you should suspect that something is terribly wrong with your code.
Try your best to avoid using numbers in your code. What if you want to change a value that you already copy-pasted a hundred times? Instead of using numbers, I would advise you to use variables and symbolic constants

Try to improve your current code, you can write something like this:

for (int i = 0; i < strlen(plaintext[0]); i++)
{
    int ind = 0;
    int lower_case = 97, upper_case = 65;
    
    if (plaintext[0][i] == lower_case)
    {
        ciphertext[0][i] = tolower(key[0][ind]);
    }
    else if (plaintext[0][i] == upper_case)
    {
        ciphertext[0][i] = toupper(key[0][ind]);
    }
    
    ind++; lower_case++; upper_case++;
}

Why does this C code give me a segmentation fault?

2 Answers2