Creating a vector results in memory leak in C

Question

I am trying to create a vector in C, following is the struct declaration:

#define VECT_INITIAL_CAPACITY 4

typedef struct vect vect_t;

struct vect {
  char **data; // data is an array of strings             
  unsigned int size;      
  unsigned int capacity;   
};

I have a function that constructs a new empty vector, here is what I did:

vect_t *vect_new() {

  vect_t *v = (vect_t*) malloc(sizeof(vect_t));

  if (v == NULL) {
    return NULL;
  }

  // Allocate memory for data
  v->data = (char**) malloc(VECT_INITIAL_CAPACITY * sizeof(char*));
  if (v->data == NULL) {
    return NULL;
  }

  for (int i = 0; i < VECT_INITIAL_CAPACITY; i++) {
    v->data[i] = NULL;
  }

  // Initialize fields
  v->size = 0;
  v->capacity = VECT_INITIAL_CAPACITY;

  return v;
}

Valgrind tells me that the line v->data = (char**) malloc(VECT_INITIAL_CAPACITY * sizeof(char*)); caused memory leak. But I'm not sure how to fix this. Can anyone point out what caused the memory leaks?

Edit: Added my cleanup code below:

/** Free all the memories of the vector. */
void vect_delete(vect_t *v) {

  // Delete data first
  for (int i = 0; i < v->size; i++) {
    free(v->data[i]);
  }
  // Delete the vector
  free(v);
}

You're not showing any cleanup code, so you'll need to post that. Remember, anything that was returned from `malloc` should be passed to `free` to clean it up. — dbush, Feb 07 '23 at 03:13
If you **have to** cast the result of malloc then you compile the C code using C++ compiler and it is not right — 0___________, Feb 07 '23 at 03:16
PSA: If you're doing a lot of tricky allocations consider `calloc()` which makes it clear *what* and *how many* you're allocating in a very consistent way. Additionally, it's not necessary to [cast the result of an allocation](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) in C. — tadman, Feb 07 '23 at 03:17
`vect_t *vect_new()` in C++ it OK, in C not. `vect_t *vect_new(void)` — 0___________, Feb 07 '23 at 03:18
`v->data = malloc(...)` but there is no corresponding `free(v->data)`. Also: if the first malloc passes but the second fails you have a memory leak (it probably doesn't matter at that point, but still...) — John3136, Feb 07 '23 at 03:21
`VECT_INITIAL_CAPACITY` "INITIAL" suggests you may `realloc()` at some time... Please understand that the `realloc()` in the "add to collection" function can cope (vey well) with an INITIAL allocation that is zero... Ponder on this... — Fe2O3, Feb 07 '23 at 03:31
I noticed the allocation of memory for "v->data" based upon "VECT_INITIAL_CAPACITY", but in your cleanup function, you utilize element "v->size" in your for loop for freeing the data. If that value is never changed from zero, I don't see the call to free ever happening. Should you be using v->capacity? — NoDakker, Feb 07 '23 at 03:35

0___________ · Accepted Answer · 2023-02-07T09:59:41.033

5

/** Delete the vector, freeing all memory it occupies. */
void vect_delete(vect_t *v) 
{
    if(v)
    {
        // Delete data first
        if(v -> data)
            for (size_t i = 0; i < v->capacity; i++)  //not v->size
            {
                free(v->data[i]);
            }
        free(v -> data);  //missing
        // Delete the vector
        free(v);
    }
}

Some remarks:

Use the correct type for sizes and indexes (size_t) not int or unsigned
Use objects instead of types in sizeofs. vect_t *v = malloc(sizeof(*v));
Do not cast the results of void * functions like malloc. If the code does not compile, then you use the wrong compiler (C++ one) to compile C code.

edited Feb 07 '23 at 09:59

answered Feb 07 '23 at 03:37

0___________

60,014
4
34
74

BTW, good use of `if(v)`. Simplifies calling calling code' responsibilities. – chux - Reinstate Monica Feb 07 '23 at 03:53
May I ask the purpose of having free(v->data) after the for loop? – d0nut Feb 07 '23 at 04:51
@d0nut because you allocate memory for it – 0___________ Feb 07 '23 at 09:52

score 2 · Answer 2 · edited Feb 07 '23 at 04:09

2

In addition to @0___________ good answer,

if (v->data == NULL) { return NULL; } also leaks.

Use if (v->data == NULL) { free(v); return NULL; }.

edited Feb 07 '23 at 04:09

Dúthomhas

8,200
2
17
39

answered Feb 07 '23 at 03:51

chux - Reinstate Monica

143,097
13
135
256

Creating a vector results in memory leak in C

2 Answers2