From a88339c21968ba48d553f97195d4457c26a7a60c Mon Sep 17 00:00:00 2001 From: Ben Payne Date: Tue, 20 Jan 2015 16:21:56 -0500 Subject: [PATCH] Fix buffer overflows due to incorrect use of sizeof A snippet of example code: UTF16 pszFilter[1024]; ... convertUTF8toUTF16((UTF8 *)mData.mFilters, pszFilter, sizeof(pszFilter)); Since the conversion function is expecting the third parameter to be the length in 16-bit characters, *not* bytes, this results in the function writing outside the bounds of the output array. To make this less likely to happen in the future (I hope), I've provided a template function that infers the correct size of a static array, so it's no longer necessary to pass the size in most cases. The sized function has been renamed with an "N" suffix to hopefully encourage this use. This bug was caught due to a warning from MSVC about stack corruption occurring in codeBlock::exec(), after opening a file open dialog twice in succession. After some hunting, I found that this was due to FileDialog::Execute() passing incorrect buffer sizes to the conversion function, which resulted in the function writing a null terminator into some memory that happened to be in the stack frame of codeBlock::exec()! --- Engine/source/core/stringBuffer.cpp | 2 +- Engine/source/core/strings/unicode.cpp | 4 +- Engine/source/core/strings/unicode.h | 8 +++- Engine/source/gfx/gFont.cpp | 4 +- Engine/source/gfx/gfxDrawUtil.cpp | 2 +- .../nativeDialogs/fileDialog.cpp | 8 ++-- Engine/source/platformWin32/winConsole.cpp | 2 +- Engine/source/platformWin32/winExec.cpp | 10 ++--- Engine/source/platformWin32/winFileio.cpp | 40 +++++++++---------- Engine/source/platformWin32/winRedbook.cpp | 2 +- Engine/source/platformWin32/winWindow.cpp | 22 +++++----- 11 files changed, 55 insertions(+), 49 deletions(-) diff --git a/Engine/source/core/stringBuffer.cpp b/Engine/source/core/stringBuffer.cpp index ae95fc060..2f05d1973 100644 --- a/Engine/source/core/stringBuffer.cpp +++ b/Engine/source/core/stringBuffer.cpp @@ -141,7 +141,7 @@ void StringBuffer::set(const UTF8 *in) incRequestCount8(); // Convert and store. Note that a UTF16 version of the string cannot be longer. FrameTemp tmpBuff(dStrlen(in)+1); - if(!in || in[0] == 0 || !convertUTF8toUTF16(in, tmpBuff, dStrlen(in)+1)) + if(!in || in[0] == 0 || !convertUTF8toUTF16N(in, tmpBuff, dStrlen(in)+1)) { // Easy out, it's a blank string, or a bad string. mBuffer.clear(); diff --git a/Engine/source/core/strings/unicode.cpp b/Engine/source/core/strings/unicode.cpp index 0dee6f3ef..26bad63d2 100644 --- a/Engine/source/core/strings/unicode.cpp +++ b/Engine/source/core/strings/unicode.cpp @@ -146,7 +146,7 @@ inline bool isAboveBMP(U32 codepoint) } //----------------------------------------------------------------------------- -U32 convertUTF8toUTF16(const UTF8 *unistring, UTF16 *outbuffer, U32 len) +U32 convertUTF8toUTF16N(const UTF8 *unistring, UTF16 *outbuffer, U32 len) { AssertFatal(len >= 1, "Buffer for unicode conversion must be large enough to hold at least the null terminator."); PROFILE_SCOPE(convertUTF8toUTF16); @@ -252,7 +252,7 @@ UTF16* convertUTF8toUTF16( const UTF8* unistring) FrameTemp buf(len); // perform conversion - nCodepoints = convertUTF8toUTF16( unistring, buf, len); + nCodepoints = convertUTF8toUTF16N( unistring, buf, len); // add 1 for the NULL terminator the converter promises it included. nCodepoints++; diff --git a/Engine/source/core/strings/unicode.h b/Engine/source/core/strings/unicode.h index 042415377..9a0c634eb 100644 --- a/Engine/source/core/strings/unicode.h +++ b/Engine/source/core/strings/unicode.h @@ -79,10 +79,16 @@ UTF8* convertUTF16toUTF8( const UTF16 *unistring); /// - Output is null terminated. Be sure to provide 1 extra byte, U16 or U32 for /// the null terminator, or you will see truncated output. /// - If the provided buffer is too small, the output will be truncated. -U32 convertUTF8toUTF16(const UTF8 *unistring, UTF16 *outbuffer, U32 len); +U32 convertUTF8toUTF16N(const UTF8 *unistring, UTF16 *outbuffer, U32 len); U32 convertUTF16toUTF8( const UTF16 *unistring, UTF8 *outbuffer, U32 len); +template +inline U32 convertUTF8toUTF16(const UTF8 *unistring, UTF16 (&outbuffer)[N]) +{ + return convertUTF8toUTF16N(unistring, outbuffer, (U32) N); +} + //----------------------------------------------------------------------------- /// Functions that converts one unicode codepoint at a time /// - Since these functions are designed to be used in tight loops, they do not diff --git a/Engine/source/gfx/gFont.cpp b/Engine/source/gfx/gFont.cpp index 7c838915d..8d555955b 100644 --- a/Engine/source/gfx/gFont.cpp +++ b/Engine/source/gfx/gFont.cpp @@ -423,7 +423,7 @@ U32 GFont::getStrNWidth(const UTF8 *str, U32 n) { // UTF8 conversion is expensive. Avoid converting in a tight loop. FrameTemp str16(n + 1); - convertUTF8toUTF16(str, str16, n + 1); + convertUTF8toUTF16N(str, str16, n + 1); return getStrNWidth(str16, dStrlen(str16)); } @@ -462,7 +462,7 @@ U32 GFont::getStrNWidth(const UTF16 *str, U32 n) U32 GFont::getStrNWidthPrecise(const UTF8 *str, U32 n) { FrameTemp str16(n + 1); - convertUTF8toUTF16(str, str16, n + 1); + convertUTF8toUTF16N(str, str16, n + 1); return getStrNWidthPrecise(str16, dStrlen(str16)); } diff --git a/Engine/source/gfx/gfxDrawUtil.cpp b/Engine/source/gfx/gfxDrawUtil.cpp index 68a336aab..b04791d1c 100644 --- a/Engine/source/gfx/gfxDrawUtil.cpp +++ b/Engine/source/gfx/gfxDrawUtil.cpp @@ -149,7 +149,7 @@ U32 GFXDrawUtil::drawTextN( GFont *font, const Point2I &ptDraw, const UTF8 *in_s // Convert to UTF16 temporarily. n++; // space for null terminator FrameTemp ubuf( n * sizeof(UTF16) ); - convertUTF8toUTF16(in_string, ubuf, n); + convertUTF8toUTF16N(in_string, ubuf, n); return drawTextN( font, ptDraw, ubuf, n, colorTable, maxColorIndex, rot ); } diff --git a/Engine/source/platformWin32/nativeDialogs/fileDialog.cpp b/Engine/source/platformWin32/nativeDialogs/fileDialog.cpp index 5db038332..55e7912fc 100644 --- a/Engine/source/platformWin32/nativeDialogs/fileDialog.cpp +++ b/Engine/source/platformWin32/nativeDialogs/fileDialog.cpp @@ -324,10 +324,10 @@ bool FileDialog::Execute() UTF16 pszFileTitle[MAX_PATH]; UTF16 pszDefaultExtension[MAX_PATH]; // Convert parameters to UTF16*'s - convertUTF8toUTF16((UTF8 *)mData.mDefaultFile, pszFile, sizeof(pszFile)); - convertUTF8toUTF16((UTF8 *)mData.mDefaultPath, pszInitialDir, sizeof(pszInitialDir)); - convertUTF8toUTF16((UTF8 *)mData.mTitle, pszTitle, sizeof(pszTitle)); - convertUTF8toUTF16((UTF8 *)mData.mFilters, pszFilter, sizeof(pszFilter) ); + convertUTF8toUTF16((UTF8 *)mData.mDefaultFile, pszFile); + convertUTF8toUTF16((UTF8 *)mData.mDefaultPath, pszInitialDir); + convertUTF8toUTF16((UTF8 *)mData.mTitle, pszTitle); + convertUTF8toUTF16((UTF8 *)mData.mFilters, pszFilter); #else // Not Unicode, All char*'s! char pszFile[MAX_PATH]; diff --git a/Engine/source/platformWin32/winConsole.cpp b/Engine/source/platformWin32/winConsole.cpp index 0040c5677..cc92c2c15 100644 --- a/Engine/source/platformWin32/winConsole.cpp +++ b/Engine/source/platformWin32/winConsole.cpp @@ -67,7 +67,7 @@ void WinConsole::enable(bool enabled) { #ifdef UNICODE UTF16 buf[512]; - convertUTF8toUTF16((UTF8 *)title, buf, sizeof(buf)); + convertUTF8toUTF16((UTF8 *)title, buf); SetConsoleTitle(buf); #else SetConsoleTitle(title); diff --git a/Engine/source/platformWin32/winExec.cpp b/Engine/source/platformWin32/winExec.cpp index 67ef9d397..eb7a1928c 100644 --- a/Engine/source/platformWin32/winExec.cpp +++ b/Engine/source/platformWin32/winExec.cpp @@ -88,15 +88,15 @@ ExecuteThread::ExecuteThread(const char *executable, const char *args /* = NULL #ifdef UNICODE WCHAR exe[ 1024 ]; - convertUTF8toUTF16( exeBuf, exe, sizeof( exe ) / sizeof( exe[ 0 ] ) ); + convertUTF8toUTF16( exeBuf, exe ); TempAlloc< WCHAR > argsBuf( ( args ? dStrlen( args ) : 0 ) + 1 ); argsBuf[ argsBuf.size - 1 ] = 0; if( args ) - convertUTF8toUTF16( args, argsBuf, argsBuf.size ); + convertUTF8toUTF16N( args, argsBuf, argsBuf.size ); if( directory ) - convertUTF8toUTF16( directory, dirBuf, dirBuf.size ); + convertUTF8toUTF16N( directory, dirBuf, dirBuf.size ); #else char* exe = exeBuf; char* argsBuf = args; @@ -162,7 +162,7 @@ void Platform::openFolder(const char* path ) #ifdef UNICODE WCHAR p[ 1024 ]; - convertUTF8toUTF16( filePath, p, sizeof( p ) / sizeof( p[ 0 ] ) ); + convertUTF8toUTF16( filePath, p ); #else char* p = filePath; #endif @@ -179,7 +179,7 @@ void Platform::openFile(const char* path ) #ifdef UNICODE WCHAR p[ 1024 ]; - convertUTF8toUTF16( filePath, p, sizeof( p ) / sizeof( p[ 0 ] ) ); + convertUTF8toUTF16( filePath, p ); #else char* p = filePath; #endif diff --git a/Engine/source/platformWin32/winFileio.cpp b/Engine/source/platformWin32/winFileio.cpp index 7296ed32e..dff87f3b7 100644 --- a/Engine/source/platformWin32/winFileio.cpp +++ b/Engine/source/platformWin32/winFileio.cpp @@ -54,7 +54,7 @@ bool dFileDelete(const char * name) TempAlloc< TCHAR > buf( dStrlen( name ) + 1 ); #ifdef UNICODE - convertUTF8toUTF16( name, buf, buf.size ); + convertUTF8toUTF16N( name, buf, buf.size ); #else dStrcpy( buf, name ); #endif @@ -74,8 +74,8 @@ bool dFileRename(const char *oldName, const char *newName) TempAlloc< TCHAR > newf( dStrlen( newName ) + 1 ); #ifdef UNICODE - convertUTF8toUTF16( oldName, oldf, oldf.size ); - convertUTF8toUTF16( newName, newf, newf.size ); + convertUTF8toUTF16N( oldName, oldf, oldf.size ); + convertUTF8toUTF16N( newName, newf, newf.size ); #else dStrcpy(oldf, oldName); dStrcpy(newf, newName); @@ -93,7 +93,7 @@ bool dFileTouch(const char * name) TempAlloc< TCHAR > buf( dStrlen( name ) + 1 ); #ifdef UNICODE - convertUTF8toUTF16( name, buf, buf.size ); + convertUTF8toUTF16N( name, buf, buf.size ); #else dStrcpy( buf, name ); #endif @@ -119,8 +119,8 @@ bool dPathCopy(const char *fromName, const char *toName, bool nooverwrite) TempAlloc< TCHAR > to( dStrlen( toName ) + 1 ); #ifdef UNICODE - convertUTF8toUTF16( fromName, from, from.size ); - convertUTF8toUTF16( toName, to, to.size ); + convertUTF8toUTF16N( fromName, from, from.size ); + convertUTF8toUTF16N( toName, to, to.size ); #else dStrcpy( from, fromName ); dStrcpy( to, toName ); @@ -187,8 +187,8 @@ bool dPathCopy(const char *fromName, const char *toName, bool nooverwrite) backslash(toFile); #ifdef UNICODE - convertUTF8toUTF16( tempBuf, wtempBuf, wtempBuf.size ); - convertUTF8toUTF16( tempBuf1, wtempBuf1, wtempBuf1.size ); + convertUTF8toUTF16N( tempBuf, wtempBuf, wtempBuf.size ); + convertUTF8toUTF16N( tempBuf1, wtempBuf1, wtempBuf1.size ); WCHAR* f = wtempBuf1; WCHAR* t = wtempBuf; #else @@ -256,7 +256,7 @@ File::FileStatus File::open(const char *filename, const AccessMode openMode) TempAlloc< TCHAR > fname( dStrlen( filename ) + 1 ); #ifdef UNICODE - convertUTF8toUTF16( filename, fname, fname.size ); + convertUTF8toUTF16N( filename, fname, fname.size ); #else dStrcpy(fname, filename); #endif @@ -586,7 +586,7 @@ static bool recurseDumpPath(const char *path, const char *pattern, Vector searchBuf( buf.size ); - convertUTF8toUTF16( buf, searchBuf, searchBuf.size ); + convertUTF8toUTF16N( buf, searchBuf, searchBuf.size ); WCHAR* search = searchBuf; #else char *search = buf; @@ -665,7 +665,7 @@ bool Platform::getFileTimes(const char *filePath, FileTime *createTime, FileTime TempAlloc< TCHAR > fp( dStrlen( filePath ) + 1 ); #ifdef UNICODE - convertUTF8toUTF16( filePath, fp, fp.size ); + convertUTF8toUTF16N( filePath, fp, fp.size ); #else dStrcpy( fp, filePath ); #endif @@ -697,7 +697,7 @@ bool Platform::createPath(const char *file) #ifdef UNICODE TempAlloc< WCHAR > fileBuf( pathbuf.size ); - convertUTF8toUTF16( file, fileBuf, fileBuf.size ); + convertUTF8toUTF16N( file, fileBuf, fileBuf.size ); const WCHAR* fileName = fileBuf; const WCHAR* dir; #else @@ -820,7 +820,7 @@ bool Platform::setCurrentDirectory(StringTableEntry newDir) TempAlloc< TCHAR > buf( dStrlen( newDir ) + 2 ); #ifdef UNICODE - convertUTF8toUTF16( newDir, buf, buf.size - 1 ); + convertUTF8toUTF16N( newDir, buf, buf.size - 1 ); #else dStrcpy( buf, newDir ); #endif @@ -935,7 +935,7 @@ bool Platform::isFile(const char *pFilePath) TempAlloc< TCHAR > buf( dStrlen( pFilePath ) + 1 ); #ifdef UNICODE - convertUTF8toUTF16( pFilePath, buf, buf.size ); + convertUTF8toUTF16N( pFilePath, buf, buf.size ); #else dStrcpy( buf, pFilePath ); #endif @@ -974,7 +974,7 @@ S32 Platform::getFileSize(const char *pFilePath) TempAlloc< TCHAR > buf( dStrlen( pFilePath ) + 1 ); #ifdef UNICODE - convertUTF8toUTF16( pFilePath, buf, buf.size ); + convertUTF8toUTF16N( pFilePath, buf, buf.size ); #else dStrcpy( buf, pFilePath ); #endif @@ -1011,7 +1011,7 @@ bool Platform::isDirectory(const char *pDirPath) TempAlloc< TCHAR > buf( dStrlen( pDirPath ) + 1 ); #ifdef UNICODE - convertUTF8toUTF16( pDirPath, buf, buf.size ); + convertUTF8toUTF16N( pDirPath, buf, buf.size ); #else dStrcpy( buf, pDirPath ); #endif @@ -1057,8 +1057,8 @@ bool Platform::isSubDirectory(const char *pParent, const char *pDir) TempAlloc< TCHAR > dir( dStrlen( pDir ) + 1 ); #ifdef UNICODE - convertUTF8toUTF16( fileName, file, file.size ); - convertUTF8toUTF16( pDir, dir, dir.size ); + convertUTF8toUTF16N( fileName, file, file.size ); + convertUTF8toUTF16N( pDir, dir, dir.size ); #else dStrcpy( file, fileName ); dStrcpy( dir, pDir ); @@ -1251,7 +1251,7 @@ bool Platform::hasSubDirectory(const char *pPath) #ifdef UNICODE WCHAR buf[ 1024 ]; - convertUTF8toUTF16( searchBuf, buf, sizeof( buf ) / sizeof( buf[ 0 ] ) ); + convertUTF8toUTF16( searchBuf, buf ); WCHAR* search = buf; #else char* search = searchBuf; @@ -1335,7 +1335,7 @@ static bool recurseDumpDirectories(const char *basePath, const char *subPath, Ve #ifdef UNICODE TempAlloc< WCHAR > searchStr( dStrlen( search ) + 1 ); - convertUTF8toUTF16( search, searchStr, searchStr.size ); + convertUTF8toUTF16N( search, searchStr, searchStr.size ); #else char* searchStr = search; #endif diff --git a/Engine/source/platformWin32/winRedbook.cpp b/Engine/source/platformWin32/winRedbook.cpp index 38c5b3933..309f7513a 100644 --- a/Engine/source/platformWin32/winRedbook.cpp +++ b/Engine/source/platformWin32/winRedbook.cpp @@ -144,7 +144,7 @@ bool Win32RedBookDevice::open() openParms.lpstrDeviceType = (LPCWSTR)MCI_DEVTYPE_CD_AUDIO; UTF16 buf[512]; - convertUTF8toUTF16((UTF8 *)mDeviceName, buf, sizeof(buf)); + convertUTF8toUTF16((UTF8 *)mDeviceName, buf); openParms.lpstrElementName = buf; #else openParms.lpstrDeviceType = (LPCSTR)MCI_DEVTYPE_CD_AUDIO; diff --git a/Engine/source/platformWin32/winWindow.cpp b/Engine/source/platformWin32/winWindow.cpp index 45119dbb4..f8417f2f1 100644 --- a/Engine/source/platformWin32/winWindow.cpp +++ b/Engine/source/platformWin32/winWindow.cpp @@ -104,7 +104,7 @@ bool Platform::excludeOtherInstances(const char *mutexName) { #ifdef UNICODE UTF16 b[512]; - convertUTF8toUTF16((UTF8 *)mutexName, b, sizeof(b)); + convertUTF8toUTF16((UTF8 *)mutexName, b); gMutexHandle = CreateMutex(NULL, true, b); #else gMutexHandle = CreateMutex(NULL, true, mutexName); @@ -164,7 +164,7 @@ bool Platform::checkOtherInstances(const char *mutexName) #ifdef UNICODE UTF16 b[512]; - convertUTF8toUTF16((UTF8 *)mutexName, b, sizeof(b)); + convertUTF8toUTF16((UTF8 *)mutexName, b); pMutex = CreateMutex(NULL, true, b); #else pMutex = CreateMutex(NULL, true, mutexName); @@ -197,8 +197,8 @@ void Platform::AlertOK(const char *windowTitle, const char *message) ShowCursor(true); #ifdef UNICODE UTF16 m[1024], t[512]; - convertUTF8toUTF16((UTF8 *)windowTitle, t, sizeof(t)); - convertUTF8toUTF16((UTF8 *)message, m, sizeof(m)); + convertUTF8toUTF16((UTF8 *)windowTitle, t); + convertUTF8toUTF16((UTF8 *)message, m); MessageBox(NULL, m, t, MB_ICONINFORMATION | MB_SETFOREGROUND | MB_TASKMODAL | MB_OK); #else MessageBox(NULL, message, windowTitle, MB_ICONINFORMATION | MB_SETFOREGROUND | MB_TASKMODAL | MB_OK); @@ -211,8 +211,8 @@ bool Platform::AlertOKCancel(const char *windowTitle, const char *message) ShowCursor(true); #ifdef UNICODE UTF16 m[1024], t[512]; - convertUTF8toUTF16((UTF8 *)windowTitle, t, sizeof(t)); - convertUTF8toUTF16((UTF8 *)message, m, sizeof(m)); + convertUTF8toUTF16((UTF8 *)windowTitle, t); + convertUTF8toUTF16((UTF8 *)message, m); return MessageBox(NULL, m, t, MB_ICONINFORMATION | MB_SETFOREGROUND | MB_TASKMODAL | MB_OKCANCEL) == IDOK; #else return MessageBox(NULL, message, windowTitle, MB_ICONINFORMATION | MB_SETFOREGROUND | MB_TASKMODAL | MB_OKCANCEL) == IDOK; @@ -225,8 +225,8 @@ bool Platform::AlertRetry(const char *windowTitle, const char *message) ShowCursor(true); #ifdef UNICODE UTF16 m[1024], t[512]; - convertUTF8toUTF16((UTF8 *)windowTitle, t, sizeof(t)); - convertUTF8toUTF16((UTF8 *)message, m, sizeof(m)); + convertUTF8toUTF16((UTF8 *)windowTitle, t); + convertUTF8toUTF16((UTF8 *)message, m); return (MessageBox(NULL, m, t, MB_ICONINFORMATION | MB_SETFOREGROUND | MB_TASKMODAL | MB_RETRYCANCEL) == IDRETRY); #else return (MessageBox(NULL, message, windowTitle, MB_ICONINFORMATION | MB_SETFOREGROUND | MB_TASKMODAL | MB_RETRYCANCEL) == IDRETRY); @@ -241,8 +241,8 @@ Platform::ALERT_ASSERT_RESULT Platform::AlertAssert(const char *windowTitle, con #ifdef UNICODE UTF16 messageUTF[1024], title[512]; - convertUTF8toUTF16((UTF8 *)windowTitle, title, sizeof(title)); - convertUTF8toUTF16((UTF8 *)message, messageUTF, sizeof(messageUTF)); + convertUTF8toUTF16((UTF8 *)windowTitle, title); + convertUTF8toUTF16((UTF8 *)message, messageUTF); #else const char* messageUTF = message; const char* title = windowTitle; @@ -560,7 +560,7 @@ bool Platform::openWebBrowser( const char* webAddress ) #ifdef UNICODE dSprintf( buf, sizeof( buf ), "%s %s", utf8WebKey, webAddress ); UTF16 b[1024]; - convertUTF8toUTF16((UTF8 *)buf, b, sizeof(b)); + convertUTF8toUTF16((UTF8 *)buf, b); #else dSprintf( buf, sizeof( buf ), "%s %s", sWebKey, webAddress ); #endif