How to make this code less memory leak prone?

Question

As an introduction, note that I am a Java programmer still getting used to the memory management issues in C++.

We have a base class which is used to encoded objects to a string of ASCII characters. Essentially, the class is using a stringstream class member to convert different datatypes to one long string, and then returns a char* to the caller which contains the encoded object data.

In testing for memory leaks, I am seeing that the implementation we are using seems prone to create memory leaks, because the user has to always remember to delete the return value of the method. Below is an excerpt of the relevant parts of the code:

    char* Msg::encode() 
    {
        // clear any data from the stringstream
        clear();
        if (!onEncode()) {
            return 0;
        }

        // need to convert stringstream to char*
        string encoded = data.str();
                    // need to copy the stringstream to a new char* because 
                    // stringstream.str() goes out of scope when method ends
        char* encoded_copy = copy(encoded);
        return encoded_copy;
    }

    bool Msg::onEncode(void)
    {
        encodeNameValue(TAG(MsgTags::TAG_USERID), companyName);
        encodeNameValue(TAG(MsgTags::TAG_DATE), date);
        return true;
    }

    bool EZXMsg::encodeNameValue(string& name, int value)
    {
        if(empty(value))
        {
            return true;
        }
                    // data is stringstream object
        data << name << TAG_VALUE_SEPARATOR << value << TAG_VALUE_PAIRS_DELIMITER;
        return true;
    }


    char* copy(string& source) {
        char *a=new char[source.length() +1];
        a[source.length()]=0;
        memcpy(a,source.c_str(),source.length());
        return a;
    }

UPDATE

Well - I should have been more accurate about how the result of encode() is consumed. It is passed to boost:async_write, and program is crashing because I believe the string goes out of scope before async_write complete. It seems like I need to copy the returned string to a class member which is alive for life time of the class which sends the message (?).

This is the way the encode() method is actually used (after I changed the return value of to string):

void iserver_client::send(ezx::iserver::EZXMsg& msg) {
       string encoded = msg.encode();   
       size_t bytes = encoded.length();
       boost::asio::async_write(socket_, boost::asio::buffer(encoded, bytes), boost::bind(&iserver_client::handle_write, this, boost::asio::placeholders::error, boost::asio::placeholders::bytes_transferred));    
}

It looks like the proper way to do this is to maintain a queue/list/vector of the strings to async write. As noted here (and also in the boost chat_client sample). (But that is a separate issue.)

Answer 1

For this question: in your copy function you return a pointer to a heap memory!So user maybe create memory leak,I think you can not use this copy function,you can do just like this in your encode func:

return data.str();

If you want to get a char*, you can use the member function of string:c_str(), just like this:

string ss("hello world");
const char *p = ss.c_str();

If you use a stack string object you will not create memory leak,

Answer 2

You could just return a std::string. You have one there anyway:

string Msg::encode() 
{
    // clear any data from the stringstream
    clear();
    if (!onEncode()) {
        return string{};
    }

    return data.str();
}

Then the caller would look like:

Msg msg;
msg.userID = 1234;
send(msg.encode().c_str());

Answer 3

The only way of achieving "automatic" deletion is with a stack variable (at some level) going out of scope. In fact, this is in general the only way of guaranteeing deletion even in case of an exception, for example.

As others mentioned std::string works just fine, since the char * is owned by the stack-allocated string, which will delete the char *.

This will not work in general, for example with non char * types.

RAII (Resource Acquisition is Initialization) is a useful idiom for dealing with such issues as memory management, lock acquisition/release, etc.

A good solution would be to use Boost's scoped_array as follows:

{
  Msg msg;
  msg.userID = 1234;
  scoped_array<char> encoded(msg.encode());
  send(encoded.get());
  // delete[] automatically called on char *
}

scoped_ptr works similarly for non-array types.

FYI: You should have used delete[] encoded to match new char[source.length() +1]

Answer 4

While using a std::string works adequately for your specific problem, the general solution is to return a std::unique_ptr instead of a raw pointer.

std::unique_ptr<char[]> Msg::encode() {
    :
    return std::unique_ptr<char[]>(encoded_copy);
}

The user will then get a new unique_ptr when they call it:

auto encoded = msg.encode();
send(encoded.get());

and the memory will be freed automatically when encoded goes out of scope and is destroyed.