heap corruption in multi-threaded object

Question

I'm trying to build a singleton class which will hold a cyclic buffer , every time some function wants to dump send something to a debug client (e.g DBGview or similar) the function will store its information inside the cyclic buffer. the object will also run a thread that will sleep for some time and then wake up and dump the buffer contents to the debug client.

this is the function everyone calls when need to send something to debug:

     TTFW_LogRet PrintToLogger (std::string Str)
     {
         Logger *Log;
         if(Instance_Mutex == NULL)
         {
              Instance_Mutex = CreateMutex(NULL,FALSE,NULL);
              if (Instance_Mutex == NULL) 
              {
                   OutputDebugStringA("CreateMutex error! the Logger will close \n");
                   return GENERAL_ERROR;
              }
          }
          Log = Logger::getLogInstance();
          Log->LogStringToOutput(Str);
          return SUCCESS;
       }

for some reason i always get a heap corruption errors after the return SUCCESS statement

this is the Getinstance() function:

Logger* Logger::getLogInstance()
{
    if(_instance == NULL)
    {
       MutexLock(Instance_Mutex);
       if(_instance == NULL)
       {
           _instance = new Logger();
       }
       MutexUnlock(Instance_Mutex);
   }
   return _instance;
  }

and the thread function is:

    DWORD WINAPI Logger::WorkerThread (LPVOID lpParam )
    {
         Logger *log = static_cast <Logger*> (lpParam);

         for(;;)
         {

              Sleep(2000);
              if(log->getOutputMethod() == odBuffer && log->CyclicBuffer1.size() >= log->Thresh && !log->CyclicBuffer1.empty())
         {
                  TTFW_LogRet ret;
                  ret = log->FullBufferDump();
                  if (ret != SUCCESS)
                  {
                       log->LogStringToOutput("Error occured in dumping cyclic buffer , the buffer will be cleared\n");
                  }
            }
          }
         }

does anybody have any idea why would a heap corruption occur with that code? note that the function to debug gets called many times and it is no synchronized with the thread who dumps the buffer.

[You should avoid the singleton pattern.](http://stackoverflow.com/a/138012/1007504) — Robert Allan Hennigan Leahy, Aug 14 '14 at 15:23

score 0 · Answer 1 · answered Aug 14 '14 at 15:52

You use double checked locking which can cause subtle problems of the kind you see. In particular, C++ allows that the line

_instance = new Logger();

may cause the _instance to get non-NULL value before the Logger constructor is completed. Then if another thread calls getLogInstance the condition if(_instance == NULL) will not be met and the thread will use not fully initialised object. Consider using straightforward locking without double checking, or some other - maybe implementation dependent - technique. In C++11 you can safely use static variables for such purposes.

I know that but the chances for that to happend are small and for my current application i just need to check something for performance I dont really need it to run in a multi-threaded for now. the corruption occured in single-thread enviroment. — Dima Shifrin, Aug 15 '14 at 09:09

score 0 · Answer 2 · answered Aug 14 '14 at 16:05

0

My psychic debugging skills tell me that LogStringToOutput is not thread-safe and is causing the heap corruption when called from multiple threads.

answered Aug 14 '14 at 16:05

Mark B

91,641
10
102
179

This function is never used without locking (only when things go wrong here and i'll take it in mind) but in the cases i had the error of heap corruption there were no calls to that function without locking it first. – Dima Shifrin Aug 15 '14 at 09:04

heap corruption in multi-threaded object

2 Answers2