Segmentation Fault in Simple Offset Encryption

Question

Alright guys, this is my first post here. The most recent assignment in my compsci class has us coding a couple of functions to encode and decode strings based on a simple offset. So far in my encryption function I am trying to convert uppercase alphas in a string to their ASCII equivalent(an int), add the offset(and adjust if the ASCII value goes past 'Z'), cast that int back to a char(the new encrypted char) and put it into a new string. What I have here compiles fine, but it gives a Segmentation Fault (core dumped) error when I run it and input simple uppercase strings. Where am I going wrong here? (NOTE: there are some commented out bits from an attempt at solving the situation that created some odd errors in main)

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

char *encrypt(char *str, int offset){
   int counter; 
   char medianstr[strlen(str)];
   char *returnstr;// = malloc(sizeof(char) * strlen(str));
   for(counter = 0; counter < strlen(str); counter++){
      if(isalpha(str[counter]) && isupper(str[counter])){//If the character at current index is an alpha and uppercase
         int charASCII = (int)str[counter];//Get ASCII value of character
         int newASCII;       
         if(charASCII+offset <= 90 ){//If the offset won't put it outside of the uppercase range
            newASCII = charASCII + offset;//Just add the offset for the new value
            medianstr[counter] = (char)newASCII;
         }else{
            newASCII = 64 + ((charASCII + offset) - 90);//If the offset will put it outside the uppercase range, add the remaining starting at 64(right before A)
            medianstr[counter] = (char)newASCII;
         }
      } 
   }
   strcpy(returnstr, medianstr);
   return returnstr;
}
/*
char *decrypt(char *str, int offset){

}
*/

int main(){
   char *inputstr;
   printf("Please enter the string to be encrypted:");
   scanf("%s", inputstr);
   char *encryptedstr;
   encryptedstr = encrypt(inputstr, 5);
   printf("%s", encryptedstr);
   //free(encryptedstr);
   return 0;
}

Answer 1

You use a bunch of pointers, but never allocate any memory to them. That will lead to segment faults.

Actually the strange thing is it seems you know you need to do this as you have the code in place, but you commented it out:

char *returnstr;// = malloc(sizeof(char) * strlen(str));

When you use a pointer you need to "point" it to allocated memory, it can either point to dynamic memory that you request via malloc() or static memory (such as an array that you declared); when you're done with dynamic memory you need to free() it, but again you seem to know this as you commented out a call to free.

Just a malloc() to inputstr and one for returnstr will be enough to get this working.

Answer 2

Without going any further the segmentation fault comes from your use of scanf().

Segmentation fault occurs at scanf() because it tries to write to *inputstr(a block of location inputstr is pointing at); it isn't allocated at this point. To invoke scanf() you need to feed in a pointer in whose memory address it points to is allocated first.

Naturally, to fix the segmentation fault you want to well, allocate the memory to your char *inputstr.

To dynamically allocate memory of 128 bytes(i.e., the pointer will point to heap):

char *inputstr = (char *) malloc(128);

Or to statically allocate memory of 128 bytes(i.e., the pointer will point to stack):

char inputstr[128];

Answer 3

There is a lot of complexity in the encrypt() function that isn't really necessary. Note that computing the length of the string on each iteration of the loop is a costly process in general. I noted in a comment:

What's with the 90 and 64? Why not use 'A' and 'Z'? And you've commented out the memory allocation for returnstr, so you're copying via an uninitialized pointer and then returning that? Not a recipe for happiness!

The other answers have also pointed out (accurately) that you've not initialized your pointer in main(), so you don't get a chance to dump core in encrypt() because you've already dumped core in main().

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

char *encrypt(char *str, int offset)
{
    int len = strlen(str) + 1;
    char *returnstr = malloc(len);
    if (returnstr == 0)
        return 0;
    for (int i = 0; i < len; i++)
    {
        char c = str[i];
        if (isupper((unsigned char)c))
        {
            c += offset;
            if (c > 'Z')
                c = 'A' + (c - 'Z') - 1;
        }
        returnstr[i] = c;
    }
    return returnstr;
}

Long variable names are not always helpful; they make the code harder to read. Note that any character for which isupper() is true also satisfies isalpha(). The cast on the argument to isupper() prevents problems when the char type is signed and you have data where the unsigned char value is in the range 0x80..0xFF (the high bit is set). With the cast, the code will work correctly; without, you can get into trouble.