Code crashes on c, most likely something to do with malloc?

Question

this is quite basic but I've been trying to solve it for a few hours now with no success. This code is supposed to read 3 ints from user input and then, points according to the ints (n = how many, d = dimension, k is unrelated to this part) and create an array of them. For some reason it crashes on the second loop, where it fails to create the second point, but im not sure why. I think it might has something to do with malloc but I'm totally lost, would appreciate any help.

I inserted the input :

5 3 2 
1.2 3.4 0.1

2 times before it crashed.

Code is below:

int main(){

    double* pdata;
    int n,d,k;
    scanf("%d %d %d",&n,&d,&k );
    SPPoint*  parr [n];
    for(int i=0; i<n; i++)
    {
        double darr [d];
        for(int j = 0; j < d-1; j++)
        {
            scanf(" %lf", &darr[j]);
        }
        scanf(" %lf", &darr[d-1]);
    pdata = darr;
    parr[i] = spPointCreate(pdata, d, i); 
    }
}

This is the code for the spPointCreate function:

struct sp_point_t{
double* data;
int dim;
int index;
};

SPPoint* spPointCreate(double* data, int dim, int index){
SPPoint* point = malloc(sizeof(SPPoint*));
if(point == NULL)
{
    return NULL;
}
point->data = (double*) malloc(sizeof(data));
for( int i=0 ; i<dim ; i++)
{
    point->data[i] = data[i];
}
point->dim = dim;
point->index = index;
return point;
}

Wrong size allocated. `SPPoint* point = malloc(sizeof(SPPoint*));` --> `SPPoint* point = malloc(sizeof *point);` Same for `point->data = malloc(sizeof *(point->data));` — chux - Reinstate Monica, Dec 27 '16 at 23:45

score 3 · Accepted Answer · answered Dec 27 '16 at 23:49

3

SPPoint* point = malloc(sizeof(SPPoint*));

should be: struct SPPoint* point = malloc(sizeof(*point));

point->data = (double*) malloc(sizeof(data));

should be point->data = malloc(dim * sizeof(*point->data)); since you want to allocate dim doubles for your point.

answered Dec 27 '16 at 23:49

A.S.H

29,101
5
23
50

Fixed the first one. Regarding the second one, the data array given to the function is of size dim. – Bar Dec 27 '16 at 23:52
Won't let me edit : Fixed the first one. Regarding the second one, the data array given to the function is of size dim. I think point->data = malloc(dim * sizeof(double)); is what you were thinking? Thanks for the answer – Bar Dec 27 '16 at 23:54
@Bar yes that is it. You have to allocate `dim` doubles – A.S.H Dec 27 '16 at 23:55
@Bar notice that even if the givendata array has size `dim`, the `sizeof` function cannot know it inside the function, because it is given simply as a pointer. – A.S.H Dec 27 '16 at 23:57
I see, so what you meant is that sizeof simply gives me the size of a double – Bar Dec 28 '16 at 00:00
Also, it's not a great idea to cast the return of `malloc()`. http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc – Chimera Dec 28 '16 at 00:01
@Bar `sizeof(data)` will give you the size of a pointer (4 or 8 bytes according to platform) and `sizeof(*data)` will give you the size of a double. – A.S.H Dec 28 '16 at 00:02
Regarding the same topic, say I want to change the following code line: ( SPPoint* parr [n]; ) to a malloc 'form' , like above. I think this is the correct way and it the program runs but I can't justify it. Can anyone explain? ( SPPoint** parr = malloc (n * sizeof(SPPoint*)); ) thanks! – Bar Dec 28 '16 at 23:28

chux - Reinstate Monica · Answer 2 · 2016-12-28T00:02:52.603

Code is mis-allocating in 2 places

// Bad
SPPoint* spPointCreate(double* data, int dim, int index){
  SPPoint* point = malloc(sizeof(SPPoint*));  // allocate the size of a pointer
  ...
  point->data = (double*) malloc(sizeof(data)); // allocate the size of a pointer

Instead avoid mis-coding the type and allocate to the the size of the de-referenced variable.

Also need to allocate N objects.

SPPoint* spPointCreate(double* data, int dim, int index){
  size_t N = 1;
  SPPoint* point = malloc(sizeof *point * N);// allocate the size of `*point` * N

  ...
  assert(dim >= 0);
  N = dim;
  point->data = malloc(sizeof *(point->data) * N);

BTW, casting the result of malloc() not needed.

2nd allocation would benefit with a NULL check. More complicated as dim may be 0 and a malloc() return of NULL in that case is OK.

  N = dim;
  point->data = malloc(sizeof *(point->data) * N);
  if (point->data == NULL && N > 0) {
    free(point);
    return NULL;
  }

Code crashes on c, most likely something to do with malloc?

2 Answers2