Understanding Malloc and Realloc in regards to an array of structs

Question

I have been struggling with the ideas behind malloc and realloc for quite some time now and at the moment I have a problem with dynamically creating an array of structs. I have a struct triangle which itself is composed of an array of struct coordinates. I would like to be able to have an array of triangles which is as large as necessary, but every time I attempt to increase the length of my array, nothing seems to happen. Realloc doesn't fail and neither does malloc. However the new triangles are not inserted in my array. Here is my code for reference.

#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
#include <errno.h>
#include <stdio.h>
struct coordinate {
    int x;
    int y;
};

struct triangle {
    struct coordinate point[3];
};
  static size_t size = 0;

static void addTriangle(struct triangle **triangles, struct triangle *t) {
    struct triangle *ts = (struct triangle*) realloc(*triangles, (size+1) * sizeof(struct triangle));
    if(ts == NULL) {
        free(ts);
        exit(EXIT_FAILURE);
    }

    *triangles = ts;
    triangles[size] = t;
    size++;

}

int main() {
    struct triangle* triangles = (struct triangle *) malloc(sizeof(struct triangle));
    if(triangles == NULL) {
        free(triangles);
        exit(EXIT_FAILURE);
    }
    for(int i = 0; i < 2; i++) {
        struct coordinate *a = malloc(sizeof(struct coordinate));
        a->x = 1 * i;
        a->y = 2 * i;
        struct coordinate *b = malloc(sizeof(struct coordinate));
        b->x = 3 * i;
        b->y = 4 * i;
        struct coordinate *c = malloc(sizeof(struct coordinate));
        c->x = 5 * i;
        c->y = 6 * i;
        struct triangle *t = malloc(sizeof(struct triangle));
        t->point[0] = *a;
        t->point[1] = *b;
        t->point[2] = *c;

        addTriangle(triangles, t);
    }

}

I have tried every variation of this I have found, but I would rather not just blindly throw in & and * until something happens.

Can you show the definitions of struct coordinate and struct triangle? — KamilCuk, Jun 21 '18 at 12:08
Sure thing, I added them to my code example. sorry about that — user25758, Jun 21 '18 at 12:10
Try sending the address of trangles to the add_traingles function. ie addTrangles( &trangles, t ); — Neil, Jun 21 '18 at 12:10
`addTriangle(triangles, t);` Your compiler should complain about different level of indirection for first parameter. You expect a `struct triangle **` but you pass a `struct triangle *`. You should always enable warnings in your compiler. Use -Wall -Wextra — Gerhardh, Jun 21 '18 at 12:12
Not related to your question but you createt memory leaks when allocating memory for coordinates. You could simply use a single variable for that. No pointers needed. — Gerhardh, Jun 21 '18 at 12:14
`triangles[size] = t;` should be `(*triangles)[size] = *t;`. — mch, Jun 21 '18 at 12:15
If `triangles` is `NULL` after you first call `malloc`, there's no point calling `free` on it as no memory was allocated — Chris Turner, Jun 21 '18 at 12:18
It would be more fun to call `addTriangle(int x1, int y1, int x2, int y2, int x3, int y3);`, that definition and usage seems ill-conceived and generally not so helpful. — unwind, Jun 21 '18 at 12:20
There is also a mismatch of what is stored in the array. You store the pointer to the new triangle in the array but you allocate memory for `struct triangle` structures. — Gerhardh, Jun 21 '18 at 12:21

Pascal Cuoq · Accepted Answer · 2018-06-21T14:24:27.483

As-is, your program invokes undefined behavior when it passes an uninitialized *triangles to realloc: https://taas.trust-in-soft.com/tsnippet/t/9ff94de4 . You probably meant to pass &triangles when you called it in main.

Changing the call in main to addTriangle(&triangles, t);, the next issue is an out-of-bounds access inside addTriangle: https://taas.trust-in-soft.com/tsnippet/t/658228a1 . Again this may be because you have the wrong level of indirection and meant something like (*triangles)[size] instead of triangles[size].

If I change the line triangles[size] = t; to (*triangles)[size] = *t; then there is no undefined behavior. You should check whether this program still does what you want, since it was modified: https://taas.trust-in-soft.com/tsnippet/t/8915bd2d

The final version:

#include <string.h>
#include <stdlib.h>
struct coordinate {
    int x;
    int y;
};

struct triangle {
    struct coordinate point[3];
};
  static size_t size = 0;

static void addTriangle(struct triangle **triangles, struct triangle *t) {
    struct triangle *ts = (struct triangle*) realloc(*triangles, (size+1) * sizeof(struct triangle));
    if(ts == NULL) {
        free(ts);
        exit(EXIT_FAILURE);
    }

    *triangles = ts;
    (*triangles)[size] = *t; // a struct assignment
    size++;

}

int main() {
    struct triangle* triangles = (struct triangle *) malloc(sizeof(struct triangle));
    if(triangles == NULL) {
        free(triangles);
        exit(EXIT_FAILURE);
    }
    for(int i = 0; i < 2; i++) {
        struct coordinate *a = malloc(sizeof(struct coordinate));
        a->x = 1 * i;
        a->y = 2 * i;
        struct coordinate *b = malloc(sizeof(struct coordinate));
        b->x = 3 * i;
        b->y = 4 * i;
        struct coordinate *c = malloc(sizeof(struct coordinate));
        c->x = 5 * i;
        c->y = 6 * i;
        struct triangle *t = malloc(sizeof(struct triangle));
        t->point[0] = *a;
        t->point[1] = *b;
        t->point[2] = *c;

        addTriangle(&triangles, t); /* pass the address of triangles
           so that addTriangle can modify this variable's contents */
    }

}

Side remarks not directly related to the problem you asked about

As long as you program in C, please do not cast the result of malloc. Simply write struct triangle* triangles = malloc(....
As noted by @aschepler in the comments, this program still leaks the memory blocks allocated from main. These can be freed at the end of each iteration without adding any undefined behavior: https://taas.trust-in-soft.com/tsnippet/t/a0705262 . Doing this, you may realize that t->point[0] = *a;, ... are in fact struct assignments and that it was unnecessary to allocate a separate struct coordinate in the first place: you could just fill in each struct coordinate member of the struct triangle. In addition, it was unnecessary to allocate the struct triangle in main, too: you could just use a local variable for that, since anyway the contents of the struct will be copied by the function addTriangle to the array that main's local variable triangles points to.
Also you don't need to call free(triangles) if triangles is a null pointer in main:
```
struct triangle* triangles = (struct triangle *) malloc(...
if(triangles == NULL) {
    free(triangles);
    exit(EXIT_FAILURE);
}
```
It is allowed to pass a null pointer to free, and this does what you would expect (it does nothing), but since you know that triangles is NULL in the then branch, simply call exit.
Handling the failure of realloc on the other hand is a subtle subject. Your program is doing it wrong, but it does not really matter because it calls exit immediately.
Storing information about the allocated array pointed by main's local variable triangles in a static file-scope variable size is not consistent. The two are so closely related that they should be in the same scope. Since you need addTriangle to be able to change size, you cannot simply move size to be a local variable of main, but you can move the local variable triangles of main to file scope, next to size. If you prefer to make size a local variable of main after all, you will need to pass its address to the function addTriangle so that the latter can update the former.

You should include the modified source in the answer with explanation why you changed it and it should be acceptable. — Goswin von Brederlow, Jun 21 '18 at 12:25
Another possible improvement: it's not necessary to `malloc` the `struct coordinate` objects (which are currently leaked). Just `struct coordinate a = { 1*i, 2*i }; t->point[0] = a;` etc. — aschepler, Jun 21 '18 at 12:32
@aschepler This is a good suggestion but is not strictly related to the problem the OP was facing, so I'm torn about including it. — Pascal Cuoq, Jun 21 '18 at 12:34
This does indeed seem to work. If I may ask, what is happening with the address when you give &triangles in main and then expect **triangles in addTriangle? You're giving the address of the pointer of triangles and in realloc using the pointer of the pointer of triangles? It seems redundant, but thats why I'm here in the first place — user25758, Jun 21 '18 at 12:35
@user25758 What you seem to want to do is to allow `addTriangle` to extend the array pointed to by the variable named `triangles` in `main`. In order to do that, it may be necessary to move the whole array somewhere else, so the function `addTriangle`, if it is intended to work this way, needs to be able to modify `main`'s variable `triangle`. In order to be able to do that, it must receive that variable's address. You had mostly the right levels of indirection everywhere else, so I fixed the program in order to be consistent. — Pascal Cuoq, Jun 21 '18 at 12:45
@aschepler See I started pointing out things that were not the initial question and look at my answer now! :) — Pascal Cuoq, Jun 21 '18 at 12:55

mch · Answer 2 · 2018-06-21T12:34:24.747

You can replace the whole body of your for loop with

struct triangle t = {{{i, 2*i},{3*i,4*i},{5*i,6*i}}};
addTriangle(&triangles, &t);

Please note the & before the parameters, because you want to pass the address of both.

I already noticed as a comment that triangles[size] = t; should be (*triangles)[size] = *t;

#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
#include <errno.h>
#include <stdio.h>
struct coordinate {
    int x;
    int y;
};

struct triangle {
    struct coordinate point[3];
};
  static size_t size = 0;

static void addTriangle(struct triangle **triangles, struct triangle *t) {
    struct triangle *ts = realloc(*triangles, (size+1) * sizeof(struct triangle));
    if(ts == NULL) {
        exit(EXIT_FAILURE);
    }

    *triangles = ts;
    (*triangles)[size] = *t;
    size++;

}

int main() {
    struct triangle* triangles = malloc(sizeof(struct triangle));
    if(triangles == NULL) {
        exit(EXIT_FAILURE);
    }
    for(int i = 0; i < 2; i++) {
        struct triangle t = {{{i, 2*i},{3*i,4*i},{5*i,6*i}}};
        addTriangle(&triangles, &t);
    }
    for(int i = 0; i < size; i++) {
        printf("%d %d, %d %d, %d %d\n", triangles[i].point[0].x,
                                        triangles[i].point[0].y,
                                        triangles[i].point[1].x,
                                        triangles[i].point[1].y,
                                        triangles[i].point[2].x,
                                        triangles[i].point[2].y);
    }
}

score 0 · Answer 3 · answered Jun 21 '18 at 12:25

0

Change the function call as

addTriangle(&triangles, t);

answered Jun 21 '18 at 12:25

Ajith C Narayanan

564
2
21

Understanding Malloc and Realloc in regards to an array of structs

3 Answers3

Side remarks not directly related to the problem you asked about