memory allocation in structures

Question

My code uses two structures, block and layout (which is a collection of an arbitrary number of blocks).

struct block{
    char type;
    unsigned short int loc;
    unsigned short int size[2];
};
struct layout{
    unsigned short int no;
    struct block *blocks;
    short int **moves;
};

I am using this function to quickly initialize (and partly fill) the structure layout, based a set of blocks:

struct layout init_layout(int block_no, struct block *blocks){
    struct layout new_layout;
    int i, j;

    new_layout.no = (unsigned short int)block_no;
    // the following two lines cause an memory corruption error
    new_layout.blocks = (struct block *)malloc(block_no);
    new_layout.moves = (short int **)malloc(block_no);
    for(i = 0; i < block_no; i++){
        new_layout.blocks[i] = blocks[i];
        new_layout.moves[i] = (short int *)malloc(2);
        for(j = 0; j < 2; j++)
            new_layout.moves[i][j] = 0;
    }

    return new_layout;
}

So far, I do not see, that there is something wrong with it. However, when I call function like this

int main(int argc, char** argv){
    // just some arbitrary values for 10 blocks
    int size[2] = {2, 2};
    struct block *blocks = (struct block *)malloc(10);
    for(length = 0; length < 10; length++){
        blocks[length] = init_block('R', 1, size);
    }

    struct layout puzzle;
    puzzle = init_layout(10, blocks);
    return 0;
}

I end up with an memory corruption error, as marked by the comment in init_layout(). What do I miss in my implementation?

If your'r up to using a 2D array for `moves`, better use one, not a pointer to pointer. — too honest for this site, Mar 02 '16 at 17:50

score 1 · Accepted Answer · edited May 23 '17 at 12:07

When you are allocating memory for anything, you need to analyze, closely -- "What is it that I'm allocating memory for?"

Below, you incorrectly assume a cast of an arbitrary number block_no will adequately size the memory needed for both new_layout.blocks and new_layout.moves -- it won't:

new_layout.blocks = (struct block *)malloc(block_no);
new_layout.moves = (short int **)malloc(block_no);

What you are allocating for new_layout.blocks is actually space for struct block *blocks; (a pointer-to-struct-block), while you can malloc (block_no * sizeof (struct block)); to allocate space for block_no struct block, it is far better to allocate based upon what you are creating (i.e. space for an array new_layout.blocks (again a pointer-to-struct-block) which needs block_no * sizeof *new_layout.blocks bytes of memory to hold block_no of type struct block, e.g.:

new_layout.blocks = malloc(sizeof *new_layout.blocks * block_no);
new_layout.moves = malloc(sizeof *new_layout.moves * block_no);

(simply dereferencing the object you are allocating an array of, will accurate allow you to use sizeof to get the object (element) size for the array. (e.g. sizeof *new_layout.blocks) which you multiply by how many you need (e.g. sizeof *new_layout.blocks * block_no)

The same applies to:

    new_layout.moves[i] = malloc(**new_layout.moves * 2);

(note: here you are allocating for 2 shorts, so you will need to dereference you pointer-to-pointer-to-short twice to be allocating for sizeof (short))

See Also: Do I cast the result of malloc? for thorough explanation.

also the same problem in main: malloc(10 * sizeof(struct block)) — lowtech, Mar 02 '16 at 17:49
You should elaborate a bit more. Just providing some code will result in a question about a similar problem sooner or later. — too honest for this site, Mar 02 '16 at 17:49
Thanks for your help, it seems I didn't properly understand the dynamic memory handling. — van, Mar 02 '16 at 18:47
Sure, no problem, everybody has to learn it once. It was bewildering to me at one time also. — David C. Rankin, Mar 02 '16 at 19:44

alk · Answer 2 · 2016-03-04T06:20:00.933

For starters, this

  new_layout.blocks = (struct block *)malloc(block_no);

should be

  new_layout.blocks = malloc(block_no * sizeof *new_layout.blocks);

For the moves this is a bit more complicated.

Assuming short int **moves; should reference a certain number of int[2] the declaration is not optimal and better should be:

   short int (*moves)[2]; /* Define a pointer to 
                              an array with two elements of type short int. */

And allocation then should look like this:

  new_layout.moves = malloc(block_no * sizeof *new_layout.moves);

Finally initialisation goes like this:

  for(i = 0; i < block_no; i++){
    new_layout.blocks[i] = blocks[i];
    for(j = 0; j < sizeof new_layout.moves[0]/sizeof new_layout.moves[0][0]; j++)
        new_layout.moves[i][j] = 0;
  }

You might have noticed:

No memory allocation in the loop any more.
The magic number 2 only appears once.

:-)

memory allocation in structures

2 Answers2