Buffer indexing overflow?

[masaeedu]

Admittedly I'm not super familiar with unmanaged code and C++/C, but it seems to me there is something wrong with this code in ConsoleRedir.cpp:

if(!ReadFile( pLP->pSettings->hStdErr, szBuffer, SIZEOF_BUFFER, &dwRead, &olR) || (dwRead == 0)) 
{
    ...
}
...
szBuffer[ dwRead / sizeof(szBuffer[0]) ] = _T('\0')

In C++, sizeof a char is always 1, so I don't understand what the point of the expression: sizeof(szBuffer[0]) is. That aside, since dwRead is the number of bytes you've read, you're going to be indexing 1 position past the end of the array when the buffer is full, which is unallocated memory.

I think you should be reading 1 less byte than the size of the buffer, i.e.:

if(!ReadFile( ..., SIZEOF_BUFFER - 1, ...) || (dwRead == 0)) 

[tmontane]

sizeof(char) is 1, but here it seems to read characters, not specifically char type, but could be wchar_t type (notice the _T macro used). To avoid having to write different piece of code to handle ANSI chars 8 bits and Unicode chars 16 bits, programmers use sizeof(buffer[0]) generally, and depending on your compiler option it can return 1 or 2.

For the rest (buffer overflow risk) I will need to check if the buffer was allocated with SIZEOF_BUFFER or SIZEOF_BUFFER +1. You may be right.

[poweradminllc]

I think you're right about the reading with SIZEOF_BUFFER - 1 or allocating with SIZEOF_BUFFER +1. Good catch.

[tmontane]

check line 219 & 297 in ConsoleRedir.cpp this seems weird compared to line 124. Also some buffers are already initialized with 0, some are not, there are some inconsistent coding style ... ;) and one (219 & 297) could lead to a buffer overflow, you are right.

[masaeedu]

@tmontane In my IDE, static inference suggests the type of the whole sizeof(szBuffer[0]) expression as (unsigned long)1UL, which is why I thought I'd bring up the issue with the division. Since the type of szBuffer is char[256], and a C++ char is always 1 byte, I don't see how anything but 1 byte could be stuffed into a single index of the array, unless there's some magic allocation going on somewhere else.

Maybe multi-byte chars are simply represented as multiple positions in the array? This would make sense, because I frequently see weird nonsense characters in my output where unicode characters should be appearing. However, this is unfamiliar territory for me so I defer to your expertise.

Re the SIZEOF_BUFFER thing, is reading SIZEOF_BUFFER - 1 chars in both places a proper fix?

[masaeedu]

According to this answer on Stack Overflow ReadFile by default simply tears Unicode wide chars into their constituent bytes, resulting in nonsense output. You need to invoke something called MultiByteToWideChar() to reconstitute the bytes of the unicode characters.