Why I get segfault when initializing the struct?

Question

Searched around for one hour. I guess I'd better post the question here.

I simplify the code. The segfault is in the function initMyStruct.

#include "stdlib.h"

typedef struct {
        int * arr1;
        int * arr2;
} myStruct;

void allocMyStruct (myStruct * a, int num) {
        a = malloc(sizeof(myStruct));
        a->arr1 = malloc(10*sizeof(int));
        a->arr2 = malloc(10*num*sizeof(int));
}
void initMyStruct (myStruct * a, int num) {
        int i;
        for (i = 0; i < 10; i++)     a->arr1[i]  =  0;
        for (i = 0; i < 10*num; i++) a->arr2[i]  = -1;
}
void freeMyStruct (myStruct * a, int num) {
        int i;
        for (i = 0; i < 10; i++)     free(a->arr1);
        for (i = 0; i < 10*num; i++) free(a->arr2);
        free(a);
}
int main (void) {
        int num = 3;
        myStruct * a;
        allocMyStruct (a, num);
        initMyStruct  (a, num);
        freeMyStruct  (a, num);
        return 1;
}

unrelated, but why is your `main` function returning 1? if a program returns 1, that's indicating a failure of some kind AFAIK. the std lib has macro's defined: `#define EXIT_SUCCESS 0` and `#define EXIT_FAILURE 1`... — Elias Van Ootegem, Nov 04 '13 at 10:59

unwind · Accepted Answer · 2013-11-04T10:44:38.720

5

Because you're not keeping the pointer to the newly allocated memory, instead you use an uninitialized pointer and getting undefined behavior.

You pass the a variable into allocMyStruct(), but that call is (like all others) by value, so the new value being assigned to it inside the function does not affect the value of a in main().

Change it so that allocMyStruct() either returns the new pointer value, or takes a pointer to the pointer. I would prefer the former, it's cleaner and using function return values often leads to better code:

myStruct * allocMyStruct(int num)
{
  myStruct *p;

  if((p = malloc(sizeof *p +
                 10 * sizeof *p->arr1 +
                 10 * num * sizeof *p->arr2)) != NULL)
  {
    p->arr1 = (int *) (p + 1);
    p->arr2 = p->arr1 + 10;
  }
  return p;
}

The above code also streamlines the memory allocation, doing it all in one big malloc() call which is then "sliced" into the three parts you actually need.

If the size of arr1 is always 10 by the way, there's no point in having it dynamically allocated, it should just be int arr1[10]; in the struct declaration.

edited Nov 04 '13 at 10:44

answered Nov 04 '13 at 10:28

unwind

391,730
64
469
606

1

+1 for the optimized `malloc` call I'm still awaiting my copy of _the C programming language_, so forgive my asking: is `p+1` always going to be correct, or should it be `p + sizeof *void` or something? – Elias Van Ootegem Nov 04 '13 at 11:39
1

How do you know that `(int *)(p + 1)` is aligned for `p->arr1`? – David Ranieri Nov 04 '13 at 12:23
1

@AlterMann: since the struct only contains `int *` in this case, there probably won't be any need for structure alignment here. But perhaps it might be worth mentioning the risks of just assuming that `p->arr1` will be at `p + 1`, the moment the struct grows, and new types are added, I suspect there could be trouble, unless the amount of bits for each member are specified explicitly... or maybe I'm just being paranoid? – Elias Van Ootegem Nov 04 '13 at 12:31
1

@AlterMann: Damn, I've gone and commented too soon again. I completely missed and overlooked the _"is aligned for `p->arr1`"_ part of your commend. Well, I missed the word _aligned_. I believe the answer to that is: if this is what the actual struct looks like, then yes, everything should be aligned nicely. Add a short or char in there, and you're in trouble, I think. But what I'm wondering is that, since pointers are aligned differently on 64bit vs 32bit systems (4 bytes vs 8 bytes) if a simple +1 is enough... it should be, but it feels unsafe to me... – Elias Van Ootegem Nov 04 '13 at 12:44

David Ranieri · Answer 2 · 2013-11-04T10:35:52.410

a is used uninitialized, change to:

myStruct * allocMyStruct (int num) {
        myStruct *a;

        a = malloc(sizeof(myStruct));
        a->arr1 = malloc(10*sizeof(int));
        a->arr2 = malloc(10*num*sizeof(int));
        return a;
}
myStruct * a = allocMyStruct(num);

Also, there is no need to loop in your free function

void freeMyStruct (myStruct * a, int num) {
        int i;
        for (i = 0; i < 10; i++)     free(a->arr1);
        for (i = 0; i < 10*num; i++) free(a->arr2);
        free(a);
}

Must be

void freeMyStruct (myStruct * a) {
        free(a->arr1);
        free(a->arr2);
        free(a);
}

nio · Answer 3 · 2013-11-04T10:36:19.560

When you call void allocMyStruct (myStruct * a, int num) the a pointer will be passed as a value and the a parameter is a local copy of your pointer from main , after you change the local a in any of your three functions, it will not change in main.

For this you have to use double pointer as a function argument, so those functions will get an address of a pointer so they can modify it.

#include "stdlib.h"

typedef struct {
        int * arr1;
        int * arr2;
} myStruct;

void allocMyStruct (myStruct ** a, int num) {
        *a = malloc(sizeof(myStruct));
        (*a)->arr1 = malloc(10*sizeof(int));
        (*a)->arr2 = malloc(10*num*sizeof(int));
}
void initMyStruct (myStruct ** a, int num) {
        int i;
        for (i = 0; i < 10; i++)     (*a)->arr1[i]  =  0;
        for (i = 0; i < 10*num; i++) (*a)->arr2[i]  = -1;
}
void freeMyStruct (myStruct ** a, int num) {
        free((*a)->arr1);
        free((*a)->arr2);
        free(*a);
        *a = NULL;
}
int main (void) {
        int num = 3;
        myStruct * a;
        allocMyStruct (&a, num);
        initMyStruct  (&a, num);
        freeMyStruct  (&a, num);
        return 1;
}

EDIT: Alter Mann is right about multiple freeing of the same address, on linux you would get instant crash for double freeing. And he has a simpler solution.

Why I get segfault when initializing the struct?

3 Answers3

Linked