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.

Welcome to Stack Overflow! [dont cast malloc](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) — Barmar, Apr 28 '20 at 05:04
You have a memory leak in `name = userInput;`. You allocated memory for `name`, but overwrite the pointer that pointed to it. — Barmar, Apr 28 '20 at 05:05
Why do you think you need to allocate memory twice? Once you allocate memory for `userInput`, you can just use that memory. — Barmar, Apr 28 '20 at 05:06
What is `getLength()` and how does it differ from `strlen()`? — Barmar, Apr 28 '20 at 05:07
`getLength(searched)` tries to find the length of an uninitialized variable -- the variable `seached` is initialized indirectly with that length only after that call. That's also the difference between the two code snippets: The original uses `getLength(userInput)`. (But take into account what Barmar has said: Don't allocate and then immediately overwrite the handle to the allocated memory.) — M Oehm, Apr 28 '20 at 05:10
(There's also the issue that you say `name = userInput` and later `free(userInput)`, which will also make the memory pointed to by `name` invalid.) — M Oehm, Apr 28 '20 at 05:12
OT: regarding: `char *userInput = (char*)malloc(20 * sizeof(char));` 1) the expression: `sizeof(char)` is defined in the C standard as 1. Multiplying anything by 1 has no effect and expression just clutters the code. Suggest removing that expression. 2) The returned type is `void*` which can be assigned to any pointer. Casting just clutters the code and is error prone. Suggest removing that case. 3) always check (!=NULL) the returned value to assure the operation was successful. (cont) — user3629249, Apr 28 '20 at 16:39
(cont) If not successful, call `perror()` to output both your error message and the text reason the system thinks the error occurred to `stderr` — user3629249, Apr 28 '20 at 16:40
regarding: `scanf("%s",userInput);` 1) when calling any of the `scanf()` family of functions, always check the returned value (not the parameter values) to assure the operation was successful Note: those functions return the number successful 'input format conversion' specifiers. 2) when using the specifier `%s` and/or `%[...]` always include a MAX CHARACTERS modifier that is 1 less than the length of the input buffer because those specifiers always append a NUL byte to the input. This also avoids any possibility of a buffer overflow and the resulting undefined behavior — user3629249, Apr 28 '20 at 16:44
regarding: `name = userInput;` that is NOT how to copy a string, Infact, all is does is overlay the pointer that was in `name`, so the original pointer is lost. This results in a memory leak. Suggest using `strcpy()` to copy the string. Note: be sure that the function: `getLength()` allows an extra character for the NUL terminating byte of the string — user3629249, Apr 28 '20 at 16:49
OT: regarding: `printf("Memory not allocated");` Error messages should be output to `stderr` and when the error is from a C library function, should also output the text reason the system thinks the error occurred. Suggest: `perror("Memory not allocated");` — user3629249, Apr 28 '20 at 16:51
OT: regarding: `exit(0);` in general, returned 0 indicates success. Suggest: `exit( EXIT_FAILURE );` Note: exit() and `EXIT_FAILURE` are exposed by `#include ` — user3629249, Apr 28 '20 at 16:53
regarding: `(getLength(searched)` this will return garbage and is undefined behavior. Suggest: `(getLength(userInput )` — user3629249, Apr 28 '20 at 16:56
@Barmar I thought that allocating memory twice would make my code more efficient, as I could use malloc to only use the necessary memory required and then free the unneeded space. — chris02, Apr 28 '20 at 17:15
If you're only allocating 20 bytes, the difference is negligible. It would make a difference if you were allocating 1000 bytes and then shrinking it. But see my answer where I use a local array instead of `malloc()` for the temporary buffer. — Barmar, Apr 28 '20 at 17:56

score 1 · Answer 1 · answered Apr 28 '20 at 05:11

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

Support Ukraine · Accepted Answer · 2020-04-28T06:56:38.727

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.

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

2 Answers2