CODEWITHSUNDEEP
cmallocintshort
Alex
Alex

Reputation: 758

strange malloc behavior in C

I am trying to create a matrix with dynamic proportions and to initialize it here is the code I am using to allocate memory and initialization:

int **matrix;
//mem allocation
matrix=(int*)malloc(sizeof(int*)*mat_w);
for (i=0;i<mat_w;i++)
    matrix[i]=(int)malloc(sizeof(int)*mat_h);
//init
for (i=0;i<mat_w;i++)
    for (j=0;j<mat_h;j++)
        matrix[i][j]=0;

This , works fine , question is , if I try to create a matrix of type short - I get a segmentation error on the init first pass.

Is this a C language issue or am I doing something wrong?

Code for matrix of type short:

short **matrix;
//mem allocation
matrix=(short*)malloc(sizeof(short*)*mat_w);
for (i=0;i<mat_w;i++)
    matrix[i]=(short)malloc(sizeof(short)*mat_h);
//init
for (i=0;i<mat_w;i++)
    for (j=0;j<mat_h;j++)
        matrix[i][j]=0;

P.S.: I dropped safety checks , index variables and boundary declarations for clarity of code.

Thanks,
Alex

Upvotes: 2

Views: 3673

Answers (6)

AnT stands with Russia
AnT stands with Russia

Reputation: 320777

There's a serious lesson your have to learn from this mistake. And it says the following: never cast the result of 'malloc'.

Moreover, this is a part of a bigger good-practice gudeline that is best followed whenever possible: never mention type names in your code, except in declarations.

This is how your code should have looked from the very beginning

  int **matrix;

  matrix = malloc(mat_w * sizeof *matrix);
  for (i = 0; i < mat_w; i++)
    matrix[i] = malloc(mat_h * sizeof *matrix[i]);

  for (i = 0; i < mat_w; i++)
    for (j = 0; j < mat_h; j++)
      matrix[i][j] = 0;

Note, that in order to switch from 'int' to 'short' in this version you just need to change the declaration of 'matrix' and nothing else.

(Of course, there's more that can be improved in this code, but I just wanted to address the immediate reason for the error.)

Upvotes: 6

Patrice Bernassola
Patrice Bernassola

Reputation: 14446

You are casting your int** to int* the return value of malloc (same for short). malloc should be use as this:

matrix = (int**)malloc(sizeof(int*) * mat_w);

or

matrix = (short**)malloc(sizeof(short*) * mat_w);

Same for each allocation inside matrix:

matrix[i] = (int*)malloc(sizeof(int) * mat_h);

or

matrix[i] = (short*)malloc(sizeof(short) * mat_h);

Upvotes: 5

Alex B
Alex B

Reputation: 84972

Your casts for the return value of malloc() are invalid. They should be int** and int* in the first case, and short** and short* in the second.

When you cast the return value of malloc() to short, a pointer returned gets truncated to fit in short value, and then gets assigned to a short* pointer, yielding a pointer value pointing to an invalid memory location. Therefore you get a segmentation fault trying to access it.

With int, you are getting lucky, since on your platform most probably sizeof(int)==sizeof(int*), so that a pointer returned by malloc() casted to int is not truncated and it all silently works. It would most probably crash in a similar fashion on a 64-bit platform.

Should be:

short **matrix;
matrix=(short**)malloc(sizeof(short*)*mat_w);
for (i=0;i<mat_w;i++)
    matrix[i]=(short*)malloc(sizeof(short)*mat_h); 
for (i=0;i<mat_w;i++)
    for (j=0;j<mat_h;j++)
        matrix[i][j]=0;

If your code is pure C (not C++), you can omit the casts, as in C casting from void* to any other pointer type is valid.

short **matrix;
matrix = malloc(sizeof(short*)*mat_w);
for (i=0;i<mat_w;i++)
    matrix[i] = malloc(sizeof(short)*mat_h); 
for (i=0;i<mat_w;i++)
    for (j=0;j<mat_h;j++)
        matrix[i][j]=0;

Upvotes: 17

Andrejs Cainikovs
Andrejs Cainikovs

Reputation: 28474

sizeof(int) equals to bus width of the particular system. You are trying to put 32bit(or 64 depending on your platform) address value to a 16bit allocated memory.

Check out second example in the Checkers post. This is the correct and preferable way of memory allocation.

Upvotes: 0

user181548
user181548

Reputation:

What compiler are you using that it doesn't scream at you about all these obvious errors?

gcc -Wall produced five warning messages with this code.

#include <stdlib.h>

int main ()
{
    int mat_w = 99;
    int mat_h = 666;
    int i;
    int j;

    int **imatrix;
    short **smatrix;
    //mem allocation
    imatrix=(int*)malloc(sizeof(int*)*mat_w);
    for (i=0;i<mat_w;i++)
    imatrix[i]=(int)malloc(sizeof(int)*mat_h);
    //init
    for (i=0;i<mat_w;i++)
    for (j=0;j<mat_h;j++)
        imatrix[i][j]=0;

    //mem allocation
    smatrix=(short*)malloc(sizeof(short*)*mat_w);
    for (i=0;i<mat_w;i++)
    smatrix[i]=(short)malloc(sizeof(short)*mat_h);
    //init
    for (i=0;i<mat_w;i++)
    for (j=0;j<mat_h;j++)
        smatrix[i][j]=0;
    return 0;
}

Gives me

malloc.c: In function 'main':
malloc.c:13: warning: assignment from incompatible pointer type
malloc.c:15: warning: assignment makes pointer from integer without a cast
malloc.c:22: warning: assignment from incompatible pointer type
malloc.c:24: warning: cast from pointer to integer of different size
malloc.c:24: warning: assignment makes pointer from integer without a cast

Upvotes: 15

Avi
Avi

Reputation: 20152

Yes, you are doing something wrong.

 int *matrix;

means that matrix is an array of integers. If you want it to be an array of arrays of integers, you should declare it like this:

 int **matrix;
 //mem allocation
 matrix=(int**)malloc(sizeof(int*)*mat_w);
 for (i=0; i<mat_w; i++)
     matrix[i]=(int*)malloc(sizeof(int)*mat_h);
 //init
 for (i=0; i<mat_w; i++)
     for (j=0; j<mat_h; j++)
         matrix[i][j]=0;

Of course, if you know in advance the dimensions of the matrix, just do it like this:

int matrix[mat_w][mat_h];
 //init
 for (i=0; i<mat_w; i++)
     for (j=0; j<mat_h; j++)
         matrix[i][j]=0;

Upvotes: 2

Related Questions