Segmentation Fault While Manipulating String-CodePudding

I'm trying to create a Ceasar Cipher. For this test, I used a key of 1 and plain text of "hello." When doing so, I get the error message "Segmentation fault (core dumped)". I know this means I am trying to access an illegal memory location, and it's happening while calling the "encrypt" function, but that's all I know.

Depiction of the error I'm getting while debugging.

And here is my code.

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

bool isValidKey(string);
string encrypt(string, int);

string c;

int main(int argc, string argv[])
{
    if (argc != 2 || isValidKey(argv[1]) == 0)
    {
        printf("Useage: ./caesar key\n");
        return 0;
    }
    string p = get_string("plaintext: ");
    c = encrypt(p, atoi(argv[1]));
    printf("%s", c);
    return 0;
}

bool isValidKey(string key)
{
    for (int i = 0; i < strlen(key); i  )
    {
        if (isdigit(key[i]) == 0)
        {
            return false;
        }
    }
    return true;
}

string encrypt(string plain, int k)
{
    for (int i = 0; i < strlen(plain); i  )
    {
        if (isalpha(plain[i]) != 0)
        {
            if (islower(plain[i]) != 0)
            {
                c[i] = ((plain[i] - 97   k) % 26)   97;
            }
            else
            {
                c[i] = ((plain[i] - 65   k) % 26)   65;
            }
        }
    }
    return c;
}

CodePudding user response：

You need to allocate memory for c.

string encrypt(string plain, int k)
{
    c = malloc(strlen(plain)   1);

    for (int i = 0; i < strlen(plain); i  )
    {
        if (isalpha(plain[i]) != 0)
        {
            if (islower(plain[i]) != 0)
            {
                c[i] = ((plain[i] - 97   k) % 26)   97;
            }
            else
            {
                c[i] = ((plain[i] - 65   k) % 26)   65;
            }
        }
    }
    return c;
}

And in main() you should add free(c); before return 0;

CodePudding user response：

The variable c declared in the file scope

string c;

is a null pointer. The above declaration is equivalent to

char *c;

or in fact to

char *c = NULL;

So using the null pointer within the function encrypt like

c[i] = ((plain[i] - 97   k) % 26)   97;

invokes undefined behavior.

Also it is a bad idea when a function like encrypt depends on a global variable.

Also using magic numbers like for example 97 makes the program unclear.

As the function parameter does not have the qualifier const then it will be logical to convert the passed string in place.

Pay attention to that the user can pass a very big integer value as a key. In this case your function will invoke undefined behavior.

So the function can look like

string encrypt( string plain, int k)
{
    k = k % 26;

    for ( size_t i = 0, n = strlen( plain ); i < n; i   )
    {
        if ( isalpha( ( unsigned char )plain[i] ) )
        {
            if ( islower( plain[i] ) )
            {
                plain[i] = ( plain[i] - 'a'   k ) % 26   'a';
            }
            else
            {
                plain[i] = ( plain[i] - 'A'   k ) % 26   'A';
            }
        }
    }

    return plain;
}

The variable c should be removed and in main it is enough to write

string p = get_string( "plaintext: " );
puts( encrypt( p, atoi( argv[1] ) ) );