How can I return String to the main function?

Question

The goal is actually substituting characters in a string of plaintext to ciphertext. User input the key using the command line argument with the key input of 26 letters.

I encountered problem when I run the program, it got Segmentation fault (core dumped). During the debug the code stops working at the function line. My question is what is happening and how to solve this so that I can create a string of keys?

Here is my code lines:

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

// Declare crypting function
string encrypt(string text, string key_upper, string key_lower);
string valid_key_upper(string key);
string valid_key_lower(string key);

int main(int argc, string argv[])
{
    // Must contain an argument
    if (argc > 2 || argc < 2)
    {
        printf("Usage: ./substitution KEY\n");
        return 1;
    }

    // take the input from the commandline and validate them.
    string key_before = argv[1];
    int key_length = strlen(key_before);

    // evaluate the key length
    if (key_length != 26)
    {
        printf("Key must contain 26 characters.\n");
        return 1;
    }


    // Create initial key container
    char key[26];
    int evaluated_key = 0;

    for (int i = 0; i < key_length; i++)
    {
        // Validate so that only letters
        if (key_before[i] < 65|| key_before[i] > 122 || (key_before[i] > 90 && key_before[i] < 97))
        {
            printf("Must only contain letters!\n");
            return 1;
        }
        // Compare the current evaluated key to the existing key in the memory
        else
        {
            for (int n = 1; n < evaluated_key; n++)
            {
                if (key_before[i] == key[n])
                {
                    printf("Must not contain duplicate!\n");
                    return 1;
                }
            }
            // copy valid key to the key container
            key[i] = key_before[i];
            evaluated_key = evaluated_key + 1;
        }
    }

    // Make lower-case and upper-case function container
    string key_upper = valid_key_upper(key);
    string key_lower = valid_key_lower(key);

    // get user input of plaintext
    string plaintext = get_string("Plaintext: ");

    // function for ciphering
    string ciphertext = encrypt(plaintext, key_upper, key_lower);

    // print out the ciphered text
    printf("Ciphertext = %s\n", ciphertext);

}

string valid_key_upper(string key)
{
    // Declare variable container
    string key_upper = NULL;

    // Take the key and evaluate each character
    for (int i = 0; i < 26; i++) // evaluate for 26 characters
    {
        if (key[i] >= 65 && key[i] <= 90)
        {
            key_upper[i] = key[i];
        }
        else if (key[i] >= 97 && key[i] <= 122)
        {
            key_upper[i] = toupper(key[i]);
        }
    }
    key_upper[26] = '\0';
    return key_upper;
}

Does this answer your question? [How to access a local variable from a different function using pointers?](https://stackoverflow.com/questions/4570366/how-to-access-a-local-variable-from-a-different-function-using-pointers) — dandan78, Jun 14 '22 at 08:50
`cs50` is obscuring the fact that `string` is nothing more than a `typedef char* string;` — Ted Lyngmo, Jun 14 '22 at 08:55
To the CS50 creators/maintainers: please consider giving up the pseudo `string` type, it only causes confusion in all cases but the simplest ones. — Jabberwocky, Jun 14 '22 at 09:06
Sie note: you should avoir magic number like `65`. Use `'A'` instead which shows clearly your intention. — Jabberwocky, Jun 14 '22 at 09:23

score 0 · Accepted Answer · answered Jun 14 '22 at 09:32

Your problem is with key_upper in valid_key_upper; no space is allocated for the string, and you initialize it to NULL, so that when when you assign values to key_upper[i], you trash the stack. You must malloc key_upper before you can assign string elements to it.

score 0 · Answer 2 · answered Jun 14 '22 at 09:32

You are using null pointers to access memory.

For example within the function valid_key_upper the pointer key_upper is set to null

string key_upper = NULL;

then in the following for loop you are trying to dereference the pointer

// Take the key and evaluate each character
for (int i = 0; i < 26; i++) // evaluate for 26 characters
{
    if (key[i] >= 65 && key[i] <= 90)
    {
        key_upper[i] = key[i];
        //...

that results in undefine behavior.

There is no need to create such a string. You can convert a current character to upper or to lower case when a string is encrypted.

In general your code can be simplified. For example instead of this if statement

if (argc > 2 || argc < 2)
{
    printf("Usage: ./substitution KEY\n");
    return 1;
}

you could write

if (argc !=  2)
{
    printf("Usage: ./substitution KEY\n");
    return 1;
}

Also it is a bad idea to use magic numbers like 65 and 122. Instead use integer character constants. That makes the code more clear.

score -1 · Answer 3 · answered Jun 14 '22 at 10:57

Thank you for everybody's answer, so I used malloc in the key_upper variable and solved the problem. Allocated 27 bytes in the variable. Also, I have simplified the function.

string valid_key_upper(string key)
{
    // Declare variable container
    string key_upper = malloc(27);

    // Take the key and evaluate each character
    for (int i = 0; i < 26; i++) // evaluate for 26 characters
    {
         key_upper[i] = toupper(key[I]);
    }
    return key_upper;
}

How can I return String to the main function?

3 Answers3