Why does this code corrupt memory?

Question

This is a fairly newbie question which should be answerable reasonably quickly...

Basically, after the first call to Printf in echo, the contents of args is corrupted. It sounds to me like i'm passing the pointers around incorrectly. But can't figure out why?

#define MAX_PRINT_OUTPUT 4096

void Echo(char *args[MAX_COMMAND_ARGUMENTS], int argCount)
{
    for (int i = 1; i < argCount; ++i)
    {
     Printf("%s ", args[i]);
     Printf("\n");
    }
};

void Printf(const char *output, ...)
{
    va_list args;
    char formattedOutput[MAX_PRINT_OUTPUT];

    va_start(args, output);
    vsnprintf(formattedOutput, sizeof(formattedOutput), output, args);
    va_end(args);

    g_PlatformDevice->Print(formattedOutput);
};

void RiseWindows::Print(const char *output)
{
    //Corruption appears to occur as soon as this function is entered
    #define CONSOLE_OUTPUT_SIZE 32767

    char buffer[CONSOLE_OUTPUT_SIZE];
    char *pBuffer = buffer;
    const char *pOutput = output;
    int i = 0;

    while (pOutput[i] && ((pBuffer - buffer) < sizeof(buffer) - 1))
    {
     if (pOutput[i] == '\n' && pOutput[i+1] == '\r' )
     {
      pBuffer[0] = '\r';
      pBuffer[1] = '\n';
      pBuffer += 2;
      ++i;
     }
     else if (pOutput[i] == '\r')
     {
      pBuffer[0] = '\r';
      pBuffer[1] = '\n';
      pBuffer += 2;
     }
     else if (pOutput[i] == '\n')
     {
      pBuffer[0] = '\r';
      pBuffer[1] = '\n';
      pBuffer += 2;
     }
     else
     {
      *pBuffer = pOutput[i];
      ++pBuffer;
     }
     ++i;
    }
    *pBuffer = 0;

    SendMessage(this->ConsoleWindow.hwndBuffer, EM_LINESCROLL, 0, 0xffff);
    SendMessage(this->ConsoleWindow.hwndBuffer, EM_SCROLLCARET, 0, 0);
    SendMessage(this->ConsoleWindow.hwndBuffer, EM_REPLACESEL, 0, (LPARAM)buffer);

};

NOTE This is not production code, just proof of concept.
EDIT g_PlatformDevice is of type RiseWindows, if that wasn't clear...
EDIT This is on a windows xp platform running under vs2008

UPDATE For anyone interested, the problem appears to have been an overflowed call stack, further down the stack then this another large array was being defined. Refactoring this eliminated the memory corruption. So chalked up to stack battering!

Answer 1

Might I suggest stepping through with a debugger to see where the code corrupts?

Answer 2

Probably not the bug you're asking about, but in your loop you are double incrementing pBuffer in some cases, which could be pushing you over the end of buffer because you only check against length-1 (for null termination).

Answer 3

not sure if it's how it's supposed to work or not but Echo doesnt print out the first element of args

// Changed i=1 to i=0;
for (int i = 0; i < argCount; ++i)
{
    Printf("%s ", args[i]);
    Printf("\n");
}

Answer 4

while (pOutput[i] && ((pBuffer - buffer) < sizeof(buffer) - 1))

change to:

while (pOutput[i] && ((pBuffer - buffer) < sizeof(buffer) - 2))

you are writing 2 characters at a time so you need to make sure you have room for two characters.

Answer 5

You want to move your #define out of the function call to the top of the file:

Preprocessor directives can appear anywhere in a source file, but they apply only to the remainder of the source file.

This probably isn't causing the corruption in this case, but it's non-standard, and could easily cause problems down the line.

Answer 6

Did you try the divide and conquer strategy?

• Start commenting out lines until it work.
• Once it work correctly uncomment lines until you hit where the error is.

Watch the memory pointed by args[] in separate window while you do step by step also can be helpful.

Answer 7

Random guess: I get the feeling that the problem is caused by this line in Printf:

char formattedOutput[MAX_PRINT_OUTPUT];

The reason I think this is because you've got some obviously declared pointers and some obviously undeclared pointers. An array of chars is a pointer - no way around that but it's not obvious. In the function definition of Echo args is defined as a TWO DIMENSIONAL ARRAY because you have it as

*args[MAX_COMMAND_ARGS]

Do you want that? My guess is that something is unintentionally being passed as a reference instead of a value because what is a pointer vs. array is vaguely defined and you're passing a pointer to a pointer which is the start of an array instead of a pointer that is the start of an array. Since you said that it gets corrupted when you enter RiseWindows::Print my guess is that you're passing the wrong thing.

In addition, a const pointer to a char only preserves the value of the pointer as far as I know, not the value of the contents at the pointer.

Answer 8

You haven't mentioned what environment this code runs under. It could be you are blowing your stack. You are declaring a 32767 byte array on the stack in RiseWindows::Print. On some embedded system environments that I am familiar with that would be bad news. Can you increase your stack size and/or allocate that buffer on the heap just to test that theory? You may want to make that buffer a std::vector instead, or possibly a private member vector to avoid allocating and reallocating it every time you call Print.

Along those lines, how big is MAX_PRINT_OUTPUT?

Answer 9

The working theory is that we're blowing the stack with:

char buffer[CONSOLE_OUTPUT_SIZE]; char *pBuffer = buffer;

Instead try:

char *pBuffer = new char[CONSOLE_OUTPUT_SIZE];

And then remember to call delete [] pBuffer at the end.

Answer 10

I haven't really investigated this, but you've got your types mixed up... This is extremely pedantic, but it does make a difference in C.

Here, you have a single array of char.

char formattedOutput[MAX_PRINT_OUTPUT];

And here, you have a function that expects a const char pointer.

void RiseWindows::Print(const char *output)

Try:

void RiseWindows::Print(const char output[])

Additionally, I notice you are modifying the memory in those buffers - are you sure you can do that? At the very least, I'm certain that you can't just arbituarily use more without allocating more memory. (Hint hint!)

I would allocate my own array, and copy the string into it. I would then use string functions to replace the newlines as applicable.

Finally, I STRONGLY suggest that you use std::string here. (Though you won't be able to put those into the varargs stuff - you'll have to use c-strings for those, but copy those back into std::string's when you can).