2

I am new to C and learning structs. I am trying to malloc a char pointer with size 30 but it is giving a segmentation fault(core dump). I searched it on the internet & SO but am not able to resolve this. Any help will be much appreciated.
Probably I am accessing the char* member of the struct incorrectly ?

typedef struct{
int x;
int y;
char *f;
char *l;
}str;

void create_mall();

void create_mall() //Malloc the struct
{
str *p;
p->f = (char*)malloc(sizeof(char)*30);  // segmentation fault here
p->l = (char*)malloc(sizeof(char)*30);
printf("Enter the user ID:");
scanf("%d",&p->x);
printf("\nEnter the phone number:");
scanf("%d",&p->y);
printf("\nEnter the First name:");
scanf("%29s",p->f);
printf("\nEnter the Last name:");
scanf("%29s",p->l);
printf("\nEntered values are: %d %d %s %s\n",p->x,p->y,p->f,p->l);
}

int main(void)
{
create_mall();
return 0;
}
S.L. Barth is on codidact.com
  • 8,198
  • 71
  • 51
  • 66
Vbp
  • 1,912
  • 1
  • 22
  • 30
  • Note that the declaration `void create_mall();` simply announces the presence of a function called `create_mall()` that returns no value but which takes any (fixed) number of arguments of indeterminate type. This is quite different from `void create_mall(void);` which says there's a function called `create_mall()` that returns no value and takes no arguments. In other words, what you've provided is not a prototype for the function in C. (In C++, it would be a prototype for a function taking no arguments and returning no value, but the language tag is C, not C++.) – Jonathan Leffler Oct 18 '12 at 05:10
  • Congratulations. Your question / problem did appear in my programming exam (including your text). – Sam Feb 02 '16 at 23:31

5 Answers5

8

Here's your problem:

str *p;

You've declared a pointer to an instance of str, but you haven't initialized it with a value. You either need to move this variable to the stack:

str p;

...or malloc some memory for it first:

str *p = (str*)malloc(sizeof(str));
Adam Maras
  • 26,269
  • 6
  • 65
  • 91
  • 2
    Pretty please, don't advice beginners to cast the return value of malloc()! –  Oct 18 '12 at 05:14
  • Why not, exactly? It promotes explicit programming and keeping tabs on your variable types. – Adam Maras Oct 18 '12 at 05:19
  • 1
    @AdamMaras [Here's why.](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) Basically it's unnecessary and it clutters code and decreases readability. This is C, not C++. –  Oct 18 '12 at 05:25
  • @H2CO3 I disagree entirely with your reasoning. This makes the code incompatible with C++ compilers and less explicit. And the argument about forgetting to `#include ` is crap... if you can't have the foresight to include the right headers for your functions, your application has no business executing without errors. – Adam Maras Oct 18 '12 at 05:31
  • 1
    @AdamMaras "And the argument about forgetting to `#include ` is crap" <- no, it isn't. I sometimes forget to include the right headers - God bless the compiler that it warns me then. "This makes the code incompatible with C++ compilers " <- Any decent C++ compiler should have a C mode. For example, GCC deduces the language from the file extension, regardless of whether one uses gcc or g++. "Less explicit" <- What kind of explicitness are you expecting here? `void *` is *implicitly* compatible with any data pointer type. –  Oct 18 '12 at 05:35
  • @H2CO3 I reverted my answer back to its original content. I'm choosing to do so because this argument is very subjective, and my answer is more consistent with the code posted in the question. – Adam Maras Oct 18 '12 at 05:36
5

You never allocated space for the struct itself, only a pointer to it.

Try something like:

str *p = malloc(sizeof(str));
Chris Stratton
  • 39,853
  • 6
  • 84
  • 117
2

As many people have pointed out, you need to allocate memory for that str struct, before writing the fields of it.

The best way to do so in C is:

p = malloc(sizeof *p);

This has the following advantages:

  1. No cast, since no cast is needed in C and having a cast can hide actual errors.
  2. No duplication of type information, by using the sizeof operator to compute how much storage is needed for the value p points at.

When you then allocate the string space, you can simplify it to:

p->f = malloc(30);

Because:

  1. No cast, for the very same reason.
  2. C guarantees that sizeof (char) is always 1, so using it like you did adds nothing, 1 * 30 is always just 30.

Last, you should always check the return value of malloc() before using it, since it can fail and return NULL.

Community
  • 1
  • 1
unwind
  • 391,730
  • 64
  • 469
  • 606
0

Check for NULL values in return of malloc() function.

Also str *p; < is not initialised.

initialize p as str *p = malloc(sizeof(str));

Aniket Inge
  • 25,375
  • 5
  • 50
  • 78
0

The problem lies here.

str *p;   ---> Problem Line 1<br>
p->f = (char*)malloc(sizeof(char)*30); ----> Problem  Line2
p->l = (char*)malloc(sizeof(char)*30);

You have declared a pointer p of type str.
Problem 1:
You have not initialized this pointer to NULL. Thus, p can point to anything.
Problem 2:
Since p is an uninitialized pointer, p->f can point anywhere which is causing the segfault. Below is the correct way

str *p = NULL;
p = malloc(sizeof(str));
// Check p for NULL
memset(p, 0, sizeof(str));

Now you have an initialized memory pointed by p. You are now free to use it as you want.

Daniel Fischer
  • 181,706
  • 17
  • 308
  • 431
Manik Sidana
  • 2,005
  • 2
  • 18
  • 29
  • if you assign `p` to the return value of malloc, it's not necessary to initialize it to `NULL` beforehand. –  Oct 18 '12 at 06:13
  • Yes, but its always better to initialize pointers to NULL at the time of declaration. If a user misses the malloc, he'll be able to identify it quickly. – Manik Sidana Oct 18 '12 at 06:22