Why does this code corrupt memory?

Question

This is a fairly new issue that should be accountable fairly quickly ...

Basically, after the first call to Printf in echo, the contents of args are corrupted. It seems to me that I am passing pointers incorrectly. But I can’t understand 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 a production code, just a proof of concept.
EDIT g_PlatformDevice is of type RiseWindows, if it is not clear ...
EDIT This is on a Windows XP platform running vs2008

UPDATE For everyone interested, the problem was apparently an overflowing call stack, further down the stack, then this other large array was defined. Refactoring eliminates memory corruption. So beat the chalk with a stack!

Answer 1

You did not indicate in which environment this code works. Perhaps you blew up your stack. You declare an array of 32767 bytes on the stack in RiseWindows :: Print. On some embedded system environments that I'm familiar with, this will be bad news. Can you increase the size of the stack and / or allocate this buffer on the heap just to test this theory? You might want to make this buffer std :: vector, or perhaps a private member vector, to avoid allocating and redistributing it each time you call Print.

Along these lines, how big is MAX_PRINT_OUTPUT?

Answer 2

This is probably not the error you are asking for, but in your loop you double the number of pBuffer, which may push you to the end of the buffer, because you only check for length-1 (for zero termination).

Answer 3

Have you tried the strategy of separation and conquest?

• Start commenting on lines until they work.
• Once it processes the lines correctly, until you hit the error.

Make sure that the memory indicated by args [] in a separate window while you do it step by step can also be useful.

Answer 4

Random assumption: there is a feeling that the problem is caused by this line in Printf:

 char formattedOutput[MAX_PRINT_OUTPUT]; 

The reason I think this is because you have some explicitly declared pointers and some explicitly undeclared pointers. An array of characters is a pointer, but this is not obvious. In the function definition, Echo args is defined as DUAL WEIGHT because you have it like

 *args[MAX_COMMAND_ARGS] 

Do you want it? I assume that something is inadvertently passed as a reference instead of a value, because the pointer against the array is indefinitely defined, and you are passing a pointer to a pointer, which is the beginning of the array, instead of a pointer, which is the beginning of the array. Since you said that it is damaged when you type RiseWindows :: Print, I assume that you are wrong.

Also, the const pointer to char only stores the value of the pointer, as far as I know, and not the value of the contents in the pointer.

Answer 5

Can I suggest going through the debugger to see where the code is corrupting?

Answer 6

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

change to:

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

you write 2 characters at a time, so you need to make sure that you have room for two characters.

Answer 7

not sure if it should work or not, but Echo doesn't print the first args element

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

Answer 8

You want to move #define from the function call to the beginning of the file:

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

This probably does not cause corruption in this case, but it is non-standard and can easily cause problems in the line.

Answer 9

The working theory is that we blew up the stack:

buffer

char [CONSOLE_OUTPUT_SIZE]; char * pBuffer = buffer;

Try instead:

char * pBuffer = new char [CONSOLE_OUTPUT_SIZE];

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

Answer 10

I really haven't researched this, but you have mixed types ... It's extremely pedantic, but it really matters in C.

Here you have one char array.

 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[]) 

In addition, I notice that you are changing memory in these buffers - are you sure you can do this? At least I'm sure you can't just use more without allocating more memory. (Tooltip hint!)

I would select my own array and copy the string into it. Then I would use string functions to replace new strings, if applicable.

Finally, I highly recommend you use std :: string here. (Although you will not be able to put them in varargs material - you will have to use c-lines for them, but copy them back to std :: string when you can).