Getting memory addresses from array for only some values in C

Question

I hate to dump code like I'm about to, but this has really baffled me. I am making a program to read PPM image files, which are files with a format, a comment, a width and height, and a maximum colour value.

Then after that there is a sequence of RGB values seperated by spaces, I have created a struct to hold three ints to represent an RGB colour, and a struct to hold all of the info about the PPM file and the array of pixels, which is an array of colour pointers.

typedef struct colour {
    int r,g,b;
} colour;

typedef struct ppm {
    char * format;
    int width, height, max;
    colour * pixels[];
} ppm;

And an initialiser to create a new colour triple:

colour * new_colour(int r, int g, int b) {
    colour * c = (colour *)malloc(sizeof(colour));
    c->r = r;
    c->g = g;
    c->b = b;
    return c;
}

I then have a method that reads a PPM file to create a ppm struct:

ppm * get_ppm(FILE * ppm_file){
    ppm * my_ppm = malloc(sizeof(ppm));
    char format[20];
    char comment[100];
    int n, m, max;

    fgets(format, 10, ppm_file);
    fgets(comment, 100, ppm_file);

    char dimension_line[100];
    fgets(dimension_line, 100, ppm_file);
    sscanf(dimension_line,"%d %d", &n, &m);

    char max_line[20];
    fgets(max_line, 20, ppm_file);
    sscanf(max_line, "%d", &max);

    int i;  
    for (i = 0; i<=(m*n - 1); i++) {
        my_ppm->pixels[i] = malloc(sizeof(colour));
        int r, g, b;
        fscanf(ppm_file,"%d %d %d ",&r,&g,&b);
        my_ppm->pixels[i] = new_colour(r,g,b);
    }

    my_ppm->format = format;
    my_ppm->width = n;
    my_ppm->height = m;
    my_ppm->max = max;
    return my_ppm;
}

And then I have a method to print my ppm file:

void show_ppm(ppm * image) {
    printf("Format: %s\nW:%d H:%d\nMAX:%d\n\n", image->format, 
                                                image->width, image-> height,
                                                image->max);
    int i;
    for (i = 0; i<=(image->height)*(image->width) - 1; i++) { 
        int r = image->pixels[i]->r;
        int g = image->pixels[i]->g;
        int b = image->pixels[i]->b;
        printf("p[%d]: (%d,%d,%d)\n",i,r,g,b);
    }
}

The image I'm using is this:

P3
# feep.ppma
4 4
15
 0  0  0  0  0  0    0  0  0   15  0 15
 0  0  0  0 15  7    0  0  0    0  0  0
 0  0  0  0  0  0    0 15  7    0  0  0
15  0 15  0  0  0    0  0  0    0  0  0

It reads in perfectly fine, I've done some printf debugging and it's always the right values, even when I save the values in the struct it's fine, but when I run show_ppm, I get what I assume are memory addresses for the first two values?

p[0]: (26928080,0,26928144)
p[1]: (26928592,0,26928656)
p[2]: (0,0,0)
p[3]: (15,0,15)
p[4]: (0,0,0)
p[5]: (0,15,7)
p[6]: (0,0,0)
p[7]: (0,0,0)
p[8]: (0,0,0)
p[9]: (0,0,0)
p[10]: (0,15,7)
p[11]: (0,0,0)
p[12]: (15,0,15)
p[13]: (0,0,0)
p[14]: (0,0,0)
p[15]: (0,0,0)

However everything else is fine? I've got no clue what I'm doing wrong here, pls help me

If you want to look at the entire file here it is: http://pastebin.com/1FrMBYAS

Thanks in advance, Ciaran

[Please see this discussion on why not to cast the return value of `malloc()` and family in `C`.](http://stackoverflow.com/q/605845/2173917). — Sourav Ghosh, Feb 22 '17 at 16:35
Instead of `char format[20]; ... my_ppm->format = format;` , need to allocate space for `my_ppm->format` and then `memcpy()` e.g. `strdup()` — chux - Reinstate Monica, Feb 22 '17 at 16:38
`my_ppm->pixels[i] = malloc(sizeof(colour));` allocates memory for one pixel, but three lines later you overwrite that with another allocation by `new_colour()` — Weather Vane, Feb 22 '17 at 16:42
Aside: code that does `i <= n-1` is harder to read than the simpler `i < n`. — Weather Vane, Feb 22 '17 at 16:44
You aren't allocating any space for `pixels`. When you `malloc` `my_ppm`, you need to add the size of the array of pixels to `sizeof(ppm)`. The declaration `colour * pixels[]` doesn't actually reserve any space for that array. — Andy Schweig, Feb 22 '17 at 16:48
Surely when I let my_ppm->pixels[i] = new_colour(r,g,b), it allocates the memory, because in new_colour, it allocates memory? — user2930356, Feb 22 '17 at 16:52
The code as shown *completely* lacks *any* error checking. You want to amend this. — alk, Feb 22 '17 at 16:52
@user2930356: Yes, `new_colour` allocates memory for the pixel, but you haven't allocated memory for the array of `colour` pointers within `ppm`. You need to take into account the memory required for that array when allocating memory for `my_ppm`. If, for example, you have 100 pixels, you should do this: `ppm *my_ppm = malloc(sizeof(ppm) + 100*sizeof(colour *));` — Andy Schweig, Feb 22 '17 at 17:50
@user2930356: are you sure you really want an array of pointers to colours rather than an array of colours? Yes, you'd have to revise the `new_colour()` function, but you'd be more space-efficient if you stored an array of colours. You should probably hold off allocating the space for the structure until you know how much space is going to be needed. — Jonathan Leffler, Feb 22 '17 at 18:07
the posted code could save a LOT of aggravation and lines of code by directly allocating room for and array of pixels, the `scanf()` directly into that array with the pixel values. — user3629249, Feb 23 '17 at 21:57

score 1 · Answer 1 · answered Feb 22 '17 at 18:28

1

The declaration colour * pixels[] does not actually reserve any space for that array. When you put something like this at the end of a structure, you need to allocate space for the array beyond the size of the structure itself to hold the elements of the array. For example, if you have 100 pixels, your allocation should look like this:

ppm *my_ppm = malloc(sizeof(ppm) + 100*sizeof(colour *));

If you don't know the number of pixels beforehand, you can allocate space for an initial amount and then realloc if you need more space.

answered Feb 22 '17 at 18:28

Andy Schweig

6,597
2
16
22

1

Or simply defer allocation of the structure until the dimension line has been read, so that you know how much space to allocate. – Jonathan Leffler Feb 22 '17 at 19:24
Given this allocation method, you should probably also show how to initialize `ppm->pixels`, after... – Bob__ Feb 22 '17 at 19:39
@Bob__: you don't need to initialize `pixels`. You just need to know how many pixels there are. – Andy Schweig Feb 22 '17 at 21:15
1

Well, now you have allocated enough memory to store (contiguosly) a `struct ppm` and 100 _pointers_ to `struct colour`. You still have to assign a value to the `ppm->pixels` pointer and above all allocate memory for all those `struct colour`s. I think that storing an array of colours, as Jonathan Leffer suggested, may be better. – Bob__ Feb 22 '17 at 22:07
1

The way `pixels` is declared (as `colour *pixels[]`), you can use it directly after doing the `malloc`. No separate operation necessary to set `pixels` to something. I agree that having it be an array of `colour` makes more sense than having it be an array of pointers. – Andy Schweig Feb 22 '17 at 22:22

Getting memory addresses from array for only some values in C

1 Answers1