0

Im writing a simple array. And how i think, do it correct in terms of leaks. But valgrind doesnt think so. Why? That phantom leak just because i do more malloc then free?

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define CAPACITY_INCREACE(arr) ((int)(arr->capacity * 1.5f))
#define CAPACITY_DEFAULT 5

typedef struct array
{
    int capacity;
    int size;
    int* arr;
} array;

array* create_array()
{
    array* arr = (array*)malloc(sizeof(array));
    arr->capacity = CAPACITY_DEFAULT;
    arr->size = 0;
    arr->arr = (int*)malloc(arr->capacity * sizeof(int));
    return arr;
}

void print_array(array *arr)
{
    printf("array, size - %d, capacity - %d\n", arr->size, arr->capacity);
    for(int i = 0; i < arr->size; ++i)
        printf("%d ", arr->arr[i]);
    printf("\n");
}

void realloc_array(array* arr, int size)
{
    int* new_arr = (int*)malloc(size * sizeof(int));
    memcpy((void*)new_arr, arr->arr, arr->size * sizeof(int));
    arr->arr = new_arr;
}

void add(array* arr, int element)
{
    if((arr->size + 1) <= arr->capacity)
    {
        arr->arr[arr->size++] = element;
        return;
    }
    
    arr->capacity = CAPACITY_INCREACE(arr);
    realloc_array(arr, arr->capacity);
    arr->arr[arr->size++] = element;
}

void delete_array(array *arr)
{
    free(arr->arr);
    free(arr);
}

int main(int argc, int** argv)
{
    array* arr = create_array();
    for(int i = 0; i < 8; i++)
        add(arr, i);
    print_array(arr);

    delete_array(arr);
    return 0;
}

Valgrind run and program compile

gcc -g array.c && ./a.out
valgrind --leak-check=full -s ./a.out

Info from valgrind, he show me where, but unfortunately im not understand what im doing wrong:

HEAP SUMMARY:
    in use at exit: 48 bytes in 2 blocks
    total heap usage: 5 allocs, 3 frees, 1,128 bytes allocated

 20 bytes in 1 blocks are definitely lost in loss record 1 of 2
    by 0x10920B: create_array (array.c:21)
    by 0x109443: main (array.c:71)
 
 28 bytes in 1 blocks are definitely lost in loss record 2 of 2
    by 0x10930C: realloc_array (array.c:43)
    by 0x1093CD: add (array.c:57)
    by 0x10946D: main (array.c:74)
 
 LEAK SUMMARY:
    definitely lost: 48 bytes in 2 blocks
    indirectly lost: 0 bytes in 0 blocks
      possibly lost: 0 bytes in 0 blocks
    still reachable: 0 bytes in 0 blocks
         suppressed: 0 bytes in 0 blocks
lbsmart
  • 83
  • 6
  • 2
    _I do more `malloc` than `free`_ .. there's your answer. If you've `malloc`ed memory you haven't `free`ed by the end, valgrind will rightfully complain. – yano Mar 02 '23 at 23:32
  • 1
    It may not be a _practical_ problem.. the OS will clean up all memory your process allocates when it exits, so if you do a one-time `malloc` that will last all process, no real problems if you don't `free` it. In fact I've seen high rep SO users argue for _not_ cleaning up memory, since it's just more code to write, test, debug, etc. But expect valgrind to complain in those cases. – yano Mar 02 '23 at 23:38
  • 1
    `if((arr->size + 1) <= arr->capacity)` ==> `if( arr->size < arr->capacity )`... Much simpler. – Fe2O3 Mar 02 '23 at 23:46
  • @yano there are two dangers if you allow leaks. The first is that your application may run out of memory. The second is that if you have a huge number of leaks then it's a lot harder to detect new leaks than if you maintain a zero leak policy. – Paul Floyd Mar 03 '23 at 07:36
  • @yano also don't put too much faith in a high SO rep. It often is just a snowball effect of being an early contributor. – Paul Floyd Mar 03 '23 at 07:42
  • @PaulFloyd it depends. If your application is looping, constantly allocating, never `free`ing, yup that's trouble. That's not the situation I described. I'm not saying I'm a huge advocate, just another option to consider that might fit a particular situation well. If you've got some one-shot, intricate `malloc`ing scheme, just let the OS clean it all up at the end, who cares? In fact I do have personal experience where I wrote buggy clean up code (was double `free`ing I think), took a while to track that down. More code means more bugs. – yano Mar 03 '23 at 15:07
  • @yano sounds like an advert for rust or at least C++. – Paul Floyd Mar 03 '23 at 16:31
  • @PaulFloyd [lively discussion](https://stackoverflow.com/questions/654754/what-really-happens-when-you-dont-free-after-malloc-before-program-termination) – yano Mar 03 '23 at 17:13
  • @PaulFloyd linked in the SO question (buried in a comment), but interesting to see that Firefox was at least strongly considering [skipping all GC](https://bugzilla.mozilla.org/show_bug.cgi?id=662444) during shutdown and simply terminating. All I'm saying is it's just another tool in the toolbox that might work best in a particular situation ... not always wrong, maybe sometimes right (just like `goto`). Point taken on added difficulty in filtering thru the real leaks vs the ones you want to ignore. – yano Mar 03 '23 at 17:26

1 Answers1

2

The function

void realloc_array(array* arr, int size)
{
    int* new_arr = (int*)malloc(size * sizeof(int));
    memcpy((void*)new_arr, arr->arr, arr->size * sizeof(int));
    arr->arr = new_arr;
}

is throwing away the original buffer arr->arr without freeing.

Add call to free() to avoid memory leak.

void realloc_array(array* arr, int size)
{
    int* new_arr = (int*)malloc(size * sizeof(int));
    memcpy((void*)new_arr, arr->arr, arr->size * sizeof(int));
    free(arr->arr); /* add this */
    arr->arr = new_arr;
}

Or use standard realloc().

void realloc_array(array* arr, int size)
{
    int* new_arr = realloc(arr->arr, size * sizeof(int));
    if (new_arr == NULL) exit(1);
    arr->arr = new_arr;
}

Also note that casting results of malloc() family is considered as a bad practice.

MikeCAT
  • 73,922
  • 11
  • 45
  • 70
  • You have to cast in C++. The question is for C, where I agree you shouldn't cast. Don't mix C and C++ - they share heritage and are similar but that doesn't make them the same. – Paul Floyd Mar 03 '23 at 07:33