Getting a very annoying segmentation fault... mallocing 2d array in C.

Question

Hi,

I'm having a little trouble with this. First of all, this is for a simple graphics library that I was writing.

What I'm doing is making it possible to declare multiple "virtual screens", with dimensions of the programmer's choice. For this, I need to dynamically allocate enough memory based on the number of pixels the screen will contain.

The function that allocates this memory seems to work only once in my program, then fails (with a segfault) every other time I call it. I think the issue may be the way I'm passing around the virtual screen (screen_t). The way I implemented it, was that the function new_screen returns not a pointer, but the actual screen itself. I'm thinking maybe that was a mistake. Additionally, when a function, such as set_current_screen needs a screen as one of its arguments, it's passed the address, like this:

set_current_screen(&screen_variable);

Is this the right way to go about that? Is this what's causing the segfault when I call new_screen?

Thanks in advance. The code is a header file (very incomplete), and a c file that draws some rects on one screen, tries to make another, gets a segfault. Here they are:

hgl.h

main.c

Answer 1

The problem lies at the bitmap declaration at the virtual_screen struct.

Try this:

struct pixel bitmap[][];

Then, correct the parts of the code that may be affected by this change.

Answer 2

First of all, your code is not SEGFAULT'ing in the second new_screen() call. You mislead all of us :) Code crashes when you try to access it. Your main problem lies on this line:

 47     vscreen.bitmap[i] = malloc(height * sizeof(int));

Since you have allocated a pointer to an array of struct pixel's, why do you get the size of an int. You should change it to the following:

vscreen.bitmap[i] = malloc(height * sizeof(struct pixel));

Then everything should be OK. But...

..There are many other problems in the code as well. In new_screen you are allocating an array of pointers, then each row separately. Why not allocate the whole screen at once?

First you can define bitmap as, struct pixel *bitmap; And allocate a space for the struct as bitmap = malloc(width * height * sizeof(struct pixel));

Then you can acces a pixel at (x,y) as bitmap[y*WIDTH+x]. You can hide this in a function as well.

Also your convention of width then height notation is quite awkward. In the later loops you are looping the bitmap array in the usual way but this causes many cache misses. Since you are addressing the array in vertical order.

Another remark is on the use of loops. You know, there is loop for that: for. It makes more sense to use for in your cases in the code.

Answer 3

So you have:

typedef struct pixel{
    unsigned char b;
    unsigned char g;
    unsigned char r;
    float a;
};

struct virtual_screen{
    int layer;
    int width;
    int height;
    int opacity;
    struct pixel **bitmap;
};

typedef struct virtual_screen screen_t;

But when you go to allocate the bitmap member of the screen_t, you do:

vscreen.bitmap=malloc(width * sizeof(unsigned int *));

Where did unsigned int* come from? bitmap is of type struct pixel**.

To avoid these kinds of mistakes, it's better to do:

vscreen.bitmap=malloc(width * sizeof *vscreen.bitmap);

And similarly, when you allocate the elements of vscreen.bitmap, you should do:

vscreen.bitmap[i] = malloc(height * sizeof *vscreen.bitmap[i]);

(Also, you should check that malloc succeeded!)

As a side note, I'll say that your allocation scheme is a bit unusual: you're allocating by columns, but typically images are stored by rows. I realize you're doing this so you can use (x, y) coordinates for the array subscripts, but IMO it's better not to fight the matrix (row, column) convention.