C usage of malloc on struct

Question

I'm trying to use malloc on a struct called image. The function is:

void image_init(struct image* img, int w, int h) {
    img = malloc(sizeof(struct image));
    (*img).w = w;
    (*img).h = h;
}

And the function to free the image again is:

void image_destroy(struct image* img) {
    free(img);
}

But whenever I try to free the malloc-ed data I'm getting the error that the adress I'm trying to free was not malloc-ed before.

The struct of image is:

struct image {
    int w,h;
    int* data;
}

I call the functions with:

struct image img;
image_init(&img,100,100);
image_destroy(&img);

Let's save keystrokes with one-letter struct member names and then waste two characters on `(*img).w` instead of `img->w`! Brilliant. — Kaz, May 18 '16 at 20:16

Giorgi Moniava · Accepted Answer · 2016-05-19T09:08:55.267

5

First here

void image_init(struct image* img, int w, int h) {
    img = malloc(sizeof(struct image));
    (*img).w = w;
    (*img).h = h;
}

img is a copy of the original pointer passed to it, hence the code on first line inside function has no effect, since it makes the copy point somewhere - not the original object.

This:

struct image img;
image_init(&img,100,100);
image_destroy(&img);

doesn't make sense either (assuming you expected img to point somewhere after call to init). img isn't a pointer, how you expect it to point somewhere?

One way you could solve this could be

struct image *img = image_init(100,100);

where

struct image* image_init(int w, int h) {
    img = malloc(sizeof(struct image));
    (*img).data = NULL;
    (*img).w = w;
    (*img).h = h;
    return img;
}

Don't forget to call free on the pointer returned by above function - you will need to free data pointer separately too, in case you allocate it too.

NOTE: My best guess (if you also can't change prototype) is you want something like this:

void image_init(struct image* img, int w, int h) {
    img->data = malloc(sizeof(int) * w * h);
    img->w = w;
    img->h = h;
}

destroy:

void image_destroy(struct image* img) {
    free(img->data);
    free(img);
}

In main

struct image* img = malloc(sizeof(struct image));
image_init(img, 100, 100);
image_destroy(img);

PS. Or if you want the usage of those functions remain as you had in main go with the answer of Johnny Mopp.

edited May 19 '16 at 09:08

answered May 18 '16 at 19:51

Giorgi Moniava

27,046
9
53
90

Thanks. But the calls of image_init and the prototype of it were given to us to implement so I thought we can't change it. Are there solutions that don't change it? – Maschs May 18 '16 at 19:56
Or alternatively you can keep the structure similar to the original, but pass `struct image **img` as a parameter and assign `*img` with the `malloc`ed pointer. – Eugene Sh. May 18 '16 at 19:56
@user3745172 In this case you don't need `malloc`. The function should be provided with an already allocated structure pointer. As you do in the question. Just remove the `malloc` line and you are done. – Eugene Sh. May 18 '16 at 19:57
@user3745172 In that case you might need to use pointer to pointer. Refer to google please, check how to initialize a struct using pointer to pointer. – Giorgi Moniava May 18 '16 at 19:57
@user3745172 see update, if you can't change prototype – Giorgi Moniava May 18 '16 at 19:58
@Giorgi No need for `malloc` in main. It can be statically allocated as in the question... – Eugene Sh. May 18 '16 at 20:01
@EugeneSh. No in that case he won't be able to use destroy function – Giorgi Moniava May 18 '16 at 20:04
Maybe `image_init` is supposed to `malloc` the `data` pointer. – 001 May 18 '16 at 20:05
Well, yeah. The interface lacks consistency. – Eugene Sh. May 18 '16 at 20:07
Don't forget that it is a good idea to ensure that all elements of the structure are properly initialized. If you don't set the `data` element to NULL, you won't know whether the memory has been allocated for it or not — in general. Your revised version does this by assigning to `data`, of course. – Jonathan Leffler May 19 '16 at 00:24
@JonathanLeffler I made a change if I correctly understood your suggestion, initially I missed there was data pointer inside the structure in the first place :) – Giorgi Moniava May 19 '16 at 09:09

score 3 · Answer 2 · answered May 18 '16 at 20:15

3

I think your assignment is not to allocate the img but the data ptr:

void image_init(struct image* img, int w, int h) {
    img->data = malloc(sizeof(int) * w * h);// Check for failure...
    img->w = w;
    img->h = h;
}
void image_destroy(struct image* img) {
    free(img->data);
}

answered May 18 '16 at 20:15

001

13,291
5
35
66

This makes a perfect sense. – Eugene Sh. May 18 '16 at 20:19

score 0 · Answer 3 · answered May 18 '16 at 20:12

struct image img;
image_init(&img,100,100);
image_destroy(&img);

allocates an image struct on your stack. Why are you trying to malloc one? If you really want one on the heap then do

    struct image * image_init( int w, int h) {
        struct image *img = malloc(sizeof(struct image));
        img->w = w;
        img->h = h;
        return img;
    }
...  
    struct image *img = image_init(100,100);

Note the more idiomatic '->' use

if you want the one on the stack then do

void image_init(struct image* img, int w, int h) {
    img->w = w;
    img->h = h;
}

score -4 · Answer 4 · answered May 18 '16 at 19:53

-4

You need to cast the malloc to struct because the return value of malloc is of type void.

answered May 18 '16 at 19:53

trench224

1
1

Beware of applying rules enforced by C++ compilers to C code. They're different languages, and this is one place where there's a crucial difference. – Jonathan Leffler May 19 '16 at 00:26

C usage of malloc on struct

4 Answers4