Segmentation Fault when trying to use scanf on a struct

c, malloc, scanf, segmentation-fault, struct

Solution

First off, don't worry - frustration is normal with beginning C :)

Since you say you're a beginner, I've written a pretty long answer that explains some other improvements you might want to make. Sorry if I cover some things you already know. Here's a summary:

- You'll need to allocate some space for the `char*`s to point to (this is what's causing the crash)

- Make sure you check the return value from malloc

- Make sure ask `scanf()` to only read as many characters as you can hold in your string.

- No need to cast the return value from malloc.

- Remember to `free()` anything you've malloc-ed.

You'll need to allocate some space for the `char*`s to point to

In C, a `char*` means "a pointer to a char". `char*` is commonly used for strings, since you can index pointers like they were arrays - for example, assuming:

 char *a = "Hello";

Then, `a[1]` means "the first `char` after the char pointed to by `a`, in this case `'e'`;

You have this code:

contactInfo *contact;
contact = (contactInfo *) malloc (sizeof(contactInfo));

At this point, you've declared a pointer to a contactInfo struct, and allocated memory of the right size to it. However, the pointers inside the structure currently don't point to anything - so your program is crashing when it calls `scanf()`. You need to also allocate space for the characters you're about to read, for example:

contact->fName = malloc(sizeof(char) * 10);

Will allocate space for 10 chars. You'll need to do this for every `char*` in the struct.

A couple of asides that I don't want you to worry about too much:

- In C, `sizeof(char)` is always 1, so you could have written `malloc(10)`, but in my opinion that's less readable.

You can also do something like:

contact->fName = malloc(sizeof(*(contact->fName)) * 10);

This is robust to changes in the type of `fName` - you'll always allocate enough space for 10 of whatever `fName` points to.

Make sure you check the return value from malloc

Back on track now - you should also check the return value from `malloc()`:

contact->fName = malloc(sizeof(char) * 10);
if(contact->fName == NULL) {
   // Allocation failed 
}

In some cases, you might be able to recover from a failed allocation (say, trying to allocate again, but asking for less space), but to start with:

contact->fName = malloc(sizeof(char) * 10);
if(contact->fName == NULL) {
   printf(stderr,"Allocation of contact->fName failed");
   exit(EXIT_FAILURE);
}

Is probably fine. Many programmers will write a wrapper for `malloc()` that does this error checking for them, so that they no longer have to worry about it.

Make sure you only ask `scanf()` to read as many characters as you can hold in your string.

Note that once you've allocated say 10 char in `fName`, `scanf()` might read too many characters. You can explictly tell scanf the limit by writing `"%Ns"` where N is the maximum number of characters in your string (minus 1 for the null terminator at the end). So, if you've allocated 10 characters, then you should write:

scanf("%9s", contact->fName);

No need to cast the return value from malloc.

One final point - you don't need to cast the return value of malloc in C, so I'd probably write:

 contact =  malloc (sizeof(contactInfo));

Remember to `free()` anything you've malloced

You might be doing this already, but every time you `malloc()` anything, make sure you have a corresponding `free()` in your code once you're done with it. This tells the operating system it can have the memory back. So, if you've got somewhere

 contact =  malloc (sizeof(contactInfo));

Later on, when you're done with that contact, you'll need to have something like:

 free(contact);

to avoid memory leaks.

As soon as you've freed something, you aren't allowed to access it any more. So, if you've malloced strings inside the contact, you'll have to first free them:

 free(contact->fName);  // doing this in the other order might crash
 free(contact);  

A few things to remember about free:

You can't free anything twice. To avoid this, a good practice is to write:

 if(contact != NULL) free(contact); 
 contact = NULL;

If you write this way, then you'll need to also initialise all pointers to NULL when you create them. When you create structs that have pointers in them, an easy way to do this is to use `calloc()` instead of `malloc()` to create the struct, since `calloc()` returns memory which is always zeroed.

When your program exits, all memory is released back to the operating system. This means you don't technically need to `free()` things that are around for the lifetime of the program. However, I'd recommend getting into the habit of freeing everything you've malloced, because otherwise you'll forget one day when it's important.

Further improvements

As a commenter points out on another answer, using magic numbers (numbers hard coded in your code) is generally bad practice. In the examples I've given you above, I've hard coded "10" into the program as the size of the string. However, it's better to do something like:

#define FNAME_MAX_LENGTH 10

and then later go:

malloc(sizeof(char) * FNAME_MAX_LENGTH);

This has the advantage that if you need to change the size of the string anywhere, you can change it in just one place. It also prevents you accidentally typing 100 or 1 in one place, causing a potentially serious, hard-to-find bug.

Of course, now that you've got a `#define` for the length, you'll need to update the `scanf()` call where we specify the length. However, since `scanf()` needs the length - 1, you won't be able to use the `#define` to specify the length (at least, not in any nice readable way).

Consequently, you might be interested in `fgets()`, which reads up to a specified length -1 (or until the end of the line - whichever is first). You could then do:

fgets(contact->fName,FNAME_MAX_LENGTH,stdin);

instead of the `scanf()` call. Another good reason to make this change is that `scanf()` can be kind of a pain.

So, in addition to the summary above:

- Using a #define for the length of the string can avoid problems and make it easier to change your code later.

- `fgets()` is easier to use than `scanf()`, and is more compatible with using a `#define` for the string length.

Problem

Original source

Related problems

• Specifically, what's dangerous about casting the result of malloc?
• Difference between scanf() and fgets()
• C - scanf() vs gets() vs fgets()