Does deleting dynamically allocated std::string using a pointer returned by c_str() cause a memory leak in C++?

Question

I'm writing a function to load a txt file and return a const char* the function below works, my question is will this function cause a memory leak if I don't store *pS and then call delete pS ?

const char* loadFile(string fname)
{
   string line,text;
   ifstream in(fname);
   while(std::getline(in, line))
   {
       text += line + "\n";
   }

   string *pS = new string(text);
   const char* data = pS->c_str();

   return data;
}

the function is used in my code as follows

static const char* pVS;
...
pVS = loadFile("VS.txt");
...
delete pVS;

Will this delete the string ?

Answer 1

"Will this delete the string ?"

No. It will try to delete std::string's underlying character storage yielding an undefined behavior.
Even if it successfully freed that storage, there are other std::string's members that it wouldn't take care of, so yes, apart from undefined behavior, there's a memory leak as well.

Solution: Change your function to return std::string object instead. Alternativelly you might return std::vector<std::string> containing lines, which seems more reasonable than adding "\n".

---

To avoid memory leaks:

• avoid dynamic allocation always when it is possible and
• when it's not possible and you have to use new or new[], then make sure that:
  • for every new there's an appropriate delete call and
  • for every new[] there's an appropriate delete[] call.

  (note that this might be harder than it seems... especially when you're dealing with error-prone code while you still need to take care of every possible return path ~ that's one of the main reasons why it is always preferable to use RAII and smart pointers in C++)

Answer 2

This function does indeed cause a memory leak, and the use in code you've shown will invoke Undefined Behaviour to boot.

The function causes a leak because you dynamically allocate a std::string (the one you store in pS) and then lose its address once loadFile returns. There's no way to deallocate this string any more, so it's leaked.

This code:

pVS = loadFile("VS.txt");
...
delete pVS;

is even worse. You're obtaining a pointer which you have not allocated via new (it came from c_str()), and you're calling delete on it. That's undefined behaviour (most likely memory corruption), pure and simple.

The correct thing to do would be to change the function and just return the std::string:

string loadFile(string fname)
{
   string line,text;
   ifstream in(fname);
   while(std::getline(in, line))
   {
       text += line + "\n";
   }
   return text;
}

If and when the caller needs a const char* out of this, they can call c_str() themselves.

Answer 3

In order to avoid the memory leak and Undefined Behavior, instead of returning a char*, return a string.

std::string loadFile(string fname)
{
  // ...
   string retval (text);
   // ...
   return retval;
}

This returns a string by value, but under optimization the compiler will often elide the copy.

If you absolutely must return a char*, then do it right:

const char* loadFile(string fname)
{
   string line,text;
   ifstream in(fname);
   while(std::getline(in, line))
   {
       text += line + "\n";
   }

   string retval(text);
   char* data = new char [retval.length()+1];
   strcpy (retval.c_str(), data);
   return data;
}

Remember that on the other side of this you will have to delete this pointer, which means using delete []:

int main()
{
  const char* data = loadFile (...);
  delete [] data;
}

Answer 4

Use

return strdup(text.c_str());

The result is deletable. When you return a char* you won't be able to avoid doing your own memory management. Way better way would be to just return an std::string.