Crash dump analysis case study (1)
Wednesday, February 21st, 2007Consider the following legacy C++/Win32 code fragment highlighted in WinDbg after opening a crash dump:
1: HANDLE hFile = CreateFile(str.GetBuffer(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
2: if (hFile != INVALID_HANDLE_VALUE)
3: {
4: DWORD dwSize = GetFileSize(hFile, NULL);
5: DWORD dwRead = 0;
6: CHAR *bufferA = new CHAR[dwSize+2];
7: memset(bufferA, 0, dwSize+2);
8: if (ReadFile(hFile, bufferA, dwSize, &dwRead, NULL))
9: {
10: DWORD i = 0, j = 0;
11: for (; i < dwSize+2-7; ++i)
12: {
13: if (bufferA[i] == 0xD && bufferA[i+1] != 0xA)
At the first glance the code seems to be right: we open a file, get its size, allocate a buffer to read, etc. All loop indexes are within array bounds too. Let’s look at disassembly and crash point:
0:000> uf component!CMyDlg::OnTimer
…
…
…
004021bc push 0
004021be push esi
004021bf call dword ptr [component!_imp__GetFileSize (0042e26c)]
004021c5 mov edi,eax ; dwSize
004021c7 lea ebx,[edi+2] ; dwSize+2
004021ca push ebx
004021cb mov dword ptr [esp+34h],0
004021d3 call component!operator new[] (00408e35)
004021d8 push ebx
004021d9 mov ebp,eax ; bufferA
004021db push 0
004021dd push ebp
004021de call component!memset (00418500)
004021e3 add esp,10h
004021e6 push 0
004021e8 lea edx,[esp+34h]
004021ec push edx
004021ed push edi
004021ee push ebp
004021ef push esi
004021f0 call dword ptr [component!_imp__ReadFile (0042e264)]
004021f6 test eax,eax
004021f8 jne component!CMyDlg::OnTimer+0×3b1 (00402331)
…
…
…
00402331 xor esi,esi ; i
00402333 add edi,0FFFFFFFBh ; +2-7 (edi contains dwSize)
00402336 cmp edi,esi ; loop condition
00402338 mov dword ptr [esp+24h],esi
0040233c jbe component!CMyDlg::OnTimer+0×43e (004023be)
00402342 mov al,byte ptr [esi+ebp] ; bufferA[i]
0:000> r
eax=00002b00 ebx=00000002 ecx=00431000 edx=00000000 esi=00002b28 edi=fffffffb
eip=00402342 esp=0012efd4 ebp=0095b4d8 iopl=0 nv up ei pl nz ac pe cy
cs=001b ss=0023 ds=0023 es=0023 fs=003b gs=0000 efl=00000217
component!CMyDlg::OnTimer+0×3c2:
00402342 8a042e mov al,byte ptr [esi+ebp] ds:0023:0095e000=??
If we look at ebx (dwSize+2) and edi registers (array upper bound, dwSize+2-7) we can easily see that dwSize was zero. Clearly we had buffer overrun because upper array bound was calculated as 0+2-7 = FFFFFFFB (the loop index was unsigned integer, DWORD). Were the index signed integer variable (int) we wouldn’t have had any problem because the condition 0 < 0+2-7 is always false and the loop body would have never been executed.
Based on that the following fix was proposed:
1: HANDLE hFile = CreateFile(str.GetBuffer(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
2: if (hFile != INVALID_HANDLE_VALUE)
3: {
4: DWORD dwSize = GetFileSize(hFile, NULL);
5: DWORD dwRead = 0;
6: CHAR *bufferA = new CHAR[dwSize+2];
7: memset(bufferA, 0, dwSize+2);
8: if (ReadFile(hFile, bufferA, dwSize, &dwRead, NULL))
9: {
10: DWORD i = 0, j = 0;
10: int i = 0, j = 0;
11: for (; i < dwSize+2-7; ++i)
11: for (; i < (int)dwSize+2-7; ++i)
12: {
GetFileSize can return INVALID_FILE_SIZE (0xFFFFFFFF) and operator new can fail theoretically (if the size is too big) so we can correct the code even further:
1: HANDLE hFile = CreateFile(str.GetBuffer(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
2: if (hFile != INVALID_HANDLE_VALUE)
3: {
4: DWORD dwSize = GetFileSize(hFile, NULL);
4a: if (dwSize != INVALID_FILE_SIZE)
4b: {
5: DWORD dwRead = 0;
6: CHAR *bufferA = new CHAR[dwSize+2];
6a: if (bufferA)
6b: {
7: memset(bufferA, 0, dwSize+2);
8: if (ReadFile(hFile, bufferA, dwSize, &dwRead, NULL))
9: {
10: int i = 0, j = 0;
11: for (; i < (int)dwSize+2-7; ++i)
12: {
- Dmitry Vostokov @ DumpAnalysis.org -