Archive for February 21st, 2007

Crash dump analysis case study (1)

Wednesday, February 21st, 2007

Consider 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 -