Is there a bad interaction between gtest and cstdarg, or am I really missing the stack smash here?

Question

My code below is meant to be a simple error logging system that behaves in a way that is similar to printf.

All of my code has been running fine in a gtest environment, but now as I exit a deterministic point in the program (one of my tests) it stack smashes. My code has been running fine up until I wrote this and it is my first foray into cstdarg so it is the most suspect.

MyMutex error_lock;

#define MAX_ERR_MSG_SIZE 128
#define MAX_ERRORS 3
class ErrorQueue
{
   std::queue<char*> errors;
   std::list<char*> old_errors;
public:
   void push(char * msg)
   {
      if (errors.size() >= MAX_ERRORS)
      {
         pop();
      }
      errors.push(msg);
   }

   void pop()
   {
      if (old_errors.size() >= MAX_ERRORS)
      {
         delete [] old_errors.front();
         old_errors.pop_front();
      }
      old_errors.push_back(errors.front());
      errors.pop();
   }

   char * front()
   {
      return errors.front();
   }

   size_t size()
   {
      return errors.size();
   }

   ~ErrorQueue()
   {
      while(!errors.empty())
      {
         delete [] errors.front();
         errors.pop();
      }

      while (!old_errors.empty())
      {
         delete [] old_errors.front();
         old_errors.pop_front();
      }

   }

};

static ErrorQueue errors;


void WriteCallError(const char * error_message, ...)
{
   char err_buffer[MAX_ERR_MSG_SIZE];

   va_list args;
   va_start(args,error_message);
   std::vsnprintf(err_buffer,MAX_ERR_MSG_SIZE,error_message,args);
   va_end(args);

   char * err_string = new char[MAX_ERR_MSG_SIZE];
   memcpy(err_string,err_buffer,MAX_ERR_MSG_SIZE);
   {
      error_lock.Lock();
      errors.push(err_string);
      error_lock.Unlock();
   }
}

I call WriteCallError numerous times elsewhere in the code and after a certain number of times it pukes and tells me I've stack smashed.

Where is my fault? Is there some funky interaction between cstdarg and gtest? Is there even enough information here?

Edit: Using a simple main I tried isolating it:

int main (int argc, char ** argv)
{
   int i = 0;
   while (1)
   {
      WriteCallError("Breaks on %d",i++);
   }

}

This will not cause a stack smash.

Can you post a [sscce](http://sscce.org) demonstrating the stack smashing? — Sam Miller, Feb 20 '13 at 01:41
I'm trying to worry it down for use. This may end up being too localized — Alex Shepard, Feb 20 '13 at 01:42
I cant duplicate it in a sscce and the suspect code is too proprietary to post directly. I'll leave this open for anyone who has suggestions for a little while, but it doesn't seem like a good fit for StackOverflow. — Alex Shepard, Feb 20 '13 at 02:04
Look at all the calls to `WriteCallError` to ensure that you're passing the correct arguments for the specified format strings. Some compilers will catch invalid format specifiers for standard functions like `printf`, but cannot provide a warning for your own printf-style function. — Austin Phillips, Feb 20 '13 at 04:22

score 2 · Answer 1 · answered Feb 20 '13 at 13:03

I think your problem is that vsnprintf does not write a terminating nul character to the array if the data you are writing is beyond the limit you have specified. When you later access this C string, because there is no terminator, it'll read beyond the end of the valid data and into unknown memory.

You didn't spot this in your test because you aren't exceeding the limit of 128 chars.

The quick fix is to ensure that the array is terminated correctly after you have used vsnprintf:

void WriteCallError(const char * error_message, ...)
{
   char err_buffer[MAX_ERR_MSG_SIZE];

   va_list args;
   va_start(args,error_message);
   std::vsnprintf(err_buffer,MAX_ERR_MSG_SIZE,error_message,args);
   va_end(args);

   // Fix here
   err_buffer[MAX_ERR_MSG_SIZE - 1] = 0;

   char * err_string = new char[MAX_ERR_MSG_SIZE];
   memcpy(err_string,err_buffer,MAX_ERR_MSG_SIZE);
   {
      error_lock.Lock();
      errors.push(err_string);
      error_lock.Unlock();
   }
}

+1 for a great answer. It probably fixes possible issues in the future, but it doesn't seem to solve my current problem. Probably means that the smash is coming from another location. — Alex Shepard, Feb 20 '13 at 15:42

score 0 · Answer 2 · answered Feb 20 '13 at 16:29

Answering so I can close the question since I solved the bug:

It happened in code similar to this:

TEST(MyTest,Case1)
{
   MyStruct1 object1;
   memset(&object1,0,sizeof(object1));

   MyStruct2 object2[1];
   memset(&object2[0],0,sizeof(object1));

   object1.object2_ptr = object2;


   // DO SOME TESTING

}

Since object1 & object2 are stack variables and object1 is bigger than object2 I was setting the memory out too far when I memset. When I left that function's part of the stack the stack pointer shouted that something crazy was going on and told me I was stack smashing.

Simple fix:

TEST(MyTest,Case1)
{
   MyStruct1 object1;
   memset(&object1,0,sizeof(MyStruct1));

   MyStruct2 object2[1];
   memset(&object2[0],0,sizeof(MyStruct2));

   object1.object2_ptr = object2;


   // DO SOME TESTING

}

Is there a bad interaction between gtest and cstdarg, or am I really missing the stack smash here?

2 Answers2