Why is this program gives a segmentation fault?

Question

I was doing some struct exercises and I can't understand the segmentation fault. I've done almost everything all right, the segmentation fault is on the loop for(i = 0;i<2;i++)

#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#include<math.h>
#include<stdint.h>
#define MAX 50
int main(void)
{
    typedef struct {
        char *name;
        char *l_name;
        u_int32_t age;
    }person;
    u_int32_t i;
    person p[2];
    p->name = (char *) malloc(sizeof(char) * MAX);
    p->l_name = (char *) malloc(sizeof(char)* MAX);

    if(p->name == NULL || p->l_name == NULL){
        fprintf(stderr,"Error allocating memory");
        exit(1);
    }

    for(i = 0;i<2;i++){
        if(!fgets(p[i].name,sizeof(p[i].name),stdin)){
            fprintf(stderr,"Error reading string");
            exit(2);
        }
        if(!fgets(p[i].l_name,sizeof(p[i].l_name),stdin)){
            fprintf(stderr,"Error reading string");
            exit(3);
        }

    }
}

What do you think `p[1].name` and `p[1].l_name` are pointing to? — Kevin, Jul 13 '21 at 20:30
`p->name` is the same as `p[0].name`. So you allocated memory for the first person's name, but you never allocated anything for the second person. — Barmar, Jul 13 '21 at 20:32
victor0x3E7 `char *name;` --> `name` is a pointer, right? So the size of a pointer (`sizeof(p[i].name)`) is a few bytes. — chux - Reinstate Monica, Jul 13 '21 at 20:32
@Barmar so I need to loop to allocate the memory for all those names and last names? — victor 0x3E7, Jul 13 '21 at 20:34
@chux-ReinstateMonica so is better to use a ```char array``` instead of a pointer? — victor 0x3E7, Jul 13 '21 at 20:35
If you're going to allocate them all to the same size anyway, you might as well use an array instead of dynamic allocation. — Barmar, Jul 13 '21 at 20:36
@victor0x3E7 Either is best. Depends on coding goals. Simple fix, try `fgets(p[i].name, MAX ,stdin)` and allocate for `p[1].name`, etc. — chux - Reinstate Monica, Jul 13 '21 at 20:37
one comment: don't cast malloc output in C. It is only required in C++. — estebarb, Jul 13 '21 at 21:22

Vlad from Moscow · Accepted Answer · 2021-07-13T20:40:00.060

You declared an array of two elements

person p[2];

And you initialized data members only of the first element p[0]

p->name = (char *) malloc(sizeof(char) * MAX);
p->l_name = (char *) malloc(sizeof(char)* MAX);

The above statements are equivalent to

p[0].name = (char *) malloc(sizeof(char) * MAX);
p[0].l_name = (char *) malloc(sizeof(char)* MAX);

Data members of the second element p[1] are not initialized and have indeterminate values.

So the for loop invokes undefined behavior when you are trying to use these uninitialized data members of the second element of the array as for example

if(!fgets(p[i].name,sizeof(p[i].name),stdin)){

And moreover you are using an incorrect expression in the call of fgets

sizeof(p[i].name)

Data member name (and also l_name) is a pointer. So the above expression will yield the size of a pointer.

Instead you need to write just MAX as for example

if(!fgets(p[i].name, MAX, stdin )){

And `sizeof(char)` is 1 by definition, so `sizeof(char) *` can be omitted everywhere. Then there's the problem of `sizeof(p[i].name)` being the size of the pointer, not the size of the allocation to which it points. — Fred Larson, Jul 13 '21 at 20:39

chux - Reinstate Monica · Answer 2 · 2021-07-13T21:47:18.927

Code fails to allocate for all p[i].name.

Wrong size passed to fgets().

Consider allocating after reading. Read into a local buffer and then form a right-sized copy.

for(i = 0; i<2; i++) {
  char buf[MAX + 2];

  if (fgets(buf, sizeof buf, stdin)) {
        fprintf(stderr,"Error reading string");
        exit(2);
    }
  buf[strcspn(buf, "\n")] = 0; // Lop off potential \n
  p[i].name = strdup(buf);     // Common, but non-standard string allocation

  if (fgets(buf, sizeof buf, stdin)) {
        fprintf(stderr,"Error reading string");
        exit(3);
    }
  buf[strcspn(buf, "\n")] = 0;
  p[i].lname = strdup(buf);
}

Sample strdup() if you do not have it in your library.

Why is this program gives a segmentation fault?

2 Answers2