Segmentation Fault when trying to use scanf on a s

问题:

I'm pretty new to c and I'm pretty frustrated at the moment as well. Here's the code I have:

typedef struct {

char* fName;
char* lName;
char* pNum;
char* address;
char* email;
} contactInfo;

void addContact(){
contactInfo *contact;
contact = (contactInfo *) malloc (sizeof(contactInfo));

printf("\n[Add a contact]\nFirst Name: ");
scanf("%s", contact->fName);
printf("%s", contact->fName);
}

For some reason when I enter a value for the scanf it gives me a segmentation fault. If I try to add a & in front of the contact->fName I get an error as well.

What's wrong with the code?

回答1:

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:

1. You'll need to allocate some space for the char*s to point to (this is what's causing the crash)
2. Make sure you check the return value from malloc
3. Make sure ask scanf() to only read as many characters as you can hold in your string.
4. No need to cast the return value from malloc.
5. 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:

1. 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.

2. 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:

1. Using a #define for the length of the string can avoid problems and make it easier to change your code later.
2. fgets() is easier to use than scanf(), and is more compatible with using a #define for the string length.

回答2:

As per the suggestion from @AdamMihalcin, I have give a almost complete code which I hope will serve you as a reference.

Few points to NOTE:

1. The return value of malloc() should be checked. Because malloc() gets memory from heap and if there is not enough memory, then malloc() may return NULL. To know more about malloc you can read its man page - man malloc

2. All the malloc'ed memory must be free'ed.

3. Difference between scanf() and fgets() and C - scanf() vs gets() vs fgets() explains why fgets() is preferred over scanf()

The code is as follows:

#include <stdio.h>
#include <stdlib.h>

/* 
define the length of each filed 
in the contactInfo struct 
*/
#define L_fName     10
#define L_lName     10
#define L_pNum      10
#define L_address   25  
#define L_email     15

typedef struct {
    char* fName;
    char* lName;
    char* pNum;
    char* address;
    char* email;
} contactInfo;

contactInfo * release_ci(contactInfo * contact) 
{
    if (contact == NULL) return NULL;
    free(contact->fName);
    free(contact->lName);
    free(contact->pNum);
    free(contact->address);
    free(contact->email);
    free(contact);
    return NULL;
}

contactInfo * alloc_ci()
{
    contactInfo *contact;
    if ((contact = malloc(sizeof(contactInfo))) == NULL) {
        printf("ERROR: unable to allocate memory for contactInfo \n");
        goto free_and_fail;
    }

    if ((contact->fName = malloc(sizeof(char) * L_fName)) == NULL) {
        printf("ERROR: unable to allocate memory for fName\n");
        goto free_and_fail;
    }

    if ((contact->lName = malloc(sizeof(char) * L_lName)) == NULL) {
        printf("ERROR: unable to allocate memory for lName\n");
        goto free_and_fail;
    }

    if ((contact->pNum = malloc(sizeof(char) * L_pNum)) == NULL) {
        printf("ERROR: unable to allocate memory for pNum\n");
        goto free_and_fail;
    }

    if ((contact->address = malloc(sizeof(char) * L_address)) == NULL) {
        printf("ERROR: unable to allocate memory for address\n");
        goto free_and_fail;
    }

    if ((contact->email = malloc(sizeof(char) * L_email)) == NULL) {
        printf("ERROR: unable to allocate memory for email\n");
        goto free_and_fail;
    }

    return contact;

free_and_fail:
    release_ci(contact);
    return NULL;
}

int main()
{
    contactInfo *ci = alloc_ci();

    if (!ci) return -1;

    printf("Enter fName     : ");
    fgets (ci->fName,   L_fName,    stdin);    
    printf("Enter lName     : ");
    fgets (ci->lName,   L_lName,    stdin);    
    printf("Enter pNum      : ");
    fgets (ci->pNum,    L_pNum,     stdin);    
    printf("Enter address   : ");
    fgets (ci->address, L_address,  stdin);    
    printf("Enter email     : ");
    fgets (ci->email,   L_email,    stdin);    

    /* TODO: validation for all the input fields */

    release_ci(ci);
    return 0;
}

回答3:

You should allocate memory for all the char * in your struct.

For example:

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

Also, you should check the return value of malloc()