-1

Recentely I've been taking the CS50 2020 course from Harvard University as an introduction to C programming. I'm not very experienced with the language or with coding as a whole, so I'm struggling a bit to figure out what is wrong with my code.

I wrote this little function which is suposed to take in a string and, by calling another function, encrypt the text using a Caesar cypher, then return it as a string. Problem is, I can't figure out how to return the character array as a string. I tried adding a NUL char at the end of the array after reading a bit about the problem, and it compiled alright, but when I ran the program I got the following error message:

error: address of stack memory associated with local variable 'result' returned [-Werror,-Wreturn-stack-address]
    return result;
           ^~~~~~

My code:

string encypher(string text)
{
    int length = strlen(text);
    char result[length];
    for(int i = 0; i < length; i++)
    {
        int letter_c = test_char(text[i]);
        result[i] = (char)letter_c;
    }
    result[length + 1] = '\0';
    return result;
}

2 Answers2

2

The problem here is that result, being an array, decays to a pointer to its first element when used in an expression, and that is what is being returned from the function. And because the lifetime of the array ends when the function returns, that pointer now points to an invalid memory location, and attempting to use it invokes undefined behavior.

Instead of creating a local array, use the malloc function to dynamically allocate memory. That memory is valid for the life of the program, or until the returned pointer is passed to free:

string result = malloc(length + 1);

Also, note that you need to set aside one extra byte for the null byte that is used to terminate a string.

dbush
  • 205,898
  • 23
  • 218
  • 273
1

In the line

return result;

the array decays to a pointer to the first element of the array, so it is effectively the following:

return &result[0];

This array is allocated on the stack in the function encypher, so it will no longer exist when the function returns. Therefore, the returned pointer is a dangling pointer, which means that it is pointing to memory which is no longer allocated and may be overwritten by something else. For this reason, such a pointer should not be used.

In order to allocate memory that will still exist after the function returns, you can either:

  • Use dynamic memory allocation, such as malloc.
  • Allocate the memory on the stack of the function calling encypher (instead of in the function encypher itself) and change the parameters of the function encypher to accept a pointer to that array.

In my opinion, the second solution is the cleaner solution, as it allows the caller to decide where and how the memory is allocated. Using that solution, your code would look like this:

void encypher( char *cyphertext, const char *plaintext )
{
    int length = strlen(plaintext);
    //removed: char result[length];
    for(int i = 0; i < length; i++)
    {
        int letter_c = test_char(plaintext[i]);
        cyphertext[i] = (char)letter_c;
    }
    cyphertext[length + 1] = '\0';
}

The function could now be called like this:

int main( void )
{
    char plaintext[23] = "This is the plaintext.";
    char cyphertext[23]; //make sure the buffer is large enough to store the cyphertext including the terminating null character

    encypher( cyphertext, plaintext );

    return 0;
}
Andreas Wenzel
  • 22,760
  • 4
  • 24
  • 39