Finding Reason For Memory Leaking With Char*

Question

I been working in a project that handles some char* pointers, and it's a requisite of the class to use char* instead of std::string, so...

I have this structure definition and this queue:

typedef struct packetQueue
{
    char* buf;
    int length;

    packetQueue()
    {
        buf = new char[];
        length = 0;
    }

    } PACKET;

concurrency::concurrent_queue IP_in_queue;

I have this buffer:

char sendBuf[MSG_SIZE + sizeof (IP_PACKET_HEADER_T) + 1]; // String to be send

and a structure for my new buffer:

PACKET ipQueue;

then I fill my buffer with this:

// Concatenates the header with sended message
memcpy(sendBuf, (void*)&sendHeader, sizeof(sendHeader));

memcpy(&sendBuf[sizeof(sendHeader)], readMessage, sendHeader.length);

ipQueue.buf = sendBuf;
ipQueue.length = packetSize;

And then I push my packet to my queue IP_in_queue.push(ipQueue); // Push the buffer in the IP_in_queue

This is my loop just in case:

while ( 1 )
{
    // Get the user input
    cout << "> ";
    cin.getline (buf, BUFLEN);

    IP_PACKET_HEADER_T sendHeader; // Store the header to be send
    PACKET ipQueue;

    char* fakeIPAddressDst, *readMessage; 

    delay = atoi(strtok (buf," ")); // Takes the first delay value
    fakeIPAddressDst = strtok (NULL, " "); // Stores the IP Address
    readMessage = strtok (NULL, " "); // Stores the sended message

    Sleep(delay); // Sleep the miliseconds defined

    // Fills the header with the data neccesary data
    sendHeader.DIP = inet_addr(fakeIPAddressDst);
    sendHeader.SIP = inet_addr(initAddress.fakeIpAddress);
    sendHeader.length = getStringLength(readMessage) + 1;
    packetSize = sizeof( sendHeader ) + sendHeader.length; // Defines the size of the packet to be send

    // Concatenates the header with sended message
    memcpy(sendBuf, (void*)&sendHeader, sizeof(sendHeader));
    memcpy(&sendBuf[sizeof(sendHeader)], readMessage, sendHeader.length);

    ipQueue.buf = sendBuf;
    ipQueue.length = packetSize;

    numbytes = packetSize; // The number of bytes of sended buffer

    char sendedString[BUFLEN + 1]; // Variable for stores the data
    IP_PACKET_HEADER_T readHeader; // To store the header for showing the information

    // Print out the content of the packet
    // Copy from buf to the header
    memcpy( (void*)&readHeader, ipQueue.buf, sizeof( IP_PACKET_HEADER_T));
    // Copy message part
    memcpy( sendedString, &ipQueue.buf[sizeof(IP_PACKET_HEADER_T)], numbytes - sizeof(IP_PACKET_HEADER_T));
    // Append \0 to the end
    sendedString[numbytes - sizeof(IP_PACKET_HEADER_T)] = '\0';

    // Save the IP information of the packet in a struct for print on the screen
    struct in_addr fakeAddrHost;
    fakeAddrHost.s_addr = readHeader.SIP;

    // Print the neccesary data
    cout << "[IN] DST: " << fakeIPAddressDst << endl; // Fake IP address of the destination
    cout << "[IN] SRC: " << inet_ntoa(fakeAddrHost) << endl; // Fake IP address of the host
    cout << "[IN] MSG: " << sendedString << endl ; // Message to send

    IP_in_queue.push(ipQueue); // Push the buffer in the IP_in_queue
}

I know there is a memory leak in this procedure but I'm not sure. When I push my packet, the buf pointer keeps pointing to my sendBuf, am I right? Because the assignment does that, but if I delete my pointer in the ipQueue after I push the program crashes. I have to say, after I push that struct into the queue, another thread try to pop that one, and obviously if I delete my ipQueue pointer I'll lost my buffer, so how can I avoid this memory leak?

Thanks

EDIT:

The memory leak using the definition of buf = nullptr

---------- Block 1 at 0x0068BB30: 264 bytes ----------
  Call Stack:
    d:\program files (x86)\microsoft visual studio 11.0\vc\include\concurrent_queue.h (402): Host.exe!Concurrency::concurrent_queue<packetQueue,std::allocator<packetQueue> >::_Allocate_page + 0xF bytes
    f:\dd\vctools\crt_bld\self_x86\crt\src\concurrent_queue.cpp (113): MSVCP110D.dll!Concurrency::details::_Micro_queue::_Push + 0xD bytes
    f:\dd\vctools\crt_bld\self_x86\crt\src\concurrent_queue.cpp (232): MSVCP110D.dll!Concurrency::details::_Concurrent_queue_base_v4::_Internal_push
    d:\program files (x86)\microsoft visual studio 11.0\vc\include\concurrent_queue.h (566): Host.exe!Concurrency::concurrent_queue<packetQueue,std::allocator<packetQueue> >::push + 0xF bytes
    d:\users\silex rpr\documents\visual studio 2012\projects\project2\project2\host.cpp (802): Host.exe!main
    f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c (536): Host.exe!__tmainCRTStartup + 0x19 bytes
    f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c (377): Host.exe!mainCRTStartup
    0x7662339A (File and line number not available): kernel32.dll!BaseThreadInitThunk + 0x12 bytes
    0x77179EF2 (File and line number not available): ntdll.dll!RtlInitializeExceptionChain + 0x63 bytes
    0x77179EC5 (File and line number not available): ntdll.dll!RtlInitializeExceptionChain + 0x36 bytes

Answer 1

The problem you have is that you haven't looked at copy construction and copy assignment: when you push an object into a std::vector<T> it gets copied and the objects with the std::vector<T> get possibly moved around using assignment. The default generated copy constructor and copy assignment just copy or assign the respective members, i.e., whenever either copy is used, you'd end up with two objects pointing to the same buf. The first one destroyed would use delete[] buf; and all others would have a stale pointer which can't be deleted again. That is, you want to add three methods to your packetQueue:

struct packetQueue
{
    packetQueue(packetQueue const& other);             // copy constructor: copies the content
    packetQueue& operator= (packetQueue const& other); //copy assignment: updates the content
    ~packetQueue()                                     // destructor: release the memory
    void swap(packetQueue& other);                     // swap the content
    // other members
};

To leverage the copy construction and destruction in the copy assignment, I find it useful to have a swap() member, because this is easily implemented and makes for a nice, simple copy assignment:

void packetQueue::swap(packetQueue& other) {
    std::swap(this->buf, other.buf);
    std::Swap(this->size, other.size);
}
packetQueue& packetQueue::operator= (packetQueue const& other) {
    packetQueue(other).swap(*this);
    return *this;
}

Answer 2

You've got a class (structure) with a constructor that allocates memory and no destructor that releases it, so you get memory leaks.

You also expose the buf member and assign to it; so your class has no control over whether the memory should be freed or not. But you need to free the memory allocated in the constructor before you assign to the buf the first time.

To get this right, you'll need to make the buf field private and add a destructor, a copy constructor and an assignment operator (and probably an accessor function). You'll still not be exception safe, though.

Answer 3

First off; this isn't C, you're using a C++ compiler. structures in C cannot have methods and constructors and new and delete don't exist.

Secondly, you allocate memory for buf in your constructor, but then...

ipQueue.buf = sendBuf;

That's a leak. You need to call delete for every call to new. You allocate buf with new, but never call delete on it, so that memory is leaked.

I see no reason to allocate buf here. Just set it to null.

typedef struct packetQueue
{
    char* buf;
    int length;

    packetQueue()
        : buf(nullptr), length(0) { }
} PACKET;

---

On a side note, this is a very nasty mix of C and C++. Is this what your teacher is teaching you guys?