Why am I getting 'Heap Corruption'?

Question

Please don't crucify me for this one. I decided it might be good to use a char* because the string I intended to build was of a known size. I am also aware that if timeinfo->tm_hour returns something other than 2 digits, things are going to go badly wrong. That said, when this function returns VIsual Studio goes ape at me about HEAP CORRUPTION. What's going wrong? (Also, should I just use a stringbuilder?)

void cLogger::_writelogmessage(std::string Message)
{
    time_t rawtime;
    struct tm* timeinfo = 0;

    time(&rawtime);
    timeinfo = localtime(&rawtime);

    char* MessageBuffer = new char[Message.length()+11];
    char* msgptr = MessageBuffer;

    _itoa(timeinfo->tm_hour, msgptr, 10);
    msgptr+=2;

    strcpy(msgptr, "::");
    msgptr+=2;

    _itoa(timeinfo->tm_min, msgptr, 10);
    msgptr+=2;

    strcpy(msgptr, "::");
    msgptr+=2;

    _itoa(timeinfo->tm_sec, msgptr, 10);
    msgptr+=2;

    strcpy(msgptr, " ");
    msgptr+=1;

    strcpy(msgptr, Message.c_str());

    _file << MessageBuffer;

    delete[] MessageBuffer;
}

Answer 1

You need to allocate one more byte, since .length of a string returns its length without the terminating NUL, for which you also need space in the char*.

I.e. suppose Message.length() returns 10. You allocate 21 bytes. Copy 11 bytes into the buffer, then copy the message, which needs 10 bytes + one for NUL. Total: 22 bytes, and you only have 21 allocated.

Answer 2

This

char* MessageBuffer = new char[Message.length()+11];

should be

char* MessageBuffer = new char[Message.length()+12];

Because you are adding 11 additional char to the buffer:

2 for hr
2 for ::
2 for min
2 for ::
2 for sec
1 for " "

you need one additional to for the terminating null char.

Answer 3

As others have pointed out, the size of MessageBuffer needs to be increased by one.

However, rather than dealing with the raw char buffer in that way, you could just stream the time information directly to _file without putting it into an intermediate string first. If you want it in an intermediate string for some reason, I would suggest that you make use of the ostringstream class.

void writelogmessage(std::string Message)
{
    time_t rawtime;
    struct tm* timeinfo = 0;

    time(&rawtime);
    timeinfo = localtime(&rawtime);

    std::ostringstream stream;
    stream<<
        timeinfo->tm_hour<<"::"<<
        timeinfo->tm_min<<"::"<<
        timeinfo->tm_sec<<" "<<
        Message;

    _file<<stream.str();
}