views:

4915

answers:

7

Hello!

I'm having some issues with producing an int matrix without creating memory leaks. I want to be able to make a given (global) matrix into any size dynamically via read_matrix(). But then i want to be able to free the memory later on. So in my main method the second printf should result in a bus error since it should not have any memory allocated to it. How would i go about creating this?

int**    first_matrix;
int**    second_matrix;
int**    result_matrix;

int** read_matrix(int size_x, int size_y)
{
    int** matrix;
    matrix = calloc(size_x, sizeof(int*));
    for(int i = 0;i<size_x;i++) {
     matrix[i] = calloc(size_y, sizeof(int));
    }
    for(int i = 0;i<size_x;i++) {
     for(int j = 0;j<size_y;j++) {
      matrix[i][j] = i*10+j;
     }
    }
    return matrix;
}

int main(int stackc, char** stack)
{
    first_matrix = read_matrix(10,10);
    printf("9:3 %d - 4:6 %d \n", first_matrix[9][3], first_matrix[4][6]);
    free(*first_matrix);
    free(first_matrix);
    printf("9:3 %d - 4:6 %d \n", first_matrix[9][3], first_matrix[4][6]);
}
+2  A: 

You need to free each row individually:


void free_matrix(int **matrix, int size_x)
{
    for(int i = 0; i < size_x; i++)
        free(matrix[i]);
    free(matrix);
}
Adam Rosenfield
You can't declare a variable in the for cycle in C.
terminus
You can in C99, but your code won't be back-portable.
Steve Jessop
A: 

You only freed the first row (or column) of first_matrix. Write another function like this:

void free_matrix(int **matrix, int rows)
{
    int i;
    for(i=0; i<rows; i++)
    {
        free(matrix[i]);
    }
    free(matrix);
}

You might want to make the matrix into a struct to store it's row and column count.

terminus
+1  A: 

Freeing the memory doesn't make it go away, it just means that another allocation might grab that same chunk of memory. Whatever you put in it will still be there until something else overwrites it.

Also, you're not freeing everything you allocated. You're only freeing the array of pointers and the first row. But even if you free everything correctly, you would still have the same effect.

If you want to create a "bus error" you need to point to memory that doesn't belong to your process. Why do you want to do that anyway?

Ferruccio
i just wanted to do that to show that the memory was free, but learned now that the fault was in my thinking there, can't run valgrind since i'm onOS X and wanted a way to see if i really had freed it
ique
A: 

I recommend using valgrind to track down unfree'd memory, as opposed to trying to make a bus error occur. It rocks for lots of other stuff as well.

Sam

Sam Reynolds
i'm on OS X so i can't run valgrind, otherwise i would :/you know any alternative that works on mac?
ique
A: 

You're getting memory leaks because you're freeing the first row of the matrix and the list of rows, but none of the 1 to nth rows. You need to call free in a loop.

There are a couple of alternatives, however: - Allocate sizeof(int*)rows + rowscols*sizeof(int) bytes and use the first bytes for the row pointers. That way, you only have a single chunk of memory to free (and it's easier on the allocator, too) - Use a struct that contains the number of rows. Then you can avoid the row list altogether (saving memory). The only downside is that you have to use a function, a macro, or some messy notation to address the matrix.

If you go with the second option, you can use a struct like this in any C99 compiler, and again only have to allocate a single block of memory (of size numints*sizeof(int)+sizeof(int)):

struct matrix {
    int rows;
    int data[0];
}
Nick Johnson
+4  A: 

Just because the memory has been free'd doesn't mean you can't access it! Of course, it's a very bad idea to access it after it's been free'd, but that's why it works in your example.

Note that free( *first_matrix ) only free's first_matrix[0], not the other arrays. You probably want some kind of marker to signify the last array (unless you will always know when you free the outer array how many inner arrays you allocated). Something like:

int** read_matrix(int size_x, int size_y)
{
    int** matrix;
    matrix = calloc(size_x, 1+sizeof(int*)); // alloc one extra ptr
    for(int i = 0;i<size_x;i++) {
        matrix[i] = calloc(size_y, sizeof(int));
    }
    matrix[size_x] = NULL; // set the extra ptr to NULL
    for(int i = 0;i<size_x;i++) {
        for(int j = 0;j<size_y;j++) {
            matrix[i][j] = i*10+j;
        }
    }
    return matrix;
}

Then when you're freeing them:

// keep looping until you find the NULL one
for( int i=0; first_matrix[i] != NULL; i++ ) {
    free( first_matrix[i] );
}
free( first_matrix );
Graeme Perrow
it seems that looping until matrix[i] != NULL works even if i don't set the last pointer to NULL, is this coincidence/something other bad?
ique
If you allocate one extra pointer and don't set it, it *might* default to NULL, depending on the OS (Windows zeroes out allocated memory, Unix doesn't). If it's not, you will run off the end of the allocated array and keep freeing things until you happen to hit a NULL pointer or (more likely) crash.
Graeme Perrow
calloc always sets the memory to null.
quinmars
A: 

The concept you are missing here, is that for every calloc, there must be a free. and that free must be applied to the pointer passed back from calloc.

I recommend you create a function (named delete_matrix) that uses a loop to free all of the pointers that you allocate in here

for(int i = 0;i < size_x;i++) { matrix[i] = calloc(size_y, sizeof(int)); }

then, once that is done, free the pointer allocated by this.

matrix = calloc(size_x, sizeof(int*));

The way you are doing it now,

free(*first_matrix); free(first_matrix);

won't do what you want it to do.

EvilTeach