How to avoid double free in destructor

Question

I have this sample code(smaller version of the actual project) where we were seeing memory leak due to memory getting allocated dynamically in the constructor(also implicit constructor calls making it more problematic in the project) and not deallocated. So I just tried writing a smaller version and deallocate memory in the destructor, but program terminates due to double free.

using namespace std;
class UDPCPChecksum
{
    int A;
    int B;
};

class UDPCPMessage
{
    UDPCPChecksum * _udpcpChecksum;
    int A;
    public:
        UDPCPMessage();
        UDPCPMessage(const std::string& msg);
        ~UDPCPMessage(){
            cout<<"trying to delete"<<endl;
            if(_udpcpChecksum !=NULL){
                delete _udpcpChecksum;
                _udpcpChecksum = NULL;}
            }
        UDPCPMessage(const UDPCPMessage& udpcpMessage);
        UDPCPMessage& operator=(const UDPCPMessage& udpcpMessage);
};

UDPCPMessage::UDPCPMessage(const std::string& msg)
{
    cout<<"parameter constructor called"<<endl;
    _udpcpChecksum = new UDPCPChecksum();
}

UDPCPMessage::UDPCPMessage()
{
    cout<<"default constructor called"<<endl;
    _udpcpChecksum = new UDPCPChecksum();
}

UDPCPMessage::UDPCPMessage(const UDPCPMessage& udpcpMessage)
{
    cout<< "copy constructor called"<<endl;
    _udpcpChecksum          = udpcpMessage._udpcpChecksum;
}

UDPCPMessage& UDPCPMessage::operator=(const UDPCPMessage& udpcpMessage)
{
    cout<< "assignment called";
    _udpcpChecksum          = udpcpMessage._udpcpChecksum;
    return *this;
}

class UDPCPConnectionInterface{
    public:
    void sendAck(UDPCPMessage udpcpmessage);
};
void UDPCPConnectionInterface::sendAck(UDPCPMessage UDPCPMessage)
{
    cout<< "send Ack"<<endl;
    
}
int main()
{
    UDPCPMessage udpcp = UDPCPMessage();
    
    UDPCPConnectionInterface intf = UDPCPConnectionInterface();
    cout<<"calling sendAck"<<endl;
    intf.sendAck(udpcp);

    return 0;
}

o/p:

default constructor called
calling sendAck
copy constructor called
send Ack
trying to delete
trying to delete
free(): double free detected in tcache 2

Is there any race condition happening while delete? what is the better way to handle such cases?

Answer 1

The copy constructor copies a pointer to the checksum. So after the copy constructor there are two instances that reference the same checksum instance by raw pointer. Both will delete the raw pointer when they go out of scope. The copy constructor&assignment must do a deep copy of the checksum instance if you really want to store a raw pointer to a dynamically allocated checksum.

Answer 2

You need to read about the rule of five.

The copy constructor needs to allocate memory for a UDPCPChecksum and also copy the value of the resource to which the copied object points. Otherwise, you just copy the pointer and suddenly have two objects pointing at the same resource and both will then try to delete it in the destructor.

The one constructor that doesn't need to do this allocation is the move constructor (which you haven't implemented).

class UDPCPMessage {
    UDPCPChecksum* _udpcpChecksum;
    int A;

public:
    UDPCPMessage();
    explicit UDPCPMessage(const std::string& msg);

    // rule of five:
    UDPCPMessage(const UDPCPMessage& udpcpMessage);
    UDPCPMessage(UDPCPMessage&& udpcpMessage) noexcept;            // move ctor
    UDPCPMessage& operator=(const UDPCPMessage& udpcpMessage);
    UDPCPMessage& operator=(UDPCPMessage&& udpcpMessage) noexcept; // move assign
    ~UDPCPMessage();
};

The default constructor allocates:

UDPCPMessage::UDPCPMessage() : _udpcpChecksum(new UDPCPChecksum) {
//                           ^ member initializer list start
    std::cout << "default constructor called\n";
}

The rest of the allocating constructors can just delegate the allocating part to the default constructor:

UDPCPMessage::UDPCPMessage(const std::string& msg) : UDPCPMessage() {
//                                                   ^ delegate
    std::cout << "parameter constructor called with " << msg << '\n';
}

and the copy constructor needs to actually copy the resource that the pointer points at:

UDPCPMessage::UDPCPMessage(const UDPCPMessage& udpcpMessage) : UDPCPMessage() {
    std::cout << "copy constructor called\n";
    *_udpcpChecksum = *udpcpMessage._udpcpChecksum;  // copy
}

... or better, using the copy constructor of the UDPCPChecksum directly:

UDPCPMessage::UDPCPMessage(const UDPCPMessage& udpcpMessage) :
    _udpcpChecksum(new UDPCPChecksum(*udpcpMessage._udpcpChecksum))
//                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
{
    std::cout << "copy constructor called\n";
}

while the move constructor can simply steal the resource:

UDPCPMessage::UDPCPMessage(UDPCPMessage&& udpcpMessage) noexcept
    : _udpcpChecksum(std::exchange(udpcpMessage._udpcpChecksum, nullptr)) {
    std::cout << "move constructor called\n";
}

and similarly for the assignment operators:

UDPCPMessage& UDPCPMessage::operator=(const UDPCPMessage& udpcpMessage) {
    *_udpcpChecksum = *udpcpMessage._udpcpChecksum;  // copy
    std::cout << "copy assignment operator called\n";
    return *this;
}

UDPCPMessage& UDPCPMessage::operator=(UDPCPMessage&& udpcpMessage) noexcept {
    // swap leaves both *this and `udpcpMessage` in a valid state:
    std::swap(_udpcpChecksum, udpcpMessage._udpcpChecksum);
    std::cout << "move assignment operator called\n";
    return *this;
}

and finally, the destructor, that does not need to do a NULL pointer check. It's safe to delete a NULL pointer.

UDPCPMessage::~UDPCPMessage() {
    std::cout << "deleting\n";
    delete _udpcpChecksum;              // no need to check if it's NULL
}

Note: Since all instances of UDPCPMessage owns a UDPCPChecksum (via a pointer) you should probably just make it a normal member variable instead of a pointer.