C++ function is leaking memory, I'm new to C++ and not sure how to fix it

Question

I'm very new to C++ and is trying to compile a program, however it's leaking memory and this part of the code.

char* ReadString(u_int address, unsigned int size) {
    char* data = new char[size];
    for(int i=0; i < size; i++){
        vm_readv(reinterpret_cast<void*>(address + (sizeof(char)*i)), reinterpret_cast<void*>(data + i), sizeof(char));
        if(data[i] == '\0'){
            break; 
        }
    }
    return data;
}

I'm not sure how to fix it.

I've tried to add delete[] data; before break; and it stops the memory, the program also runs. But i think this may crash the program somewhere after??

At any rate, I'm confused on how to properly deal with the leak.

I've been reading and using a smart pointer might be a good way to resolve it, but again, I don't know how to properly turn char* data = new char[size]; into a pointer without breaking data[i].

Edit: Attempts on stopping the leak, the leak stop with this. But I think it may cause a crash later?

char* ReadString(u_int address, unsigned int size) {
    char* data = new char[size];
    for(int i=0; i < size; i++){
        vm_readv(reinterpret_cast<void*>(address + (sizeof(char)*i)), reinterpret_cast<void*>(data + i), sizeof(char));
        if(data[i] == '\0'){
            delete [] data; // ------->>>>>>> Here
            break; 
        }
    }
    return data;
}

Welcome to StackOverflow! I see you're a new contributor, so I advise you to check out [How to ask a good question](https://stackoverflow.com/help/how-to-ask) and [How to create a minimal reproducible example](https://stackoverflow.com/help/minimal-reproducible-example). At the moment, its hard to tell exactly what you're asking. You are aware that you need to free memory _after_ you're done using it, right? The code you've posted so far doesn't show us when or how you are attempting to free memory, so we can't comment on how memory is being leaked. — Brian61354270, Jun 21 '20 at 20:18
The problem is in the code that uses ReadString - it somewhere stores pointer to allocated memory and should take responsibility for freeing it. There is no other safe way to handle leaks than with RAII - the easiest way will be to switch to std::vector... — marcinj, Jun 21 '20 at 20:22
*I'm confused on how to properly deal with the leak.* -- `std::vector ReadString(u_int address, unsigned int size) { std::vector data(size); ... return data;}` — PaulMcKenzie, Jun 21 '20 at 20:26
@Brian Thank you! Yes I'm aware I need to free the memory after using it, it's why I'm trying to free it, but delete [] data; doesn't seems to be a good solution, and It's a pretty long chain of function, not sure how to delete it later?? Sorry, I'm used with PHP & LUA where everything is a lot simpler x( This C++ is confusing me, hahaha... @JesperJuhl I tried using std::vector but I'm getting this `no viable conversion from returned value of type 'std::vector' to function return type 'char *'` — Helvanth, Jun 21 '20 at 20:26
@Helvanth Your solution is not a solution, since it only conditionally calls `delete[]`, thus the code can still leak. Also C++ would be much easier for you if you didn't use raw memory allocation and pointers, and instead from the start used containers such as `std::vector`. — PaulMcKenzie, Jun 21 '20 at 20:29
Since you are returning `data`, you must be using `data` ***after*** you return it. You have to free the memory ***after*** you finish using `data`, not before it. — Sam Varshavchik, Jun 21 '20 at 20:29
@Helvanth Change the return type to `std::vector` as well. *"PHP & LUA where everything is a lot simpler ... C++ is confusing me"* That's because instead of using the easy C++ features (`std::vector`), you instead started with the old C-like features (raw pointers and manual memory management). C++ can be easy if you use it properly. — HolyBlackCat, Jun 21 '20 at 20:31
Recommendation: [Get a good, up-to-date book](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list). Don't learn the fundamentals of C++ from the Internet and, unfortunately, you have to doublecheck a lot of academic resources. There's too much baffle-gab and outright bad information out there. The odds of finding the good stuff before you are mislead by a lot of bad stuff is low. — user4581301, Jun 21 '20 at 21:16

score 2 · Accepted Answer · answered Jun 21 '20 at 20:44

Don't use raw new and delete. It was never good practice to use new whenever you need to create an object in C++. If you need a string then use std::string. It's resize method lets you resize it and some_string[i] lets you access the i-th character. I assume vm_readv reads a single character, though I am a bit confused by the usage of void* and reinterpret_cast.

std::string ReadString(u_int address, unsigned int size) {
    std::string result;
    result.resize(size);
    for (size_t i=0;i<size;++i) {
         // read result[i]
         // its address is &result[i]
    }
    return result;
}

In your code the leak is not in the function but in the caller of the function. Because the function returns data it transfers ownership of it (= responsibility to delete it) to the caller. Not using manual memory allocations is simpler, less error prone and if you need a string of dynamic size the problem of managing the memory has already been solved for you: std::string.

this leads me into properly resolving the memory problem, thank you for the clear explaination! especially the address & result. — Helvanth, Jun 21 '20 at 21:49

C++ function is leaking memory, I'm new to C++ and not sure how to fix it

1 Answers1