18

I have got a really bad memory leak I am trying to fix, but somehow i am not able to delete Objects without triggering this assertation.

I have searched for a solution via Google and have read the Questions on stackoverflow about this Error but I was still not able to find the answer!

Possible reasons to get this Error according to my research:
1. deleting objects more then one
2. shadow copying
3. creating and deleting Objects that are loaded from an external dll
4. creating objects without storing the pointer

BUT:
1. I checked the code and was not able to find double deletion
2. I use a copy constructor to copy Objects
3. The Error relatet classes are build (with MS Visual Studio) to a seperate lib but not to a dll. AND all the classes that are related to this error are located in the same lib.
4. I checked the code and it seems like that's not the problem

It would be great if anybody is able to spot the mistake in the code below, and I appreciate every hint that points me to the solution of the problem.

EDIT:
I forgot to mention the same deleting problem in sendThreadMain of MessageSystem (see code below). If i delete the Message there it causes unexpected errors somewhere else in the code. Might just be wrong data transmission... but i do not really know.
This code is run on Windows and Linux!

Here are the error related parts of the code:

Message

class Message 
{
public:
    Message (char type, unsigned char id, unsigned short size) 
    {
        mType = type;
        mId = id;
        mSize= size;
    }

    Message(const Message &o)
    {
        mType = o.mType;
        mId = o.mId;
        mSize = o.mSize;
    }

    char getType() const {return mType;};
    unsigned char getId() const {return mId;};
    unsigned short getSize() const {return mSize;};

protected:
    char mType;
    unsigned char mId;
    unsigned short mSize;
};


class JoinMessage : public Message
{
public:
    JoinMessage () : Message ('j', 0, sizeof (JoinMessage))
    {
        team = TEAM_SPECTATOR;
    }
    JoinMessage (unsigned char id) : Message ('j', id, sizeof (JoinMessage)){}
    JoinMessage (const JoinMessage &o) : Message (o)
    {
        team = o.team;
        setName(o.getName());
    }


    void setName(std::string newName)
    {
        if (newName.length() > MAX_PLAYER_NAME_LENGHT)
            newName = newName.substr(0, MAX_PLAYER_NAME_LENGHT);

        memset(name, 0, MAX_PLAYER_NAME_LENGHT);
        for(unsigned int i = 0; i < newName.length(); i++)
            name[i] = newName[i];
    }

    std::string getName() const
    {
        std::string stringToReturn;

        for(unsigned int i = 0; i < MAX_PLAYER_NAME_LENGHT; i++)
        {
            if (name[i])
                stringToReturn.push_back(name[i]);
            else
                break;
        }

        return stringToReturn;
    }

    TeamIdentifier team;

private:
    unsigned char name[MAX_PLAYER_NAME_LENGHT];
};

// there are a lot other messages

MessageQueue

MessageQueue::~MessageQueue()
{
    boost::mutex::scoped_lock lock (queueMutex);

    while(messageQueue.size() > 0)
    {
        // the crash is non-reproducible
        // works 90% of the time
        delete messageQueue.front (); // <- Debug Assertion Failed … _BLOCK_TYPE_IS_VALID
        messageQueue.pop_front();
    }

}

void MessageQueue::enqueMessage (Message* message)
{
    {
        boost::mutex::scoped_lock lock (queueMutex);
        messageQueue.push_back(message);
    }
}

Message* MessageQueue::dequeMessage ()
{
    boost::mutex::scoped_lock lock (queueMutex);
    if (messageQueue.size() == 0) 
        return nullptr;

    Message* message = messageQueue.front ();
    messageQueue.pop_front();

    return message;
}

MessageSystem

template <class MessageType>
void broadcast (MessageType &message)
{
    MessageType *internMessage = new MessageType(message);

    boost::mutex::scoped_lock lock (mRecipientMapMutex);
    std::map <boost::asio::ip::udp::endpoint, MessageQueue *>::iterator it;

    for (it = mRecipientMap.begin ();
        it != mRecipientMap.end ();
        it++)
    {
        it->second->enqueMessage(internMessage);

    }
}


template <class MessageType>
void post (MessageType &message, boost::asio::ip::udp::endpoint &recipient)
{
    MessageType *internMessage = new MessageType(message);

    std::map <boost::asio::ip::udp::endpoint, MessageQueue* >::iterator it;
    MessageQueue *messageQueue = NULL;
    {
        boost::mutex::scoped_lock lock (mRecipientMapMutex);
        it = mRecipientMap.find (recipient);
        if (it != mRecipientMap.end())
            messageQueue = it->second;

        if(messageQueue)
            messageQueue->enqueMessage (internMessage);
    }

}


void MessageSystem::sendThreadMain ()
{
    // copy endpoints to vecotr so it can be
    // deleted from map while iterating
    std::vector<udp::endpoint> endpoints;
    {
        boost::mutex::scoped_lock lock (mRecipientMapMutex);
        std::map <udp::endpoint, MessageQueue *>::iterator mapIt = mRecipientMap.begin ();
        while (mapIt != mRecipientMap.end())
        {
            endpoints.push_back(mapIt->first);
            mapIt++;
        }
    }

    std::vector<udp::endpoint>::iterator endpointIt = endpoints.begin();
        while (endpointIt != endpoints.end())
        {
            char sendBuffer[PACKET_SIZE];
            int sendBufferPosition = 0;
            {
                boost::mutex::scoped_lock lock (mRecipientMapMutex);

                MessageQueue *messageQueue = mRecipientMap[*endpointIt];
                if (messageQueue == nullptr)
                {
                    mRecipientMap.erase(*endpointIt);
                    endpointIt++;
                    continue;
                }

                while (Message *message = messageQueue->dequeMessage ())
                {
                    if (sendBufferPosition + message->getSize() > PACKET_SIZE) 
                    {
                        // put message back and send it later
                        messageQueue->enqueMessage (message);
                        break;
                    }

                    // copy message into buffer
                    std::memcpy (
                        &sendBuffer [sendBufferPosition], message, message->getSize());

                    sendBufferPosition += message->getSize();
                    // deleting this message causes a crash if 2 or more
                    // recipients are registered within MessageSystem
                    //delete message; <- RANDOM CRASH elsewhere in the program
                }
            }
    .... // more code down here that seems not related to the error
cwin
  • 956
  • 1
  • 7
  • 14
  • You do not add messages to the queue in any other place than the `broadcast` function? – Some programmer dude Dec 02 '11 at 10:26
  • There's good news and bad news. The good news is that this most definitely is not caused by a memory leak. – Hans Passant Dec 02 '11 at 10:30
  • I'm not sure if this is the problem but you should have a virtual destructor in the class Message. –  Dec 02 '11 at 10:35
  • If you can build in Linux, you can use valgrind to pinpoint memory leaks very accurately – Gearoid Murphy Dec 02 '11 at 10:45
  • @JoachimPileborg: all calls are now listed in the code above. Please take a closer look at MessageSystem. – cwin Dec 02 '11 at 11:00
  • @Gearoid Murphy: yes I can, I will take a look at that. THX. – cwin Dec 02 '11 at 11:05
  • @HansPassant: the problem I am trying to solve is that NOT deleting the messages causes memory leaks. The Memory leak does not crash my application but I want to get rid of them because I get 100mb per hour – cwin Dec 02 '11 at 11:07
  • For a while I thought it might have been a one-off error in `JoinMessage::setName`, because the error you get is typical of writing to memory outside of allocated areas. It's not a problem with the name since you don't handle it like a string, but it still seems to me that you are overwriting something somewhere. – Some programmer dude Dec 02 '11 at 11:37
  • Most often, #1 is the problem that leads to this "assertion failure." – Rishi Jul 09 '14 at 15:14
  • I found a #5 for you, using new[] without delete[] – Josh C Jan 13 '16 at 16:19
  • In my case it was #6 as mentioned in http://stackoverflow.com/a/27769245: the behavior is undefined (i.e. resulted in the assertion failure) if delete is called on the static type base class but its destructor is not defined virtual. – hchr Aug 10 '16 at 12:46

3 Answers3

5

Today I figured it out on my own. It was #1 of the 4 possibilities mentioned in the Question.

  1. deleting objects more then once (by saving multiple pointers to the exact same object)

Here is my Solution in MessageQueue:

template <class MessageType>
void broadcast (MessageType &message)
{
    // I was creating 1 new Message right here but I need 1 new Message
    // in EVERY MessageQueue so i moved the next line ...
    // MessageType *internMessage = new MessageType(message);

    boost::mutex::scoped_lock lock (mRecipientMapMutex);
    std::map <boost::asio::ip::udp::endpoint, MessageQueue *>::iterator it;

    for (it = mRecipientMap.begin ();
        it != mRecipientMap.end ();
        it++)
    {
        // ... down here. Now every queue contains its own copy of the Message
        MessageType *internMessage = new MessageType(message);
        it->second->enqueMessage(internMessage);
    }
}
cwin
  • 956
  • 1
  • 7
  • 14
1

Well, I faced similar problem, the following code

Message* message = messageQueue.front ();
messageQueue.pop_front();

return message;

The code that produced error with me was:

Point *p = q.LookFor(&q, &pts[5], &Dist);
cout ...
delete p;

It seems that the function delete the pointer it creates in the runtime, so you're not allowed to delete it "again"

so I replaced it with

Point p = *(q.LookFor(&q, &pts[5], &Dist));

and it's gone.

Ahmed U3
  • 698
  • 5
  • 9
1

It might be a simple problem of wrong order. You are doing:

while(messageQueue.size() > 0)
{
    delete messageQueue.front();
    messageQueue.pop_front();
}

Maybe deleting the message after popping it, instead of before, would do the trick:

while(messageQueue.size() > 0)
{
    Message* pFront = messageQueue.front();
    messageQueue.pop_front();
    delete pFront;
}

Anyway, I am not confident at all on this solution, since deleting the object pointed by pFront should have no effect on the queue itself, which just stores pointers. But you can try.

Gorpik
  • 10,940
  • 4
  • 36
  • 56
  • 1
    I also think that it should have no effect on the queue, but I'll try that anyway. But it seems to be another problem since deleting Messages does not work after removing them from the Queue (see while loop at the end of MessageSystem) – cwin Dec 02 '11 at 11:17