Segmentation fault in C code using `string` from cs.50.h

Question

I have a program here where I'm trying to decode a string of letters using a ceasar cipher; essentially I'm moving each character in the string "down" a letter ("a" -> "b", "f" -> "g", "z" -> "a").

The amount that I move a letter down depends on the key I give it.

In this specific program, I have a secret coded message hardcoded into the main() function, and a for loop iterating through each possible key.

The idea is that if this secret message is simply shifted downward by x amount of letters, spitting out 25 versions of the cipher will reveal an intelligible answer.

Unfortunately I'm using some concepts that are new to me - argc, argv, and multiple functions in one program. I am very new to this.

Can someone help explain the segmentation fault error I'm getting? I don't think I'm causing any overflows on arguments.

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

string decode(int key, string message);

int main(void)
{
    string secret_message = "ueuag rKJ AGIQ GIAR FEgN";

    for (int i = 0; i < 25; i++)
    {
        decode(i, secret_message);
        printf("%s", secret_message);
    }

    return 0;
}

string decode(int key, string message)
{
    int i;

    for (i = 0; i < strlen(message); i++)
    {
        if (message[i] >= 'a' && message[i] <= 'z')
        {
            message[i] = ((message[i] - 97 + key) % 26) + 97;
        }
        else if (message[i] >= 'A' && message[i] <= 'Z')
        {
            message[i] = ((message[i] - 65 + key) % 26) + 65;
        }
    }

    return message;
}

Answer 1

Why is string a bad idea for a type in c?, here you can see an example. You are modifying a string literal and you shouldn't. Doing it invokes undefined behavior. Instead of having a string type you should treat strings as what they are in c.

Instead do it like this

char secret_message[] = "ueuag rKJ AGIQ GIAR FEgN";

And the decode function

char *decode(int key, char *message)
{
    int i;

    for (i = 0; message[i] != '\0'; i++)
    {
        if (message[i] >= 'a' && message[i] <= 'z')
        {
            message[i] = ((message[i] - 97 + key) % 26) + 97;
        }
        else if (message[i] >= 'A' && message[i] <= 'Z')
        {
            message[i] = ((message[i] - 65 + key) % 26) + 65;
        }
    }

    return message;
}

As you can see I am treating the string as an array because that's what it is, an array of bytes with a '\0' at the end. If you knew this you would never do something like typedef char * string because it's very misleading.

In cs50.h string is a typedef for a char pointer, and a pointer is not a string, a string is a sequence of bytes and a char * pointer can point to an array of char which could be a string if you define it and initialize it correctly. But a char * pointer could point to a string literal, and you can't alter them which is not clear if you define it as

string string_literal = "Do not attempt to modify me, it's undefined behavior";

When declaring a pointer to a string literal you should use const char * to avoid accidentaly trying to modify it.

Also, typedefing pointers in my opinion has no benefit at all and causes a lot of confusion, a pointer is a pointer and must have a * near it's identifier when declared, if you remove the need for a * you can easily overlook pointers in the code and be confused.