Using delete[] in C++ isn't freeing all the memory

Question

If I use the delete[] operator on my char* arrays (a*, b*, and c*), valgrind gives me 3 errors from 3 contexts and a memory leak error, saying "LEAK SUMMARY: ==359== definitely lost: 51 bytes in 8 blocks," but if I remove those delete statements I lose 60 bytes, meaning that the delete[] is doing something. However, when I move the delete operator into the for loop, it works properly, saying "All heap blocks were freed -- no leaks are possible" but then I get 27 errors from 6 contexts, mainly from invalid read/writes as the address the code is trying to access is in a block that has already been free'd. How do I free all the heap blocks without getting these invalid read/write errors?

Your code is long and complicated, but what can I tell from a quick glance is that you do multiple `new char[...]` in a loop. But then you free those pointers outside a loop. That's likely a memory leak. Plus if `if` condition is not met you `delete[]` uninitialized pointers. Plus you use variable length arrays which are not C++ standard. — freakish, Oct 15 '21 at 06:30
Your allocations for `a`, `b`, and `c` are performed in a loop, while `delete[]` only happens once (after the loop is finished). If the loop executes the allocating code more than once, all but the last allocation is leaked. — ShadowRanger, Oct 15 '21 at 06:31
Your code - as I understand it - boils down to `while(readSomething) { if (condition) { a = new char[...]; } } delete a;` If `condition` is true in more than one iteration you leak memory. — Lukas-T, Oct 15 '21 at 06:32
Please reduce your code to a [mcve]. In any case, it's not because you use `delete[]`, but because you're doing things incorrectly. — Ulrich Eckhardt, Oct 15 '21 at 06:33
Btw, why even so complicated? You read something, take a substring, copy that to a buffer (that might overflow!), then allocate a plain char array and copy the buffer there ... why not just make `wordbank` a `std::vector` and work with `std::string` all the time? I think you really overcomplicate things. — Lukas-T, Oct 15 '21 at 06:34
It also looks like your program can potentially free memory which does not belong to you. For example, if ```else if``` block in ```for``` loop is never visited, then the value of ```a``` lefts uninitialized and freed on the end of the program. To avoid this, initialize pointers with ```nullptr```, freeing it does not have any effects. — elo, Oct 15 '21 at 06:35
Meaningful variable names would go a long way toward making this code readable for us and easier for you to read, write, debug, and maintain. — Retired Ninja, Oct 15 '21 at 06:45
`*(wordBank + x) = 0;` (i.e. `wordBank[x] = 0;`) has undefined behaviour and makes your entire program invalid. — molbdnilo, Oct 15 '21 at 07:23

freakish · Answer 1 · 2021-10-15T06:56:06.077

Here are few observations:

1.

int x;
if(getline(myfile, check)){
    x = stoi(check);
}
char* wordBank[x];

You use variable-length array which is not standard C++. Secondly if the if condition is not met you have a very bad UB here.

2.

char* a;
...
if (...) {
   for (...) {
       if (...) {
           a = new char[...];
       }
   }
}
...
delete[] a;

There are two issues with this code: the first is that if no if condition is met you delete[] an unitialized pointer. This is UB. Secondly, consider the case when the loop spins two times (and it reaches new char[] line each time). In the first iteration you create a new pointer, in the second iteration you overwrite that pointer with a new pointer. And then you free that last pointer. But the first pointer is lost forever, you have a memory leak.

There might be more issues with the code but that's all I can see at the moment.

In order to solve that I strongly suggest you get rid of raw pointers and use standard C++ structures like vectors, strings and maybe even unique_ptrs (which would also solve another issue with your code: it is not exception safe). Anyway some major refactoring is ahead of you.

Using delete[] in C++ isn't freeing all the memory

1 Answers1