Memory Leaks in C

Question

So I'm very new to C, and I'm writing a matrix compression function for a trivial Bitmap Image recognition program. I have the following code, and Valgrind in telling me I have memory leaks at the following marked lines, although I have no idea what's causing it. Any advice would be appreciated.

/* Returns a NULL-terminated list of Row structs, each containing a NULL-terminated list of Elem   structs.
 * See sparsify.h for descriptions of the Row/Elem structs.
 * Each Elem corresponds to an entry in dense_matrix whose value is not 255 (white).
 * This function can return NULL if the dense_matrix is entirely white.
 */
Row *dense_to_sparse(unsigned char *dense_matrix, int width, int height) {
    Row *result = NULL;
    _Bool first_row;
    for (int row = height - 1; row >= 0; row--) {
        first_row  = 0;
        for (int elem = width - 1; elem >= 0; elem--) {
            unsigned char curr_item = dense_matrix[(row*width) + elem];
            if (curr_item!= 255) {
                if (!first_row) {
(Memory Leak)       Row *curr_row  = (Row *) malloc(sizeof(Row));
                    if (curr_row == NULL) {
                        allocation_failed();
                    }
                    curr_row->next = result;
                    curr_row->y = row;
                    curr_row->elems = NULL;
                    result = curr_row;
                    //free(curr_row);
                    first_row = 1;
                }
(Memory Leak)   Elem *curr_elem = (Elem *) malloc(sizeof(Elem));
                if (curr_elem == NULL) {
                    allocation_failed();
                }
                curr_elem->value = curr_item;
                curr_elem->x = elem;
                curr_elem->next = result->elems;
                result->elems = curr_elem;
                //free(curr_elem);
            }
        }
    }
    return result;
}

I believe it may be a problem with freeing curr_row and curr_elem, although when I try to free them at the end of each loop, it gives me a runtime error:

parsify(73897,0x7fff75584310) malloc: * error for object 0x7fbf81403a48: incorrect checksum for freed object - object was probably modified after being freed.

[Please don't cast the return value of `malloc()` and friends, in C](http://stackoverflow.com/a/605858/28169). — unwind, Feb 13 '14 at 08:07
Ahhh very enlightening. Thank you! Glad I read this early in my C excursions =D — Brandon, Feb 14 '14 at 02:32

score 7 · Answer 1 · edited Feb 13 '14 at 15:32

7

You need to call free on every pointer that you get from malloc. C doesn't automatically free up memory that you allocate, so you need to tell it "I'm done." Free is how you do this.

EDIT: You should also probably call free at the very end of the function, after you know you're done with the memory. If you do it at the end of the loop, you may run into problems with using memory you already freed.

EDIT EDIT: When you free it, note that you have put result in curr_row->next. You're probably accessing this later, post-free, which is a serious problem. You likely want to free all of them at the same time, as clearly you still need the memory (you still have pointers to it).

edited Feb 13 '14 at 15:32

pat

12,587
1
23
52

answered Feb 13 '14 at 07:44

gregkow

560
2
11

Calling it at the end, right before the return gives me an undeclared identifier error for my 'curr_row' etc. – Brandon Feb 13 '14 at 07:48
Try declaring Row* curr_row before the loop. Currently, it's only in the scope of the for loop. However, your problem with memory is deeper than this, and you really need to iteratively free every single row at the end. See my edits above. – gregkow Feb 13 '14 at 07:49
So I moved my declarations before both of the loops, and freed both curr_row and curr_elem right before my return, and now its just seg-faulting. From my understanding, calling curr_row->next = result followed by result = curr_row just tacks on curr row to the front of the result and moves the pointer back to the beginning (typical algo for creating a linked-list in reverse). Anyways, I dont see how free these curr pointers at the end is causing a segmentation fault now, as the only pointer I'm returning is the result. – Brandon Feb 13 '14 at 08:06
You return the result, but you have to remember that `result->next` is still a field. Specifically, it is a pointer pointing to something you freed. If whatever you return result to dereferences these, you will crash. – gregkow Feb 13 '14 at 08:12
1

You can check your fixes using a memory debugger like Valgrind. This does not replace the hints from the previous comments. – mrks Feb 13 '14 at 08:30
As @mrks say, use Valgrind is the best way to find and fix memory leaks (but it does much more). Take a look here: http://www.cprogramming.com/debugging/valgrind.html – Kyrol Feb 13 '14 at 09:22

score 1 · Accepted Answer · answered Feb 13 '14 at 15:44

You cannot free the memory in dense_to_sparse because the whole point of the function is to create and return the newly allocated data structure. Presumably, the code that calls dense_to_sparse wants to use the result.

You will need a separate function to deallocate the memory that you should call once you no longer need it.

void free_sparse_matrix (Row *matrix)
{
    Row *row = matrix;

    while (row != NULL) {
        Row *next_row = row->next;
        Elem *elem = row->elems;

        while (elem != NULL) {
            Elem *next_elem = elem->next;

            free (elem);
            elem = next_elem;
        }

        free (row);
        row = next_row;
    }
}

This fixed my problem. I had a function for this, but wasn't working quiet as well as I thought apparently. Thank you! — Brandon, Feb 14 '14 at 02:29

Memory Leaks in C

2 Answers2