Taking string input using pointers in c gives me segmentation fault in one function. Same block of code works fine for another function?

Question

So I want to take inputs of names as strings using char* pointers. I looked online and found a solution by giving my pointer memory using malloc and then taking the input using scanf, as shown below:

  char *userInput = (char*)malloc(20 * sizeof(char));
  printf("\nEnter a name: ");
  scanf("%s",userInput);
  char *name = (char*)malloc(getLength(userInput)* sizeof(char));
  name = userInput;
  if(name == NULL){
    printf("Memory not allocated");
    exit(0);
  }
  free(userInput);

This works, so I copied and pasted it to another function where similar input is required as below:

  char *userInput = (char*)malloc(20 * sizeof(char));
  printf("\nEnter a name: ");
  scanf("%s",userInput);
  char *searched = (char*)malloc(getLength(searched)* sizeof(char));
  searched = userInput;
  if(searched == NULL){
    printf("Memory not allocated");
    exit(0);
  }
  free(userInput);

But when I run the code for this function, it gives me "exited, segmentation fault".

Any ideas on why it doesn't work in my second function?

EDIT: getLength() is a function that returns the length of a given string.

Answer 1

The code you found online, if you reproduced it correctly, is wrong. The problem is that you're freeing userInput, even though searched points to that memory.

You should copy the string from userInput to searched before freeing it.

But there's no need to use dynamic allocation for userInput. You can just use a local array.

You should use the length of userInput when allocating searched. And you need to add 1 to allow room for the null terminator.

You should check whether the allocation was successful before trying to copy userInput into it.

  char userInput[20];
  printf("\nEnter a name: ");
  scanf("%19s",userInput); // limit input length to 19 so it fits in userInput
  char *searched = malloc((strlen(userInput)+1)* sizeof(char));
  if(searched == NULL){
    printf("Memory not allocated");
    exit(0);
  }
  strcpy(searched, userInput);

Answer 2

The two code blocks differs here:

char *name = (char*)malloc(getLength(userInput)* sizeof(char));
      ^^^^                           ^^^^^^^^^

char *searched = (char*)malloc(getLength(searched)* sizeof(char));
      ^^^^^^^^                           ^^^^^^^^^

The second uses searched twice. So you different modify the original code correctly.

However, notice that both code blocks are wrong.

There are several problems with the code (both examples).

scanf("%s",userInput); is real bad as it let the user overflow your buffer. Look at fgets instead or at least do scanf("%19s",userInput);

Here:

char *name = (char*)malloc(getLength(userInput)* sizeof(char));
name = userInput;

the malloc line is useless as you overwrite name with userInput immediately after. So all malloc gives you is a memory leak.

And here

free(userInput);

you free the memory that userInput points to. However, since you did name = userInput; it is also the memory that name points to. So after the free, none of the pointers are valid.

My guess is that instead of:

name = userInput;

you would want

strcpy(name, userInput);

That said - I'm don't know what getLength is but maybe the malloc should be:

char *name = (char*)malloc(1 + getLength(userInput)* sizeof(char));
                           ^^^

to get memory for the string termination. At least that is what you do when using strlen

So:

  char *userInput = malloc(20);
  if(userInput == NULL){
    printf("Memory not allocated");
    exit(0);
  }

  printf("\nEnter a name: \n");
  scanf("%19s",userInput);

  char *searched = malloc(1 + strlen(userInput ));
  if(searched == NULL){
    printf("Memory not allocated");
    exit(0);
  }

  strcpy(searched, userInput);
  free(userInput);

But... what is the real purpose of the code?

It seems to be that searched shall hold (in dynamic allocated memory) a string entered by the user and that the amount dynamic allocated memory shall be exactly what is required to hold the string.

That could make sense in some application but not in your case!

The first malloc is for 20 chars. Consequently, the second malloc will be for 20 or less chars.

Since malloc has a memory overhead and some alignment requirement, the amount of real memory required by, e.g. malloc(20) is more than 20 bytes. In other words - the difference in real memory used by malloc(20) and malloc(10) is likely to be small. So the whole idea of the code is pretty useless - you don't save significant memory by doing the second malloc and the string copy.

So you should simply do:

  char *searched = malloc(20);
  if(searched == NULL){
    printf("Memory not allocated");
    exit(0);
  }

  printf("\nEnter a name: \n");
  scanf("%19s",searched);

  // Go on using searched
  ...
  ...


  // Somewhere later in the code, call free
  free(searched);

The original code only makes sense if your program sometimes gets very long strings as input and sometimes very short. In that case the first malloc would be for a much bigger number, e.g. 1000000 chars, and then it would make sense to copy the input to a smaller buffer afterwards.