Problems with print chat function?

[warz]

Do you think a print chat function would have a good chance of causing an overflow? My client is crashing when joining large channels, or typing /who to large channels. The only thing I can think of that's linked to both of these events is my print chat function.

Code Select Expand


void __cdecl AppendTextTS(HWND hRichEdit, COLORREF Color, char *szFmt, ...)
{
char szTxtToAppend[2048] = "";
char szTxtToAppend2[2048] = "";
va_list vaArg;
va_start(vaArg, szFmt);
vsprintf(szTxtToAppend, szFmt, vaArg);
va_end(vaArg);

SYSTEMTIME st;
GetLocalTime(&st);
sprintf(szTxtToAppend2, "[%02i:%02i:%02i] %s", st.wHour, st.wMinute, st.wSecond, szTxtToAppend);

CHARFORMAT cfFormat;
CHARRANGE CharRange = {-1, -1};
SendMessage(hRichEdit, EM_EXSETSEL, 0, (LPARAM)&CharRange);
CharFormat(Color, cfFormat);
SendMessage(hRichEdit, EM_SETCHARFORMAT, SCF_SELECTION, (LPARAM)&cfFormat);
SendMessage(hRichEdit, EM_REPLACESEL, FALSE, (LPARAM)szTxtToAppend2);
SendMessage(hRichEdit, EM_SCROLL, SB_LINEDOWN, 0);
}


You see anything wrong with this ? It's also very inefficeint.

[warz]

Was a rather ugly method of doing it originally. I switched it up to include _vsnprintf to avoid overflows.

Code Select Expand


void __cdecl AppendTextTS(HWND hRichEdit, COLORREF Color, char *szFmt, ...)
{
char buffer[328];
va_list args;

va_start(args, szFmt);
_vsnprintf(buffer, sizeof(buffer), szFmt, args);
va_end(args);

CHARFORMAT cfFormat;
CHARRANGE CharRange = {-1, -1};
SendMessage(hRichEdit, EM_EXSETSEL, 0, (LPARAM)&CharRange);
CharFormat(Color, cfFormat);
SendMessage(hRichEdit, EM_SETCHARFORMAT, SCF_SELECTION, (LPARAM)&cfFormat);
SendMessage(hRichEdit, EM_REPLACESEL, FALSE, (LPARAM)buffer);
SendMessage(hRichEdit, EM_SCROLL, SB_LINEDOWN, 0);
}


[Skywing]

Just a quick note that the above code still has a bug in that it needs to null terminate the buffer _vsnprintf'd too explicitly.

[warz]

Code Select Expand


void __cdecl AppendTextTS(HWND hRichEdit, COLORREF Color, char *szFmt, ...)
{
char buffer[328];
va_list args;

va_start(args, szFmt);
int written = _vsnprintf(buffer, sizeof(buffer) - 1, szFmt, args);
va_end(args);

if(written >= 0) {
buffer[written + 1] = '\0';
} else {
if(written == -1)
buffer[328] = '\0';
}

CHARFORMAT cfFormat;
CHARRANGE CharRange = {-1, -1};
SendMessage(hRichEdit, EM_EXSETSEL, 0, (LPARAM)&CharRange);
CharFormat(Color, cfFormat);
SendMessage(hRichEdit, EM_SETCHARFORMAT, SCF_SELECTION, (LPARAM)&cfFormat);
SendMessage(hRichEdit, EM_REPLACESEL, FALSE, (LPARAM)buffer);
SendMessage(hRichEdit, EM_SCROLL, SB_LINEDOWN, 0);
}


explicitly nulled now?

[Skywing]

No, there are still some bugs in your code.

The simplest way to do it is:

Code Select Expand

_vsnprintf(buffer, sizeof(buffer)-1, szFmt, args);
buffer[sizeof(buffer)-1] = '\0';

This will always null terminate the buffer properly.  Alternatively, you could fix the method you were trying to use, but I would use the simpler and easier to understand approach as any gain in efficiency would either be nonexistant or immeasurably small (and in fact might be slower if the processor misses a predicted jump and incurs a pipeline stall).

From the documentation of the function:

"Return Value
vsnprintf, _vsnprintf, and _vsnwprintf return the number of characters written if the number of characters to write is less than or equal to count; if the number of characters to write is greater than count, these functions return -1 indicating that output has been truncated. The return value does not include the terminating null, if one is written."

So, you could have 327 returned, which could cause you to write to buffer[328] which is beyond the scope of the array.  You also have an explicit write to buffer[328] if written is -1, which is also beyond the scope of the array.  Both of these could potentially cause subtle stack corruption issues that might not show up for some time before they cause your program to crash or behave in strange, unpredictable ways.

[warz]

Hm, yeah. I guess those were unnecessary steps. I also see what you're saying. Allocating a 328 byte array, but the array begins at 0, not 1. So buffer[328] is actually beyond that array. Rookie mistake.

Code Select Expand


void __cdecl AppendText(HWND hRichEdit, COLORREF Color, char *szFmt, ...)
{
char buffer[328];
va_list args;

va_start(args, szFmt);
_vsnprintf(buffer, sizeof(buffer) - 1, szFmt, args);
buffer[sizeof(buffer)-1] = '\0';
va_end(args);

CHARFORMAT cfFormat;
CHARRANGE CharRange = {-1, -1};
SendMessage(hRichEdit, EM_EXSETSEL, 0, (LPARAM)&CharRange);
CharFormat(Color, cfFormat);
SendMessage(hRichEdit, EM_SETCHARFORMAT, SCF_SELECTION, (LPARAM)&cfFormat);
SendMessage(hRichEdit, EM_REPLACESEL, FALSE, (LPARAM)buffer);
SendMessage(hRichEdit, EM_SCROLL, SB_LINEDOWN, 0);
}