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;
}
Improve this question
2
  • 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++.) Commented Oct 18, 2012 at 5:10
  • Congratulations. Your question / problem did appear in my programming exam (including your text). Commented Feb 2, 2016 at 23:31

5 Answers 5

Reset to default
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));
Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

Pretty please, don't advice beginners to cast the return value of malloc()!
Why not, exactly? It promotes explicit programming and keeping tabs on your variable types.
@AdamMaras Here's why. Basically it's unnecessary and it clutters code and decreases readability. This is C, not C++.
@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 <stdlib.h> 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.
@AdamMaras "And the argument about forgetting to #include <stdlib.h> 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.
|
5

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

Try something like:

str *p = malloc(sizeof(str));
Improve this answer

8 Comments

+1 for being the only answer that didn't cast the malloc() result.
@WhozCraig why shouldn't we cast the result of malloc() ?
@mux That's why.
@H2CO3 thanks for posting the link. I was seconds from linking the same exact post =P
@PrototypeStark C is not stupid as opposed to C++.
|
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.

Improve this answer

Comments

0

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

Also str *p; < is not initialised.

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

Improve this answer

Comments

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.

Improve this answer

2 Comments

if you assign p to the return value of malloc, it's not necessary to initialize it to NULL beforehand.
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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.