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;
}

CodePudding user response：

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.

CodePudding user response：

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.

CodePudding user response：

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;
}