segmentation fault using malloc

Question

I'm new to C, so this may be a silly question to ask:

What I want to do here is to input the data to the array of pointers to a structure and then print it out. But I get a segmentation fault when running into the insert function.

Below is my code

common.h

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

typedef struct book * Book;

struct book{
    int id;
    char *name;
};

extern int b_insert(Book *b, int id, char *name);
extern int b_print(Book books[], int len);

insert.c

#include "common.h"

int b_insert(Book *b, int id, char *name){
    Book p;
    p = (Book)malloc(sizeof(struct book));
    p->id = id;
    strcpy(p->name, name);

    *b = p;
    printf("success insert book:\n");
    printf("\tID: %d Name: %s\n", (*b)->id, (*b)->name);

    return 0;

}


int b_print(Book books[], int len){
    int i;
    printf("Book List\n");
    for(i=0; i<len; i++){
        printf("books[%d] = ID: %d, Name: %s\n", i, books[i]->id, books[i]->name);
    }
    return 0;

}

main.c

#include "common.h"
#define MAX 2

int main(){
    Book books[MAX];
    Book *b=books;
    int i;
    int id;
    char name[10];




    for(i=0; i<MAX; i++){
        printf("please input new books info\n");
        printf("ID: ");
        scanf("%d", &id);
        printf("Name: ");
        scanf("%s", name);
        if(b_insert(b, id, name) == -1){
            printf("fail to insert\n");
        }
        b++;
    }

    b_print(books, MAX);

    return 0;
}

Are you sure the error happens on the malloc ? using assert or gdb can help you pinpoint the problem — dvhh
– dvhh, Commented Jul 7, 2015 at 9:40

Spikatrix · Accepted Answer · 2015-07-07 09:46:07Z

3

Main problem:

Allocate memory for p->name before using

strcpy(p->name, name);

using malloc:

p->name = malloc(10); //Or some other size

Other problems:

Remove the cast here:
```
p = (Book)malloc(sizeof(struct book));
```
Why? Here is the answer
if(b_insert(b, id, name) == -1){ will never be true.
Check the result of malloc to check if it was successful in allocating memory.
Check the return value of all the scanfs to see if it was successful in scanning data.
Add a length modifier to the second scanf to prevent buffer overflows:
```
scanf("%9s", name); /* +1 for the NUL-terminator */
```

edited Jul 7, 2015 at 9:46

answered Jul 7, 2015 at 9:40

Spikatrix

20.3k7 gold badges44 silver badges85 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Lundin Over a year ago

Furthermore, Book p; p = (Book)malloc needs to be Book* p; p = malloc. This is one of several things causing seg faults.

Quentin Over a year ago

@Lundin it doesn't : Book is a typedef hiding a pointer. Thanks for a demonstration of why this is an awful idea :)

Lundin Over a year ago

@Quentin Ah yeah, the typedef must be changed. There's no point in using incomplete type here either.

LPs · Accepted Answer · 2015-07-07 12:10:04Z

3

You're not allocating space for name:

int b_insert(Book *b, int id, char *name){
    Book p; 
    p = malloc(sizeof(struct book));
    if (p != NULL)
    {
       p->name = malloc(strlen(name)+1); // It allocates space where the input name will be copied.

       if (p->name != NULL)
       {
          p->id = id;
          strcpy(p->name, name);

          *b = p;
          printf("success insert book:\n");
          printf("\tID: %d Name: %s\n", (*b)->id, (*b)->name);
       }
       else return -1; // No space to allocate string
    }
    else return -1; // No space to allocate struct

    return 0;
}

edited Jul 7, 2015 at 12:10

answered Jul 7, 2015 at 9:40

LPs

16.2k8 gold badges34 silver badges64 bronze badges

Comments

Gunnar · Accepted Answer · 2015-07-07 13:02:01Z

0

As mentioned before, allocate space for p->name. You should probably also use something different to read the book title, either scanf format %ms with a pointer to a char pointer, or %9s with your buffer, otherwise the title "war or peace" will also result in a segfault.

edited Jul 7, 2015 at 13:02

answered Jul 7, 2015 at 9:49

Gunnar

315 bronze badges

1 Comment

Spikatrix Over a year ago

%10s does not save space for the NUL-terminator.

macieg_b · Accepted Answer · 2015-07-07 11:37:37Z

-1

Here you create a static variable and the space for it is allocated automatically.

Book p;

You can allocate a space manually when you assign it to pointer, in this line it's not pointer but static variable.

p = (Book)malloc(sizeof(struct book));

What's more if you want to refer to attribute of static variable you should use "." instead of "->". So you have two option. Create a pointer and allocate a space for the structure and then you "->" oraz create static variable.

p->id = id;

answered Jul 7, 2015 at 11:37

macieg_b

1754 silver badges15 bronze badges

1 Comment

Spikatrix Over a year ago

See what type is Book.

Collectives™ on Stack Overflow

segmentation fault using malloc

4 Answers 4

Main problem:

Other problems:

3 Comments

Comments

1 Comment

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

Main problem:

Other problems:

3 Comments

Comments

1 Comment

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related