/util/drmingw: b17553e9dc94: Various fixes to buffer interaction...
Daniel Atallah
datallah at pidgin.im
Mon Sep 10 12:11:13 EDT 2012
Changeset: b17553e9dc94207a1ed1fbd52dac2fd90de2e31e
Author: Daniel Atallah <datallah at pidgin.im>
Date: 2012-09-08 12:49 -0400
Branch: default
URL: http://hg.pidgin.im/util/drmingw/rev/b17553e9dc94
Description:
Various fixes to buffer interaction for safety against overruns and other related issues.
* Replace lstrcpyn with _tcsncpy (which isn't really any safer, but more consistently used and at the same time fix bounds checking)
* Use _vsntprintf instead of wvsprintf
* Avoid using lstrcpyn a specifically unsupported way (to copy intersecting strings)
* Handle GetModuleFileName potentially returning a unterminated buffer
diffstat:
exchndl.c | 102 ++++++++++++++++++++++++++++++++++++++++---------------------
1 files changed, 66 insertions(+), 36 deletions(-)
diffs (267 lines):
diff --git a/exchndl.c b/exchndl.c
--- a/exchndl.c
+++ b/exchndl.c
@@ -38,10 +38,12 @@ int __cdecl rprintf(const TCHAR * format
va_list argptr;
va_start(argptr, format);
- retValue = wvsprintf(szBuff, format, argptr);
+ retValue = _vsntprintf(szBuff, sizeof(szBuff) / sizeof(TCHAR) - 1, format, argptr);
+ szBuff[sizeof(szBuff) / sizeof(TCHAR) - 1] = _T('\0');
va_end(argptr);
- WriteFile(hReportFile, szBuff, retValue * sizeof(TCHAR), &cbWritten, 0);
+ if (retValue > 0)
+ WriteFile(hReportFile, szBuff, retValue * sizeof(TCHAR), &cbWritten, 0);
return retValue;
}
@@ -129,12 +131,12 @@ BOOL BfdDemangleSymName(LPCTSTR lpName,
if((res = cplus_demangle(lpName, DMGL_ANSI /*| DMGL_PARAMS*/)) == NULL)
{
- lstrcpyn(lpDemangledName, lpName, nSize);
return FALSE;
}
else
{
- lstrcpyn(lpDemangledName, res, nSize);
+ _tcsncpy(lpDemangledName, res, nSize);
+ lpDemangledName[nSize - 1] = _T('\0');
free (res);
return TRUE;
}
@@ -165,7 +167,8 @@ BOOL BfdGetSymFromAddr(bfd *abfd, asymbo
if(info.functionname == NULL || *info.functionname == '\0')
return FALSE;
- lstrcpyn(lpSymName, info.functionname, nSize);
+ _tcsncpy(lpSymName, info.functionname, nSize);
+ lpSymName[nSize - 1] = _T('\0');
return TRUE;
}
@@ -193,7 +196,8 @@ BOOL BfdGetLineFromAddr(bfd *abfd, asymb
assert(lpFileName && lpLineNumber);
- lstrcpyn(lpFileName, info.filename, nSize);
+ _tcsncpy(lpFileName, info.filename, nSize);
+ lpFileName[nSize - 1] = _T('\0');
*lpLineNumber = info.line;
return TRUE;
@@ -379,18 +383,20 @@ BOOL WINAPI j_SymGetLineFromAddr(HANDLE
static
BOOL ImagehlpDemangleSymName(LPCTSTR lpName, LPTSTR lpDemangledName, DWORD nSize)
{
- BYTE symbolBuffer[sizeof(IMAGEHLP_SYMBOL) + 512];
+ BYTE symbolBuffer[sizeof(IMAGEHLP_SYMBOL) + nSize];
PIMAGEHLP_SYMBOL pSymbol = (PIMAGEHLP_SYMBOL) symbolBuffer;
memset( symbolBuffer, 0, sizeof(symbolBuffer) );
pSymbol->SizeOfStruct = sizeof(symbolBuffer);
- pSymbol->MaxNameLength = 512;
+ pSymbol->MaxNameLength = nSize;
- lstrcpyn(pSymbol->Name, lpName, pSymbol->MaxNameLength);
+ _tcsncpy(pSymbol->Name, lpName, nSize);
+ pSymbol->Name[nSize - 1] = _T('\0');
if(!j_SymUnDName(pSymbol, lpDemangledName, nSize))
return FALSE;
+ lpDemangledName[nSize - 1] = _T('\0');
return TRUE;
}
@@ -406,19 +412,20 @@ BOOL ImagehlpGetSymFromAddr(HANDLE hProc
// to the buffer. We also need to initialize not one, but TWO
// members of the structure before it can be used.
- BYTE symbolBuffer[sizeof(IMAGEHLP_SYMBOL) + 512];
+ BYTE symbolBuffer[sizeof(IMAGEHLP_SYMBOL) + nSize];
PIMAGEHLP_SYMBOL pSymbol = (PIMAGEHLP_SYMBOL) symbolBuffer;
DWORD dwDisplacement = 0; // Displacement of the input address, relative to the start of the symbol
pSymbol->SizeOfStruct = sizeof(symbolBuffer);
- pSymbol->MaxNameLength = 512;
+ pSymbol->MaxNameLength = nSize;
assert(bSymInitialized);
if(!j_SymGetSymFromAddr(hProcess, dwAddress, &dwDisplacement, pSymbol))
return FALSE;
- lstrcpyn(lpSymName, pSymbol->Name, nSize);
+ _tcsncpy(lpSymName, pSymbol->Name, nSize);
+ lpSymName[nSize - 1] = _T('\0');
return TRUE;
}
@@ -459,10 +466,11 @@ BOOL ImagehlpGetLineFromAddr(HANDLE hPro
#endif
assert(lpFileName && lpLineNumber);
-
- lstrcpyn(lpFileName, Line.FileName, nSize);
+
+ _tcsncpy(lpFileName, Line.FileName, nSize);
+ lpFileName[nSize - 1] = _T('\0');
*lpLineNumber = Line.LineNumber;
-
+
return TRUE;
}
@@ -544,7 +552,7 @@ BOOL PEGetSymFromAddr(HANDLE hProcess, D
}
}
}
- }
+ }
if(!dwNearestAddress)
return FALSE;
@@ -599,7 +607,8 @@ BOOL WINAPI IntelStackWalk(
return FALSE;
}
- ReadMemoryRoutine(hProcess, (LPCVOID) (StackFrame->AddrFrame.Offset + 2*sizeof(DWORD)), StackFrame->Params, sizeof(StackFrame->Params), NULL);
+ if(!ReadMemoryRoutine(hProcess, (LPCVOID) (StackFrame->AddrFrame.Offset + 2*sizeof(DWORD)), StackFrame->Params, sizeof(StackFrame->Params), NULL))
+ return FALSE;
return TRUE;
}
@@ -645,6 +654,7 @@ BOOL StackBackTrace(HANDLE hProcess, HAN
BOOL bSuccess = FALSE;
HMODULE hPrevModule = hModule;
TCHAR szSymName[512] = _T("");
+ TCHAR szSymNameTmp[512] = _T("");
TCHAR szFileName[MAX_PATH] = _T("");
DWORD LineNumber = 0;
@@ -705,8 +715,10 @@ BOOL StackBackTrace(HANDLE hProcess, HAN
rprintf( _T("%08lX"), StackFrame.AddrPC.Offset);
- if((hModule = (HMODULE) GetModuleBase(StackFrame.AddrPC.Offset)) && GetModuleFileName(hModule, szModule, sizeof(szModule)))
+ if((hModule = (HMODULE) GetModuleBase(StackFrame.AddrPC.Offset))
+ && GetModuleFileName(hModule, szModule, sizeof(szModule) / sizeof(TCHAR)))
{
+ szModule[sizeof(szModule) / sizeof(TCHAR) - 1] = _T('\0');
#ifndef HAVE_BFD
rprintf( _T(" %s:ModulBase %08lX"), szModule, hModule);
#else /* HAVE_BFD */
@@ -750,30 +762,32 @@ BOOL StackBackTrace(HANDLE hProcess, HAN
}
if(!bSuccess && abfd && syms && symcount)
- if((bSuccess = BfdGetSymFromAddr(abfd, syms, symcount, StackFrame.AddrPC.Offset, szSymName, 512)))
+ if((bSuccess = BfdGetSymFromAddr(abfd, syms, symcount, StackFrame.AddrPC.Offset, szSymName, sizeof(szSymName) / sizeof(TCHAR))))
{
/*
framepointer = StackFrame.AddrFrame.Offset;
hprocess = hProcess;
*/
- BfdDemangleSymName(szSymName, szSymName, 512);
+ if (BfdDemangleSymName(szSymName, szSymNameTmp, sizeof(szSymNameTmp) / sizeof(TCHAR)))
+ rprintf( _T(" %s"), szSymNameTmp);
+ else
+ rprintf( _T(" %s"), szSymName);
- rprintf( _T(" %s"), szSymName);
-
- if(BfdGetLineFromAddr(abfd, syms, symcount, StackFrame.AddrPC.Offset, szFileName, MAX_PATH, &LineNumber))
+ if(BfdGetLineFromAddr(abfd, syms, symcount, StackFrame.AddrPC.Offset, szFileName, sizeof(szFileName) / sizeof(TCHAR), &LineNumber))
rprintf( _T(" %s:%ld"), szFileName, LineNumber);
}
#endif /* HAVE_BFD */
if(!bSuccess && bSymInitialized)
- if((bSuccess = ImagehlpGetSymFromAddr(hProcess, StackFrame.AddrPC.Offset, szSymName, 512)))
+ if((bSuccess = ImagehlpGetSymFromAddr(hProcess, StackFrame.AddrPC.Offset, szSymName, sizeof(szSymName) / sizeof(TCHAR))))
{
- rprintf( _T(" %s"), szSymName);
-
- ImagehlpDemangleSymName(szSymName, szSymName, 512);
+ if (ImagehlpDemangleSymName(szSymName, szSymNameTmp, sizeof(szSymNameTmp) / sizeof(TCHAR)))
+ rprintf( _T(" %s"), szSymNameTmp);
+ else
+ rprintf( _T(" %s"), szSymName);
- if(ImagehlpGetLineFromAddr(hProcess, StackFrame.AddrPC.Offset, szFileName, MAX_PATH, &LineNumber))
+ if(ImagehlpGetLineFromAddr(hProcess, StackFrame.AddrPC.Offset, szFileName, sizeof(szFileName) / sizeof(TCHAR), &LineNumber))
rprintf( _T(" %s:%ld"), szFileName, LineNumber);
}
@@ -859,7 +873,11 @@ void GenerateExceptionReport(PEXCEPTION_
}
// First print information about the type of fault
- rprintf(_T("%s caused "), GetModuleFileName(NULL, szModule, MAX_PATH) ? szModule : "Application");
+ if (GetModuleFileName(NULL, szModule, sizeof(szModule) / sizeof(TCHAR)))
+ szModule[sizeof(szModule) / sizeof(TCHAR) - 1] = _T('\0');
+ else
+ _tcscpy(szModule, _T("Application"));
+ rprintf(_T("%s caused "), szModule);
switch(pExceptionRecord->ExceptionCode)
{
case EXCEPTION_ACCESS_VIOLATION:
@@ -992,8 +1010,12 @@ void GenerateExceptionReport(PEXCEPTION_
// Now print information about where the fault occured
rprintf(_T(" at location %08x"), (DWORD) pExceptionRecord->ExceptionAddress);
- if((hModule = (HMODULE) GetModuleBase((DWORD) pExceptionRecord->ExceptionAddress)) && GetModuleFileName(hModule, szModule, sizeof(szModule)))
+ if((hModule = (HMODULE) GetModuleBase((DWORD) pExceptionRecord->ExceptionAddress))
+ && GetModuleFileName(hModule, szModule, sizeof(szModule) / sizeof(TCHAR)))
+ {
+ szModule[sizeof(szModule) / sizeof(TCHAR) - 1] = _T('\0');
rprintf(_T(" in module %s"), szModule);
+ }
// If the exception was an access violation, print out some additional information, to the error log and the debugger.
if(pExceptionRecord->ExceptionCode == EXCEPTION_ACCESS_VIOLATION && pExceptionRecord->NumberParameters >= 2)
@@ -1138,27 +1160,35 @@ static void OnStartup(void) __attribute_
void OnStartup(void)
{
+ int bufSize = sizeof(szLogFileName) / sizeof(TCHAR);
// Install the unhandled exception filter function
prevExceptionFilter = SetUnhandledExceptionFilter(TopLevelExceptionFilter);
// Figure out what the report file will be named, and store it away
- if(GetModuleFileName(NULL, szLogFileName, MAX_PATH))
+ if(GetModuleFileName(NULL, szLogFileName, bufSize))
{
LPTSTR lpszDot;
-
+ /* From the GetModuleFileName MSDN docs:
+ "Windows XP: The string is truncated to nSize characters and is not null-terminated."
+
+ This also helps for _tcsncpy and below (we can be sure that the string will end up
+ null-terminated by not letting it overwrite the last char in the buffer).
+ */
+ szLogFileName[bufSize - 1] = _T('\0');
+
// Look for the '.' before the "EXE" extension. Replace the extension
// with "RPT"
if((lpszDot = _tcsrchr(szLogFileName, _T('.'))))
{
lpszDot++; // Advance past the '.'
- _tcscpy(lpszDot, _T("RPT")); // "RPT" -> "Report"
+ _tcsncpy(lpszDot, _T("RPT"), bufSize - 1 - _tcslen(szLogFileName)); // "RPT" -> "Report"
}
else
- _tcscat(szLogFileName, _T(".RPT"));
+ _tcsncat(szLogFileName, _T(".RPT"), bufSize - 1 - _tcslen(szLogFileName));
}
- else if(GetWindowsDirectory(szLogFileName, MAX_PATH))
+ else if(GetWindowsDirectory(szLogFileName, bufSize))
{
- _tcscat(szLogFileName, _T("EXCHNDL.RPT"));
+ _tcsncat(szLogFileName, _T("\\EXCHNDL.RPT"), bufSize - 1 - _tcslen(szLogFileName));
}
}
More information about the Commits
mailing list