CODEWITHSUNDEEP
cstructsegmentation-faultmalloc
linusz
linusz

Reputation: 783

Why I get segfault when initializing the struct?

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;
}

Upvotes: 1

Views: 3058

Answers (3)

unwind
unwind

Reputation: 399753

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.

Upvotes: 5

nio
nio

Reputation: 5289

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.

Upvotes: 1

David Ranieri
David Ranieri

Reputation: 41017

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);
}

Upvotes: 2

Related Questions