Crash Dump Analysis

CARE: Crash Analysis Report Environment

DATA (Dump Analysis + Trace Analysis) Facebook group
Please join the community of memory (dump) and trace analysis engineers. This group promotes scientific methods and memory dump-based worldview.

Twitter @ DumpAnalysis
You can now follow portal and blog news at DumpAnalysis on Twitter

LinkedIn Group Dr. Watson Enthusiasts
All about Dr. Watson errors and more. Get news, excerpts and progress reports about the forthcoming book The Science of Dr. Watson: An Illustrated History of Debugging (ISBN 978-1906717070)

2010 (0x7DA) - The Year of Dump Analysis
2011 (0x7DB) - 2020 (0x7E4) The Debugging Decade

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 -

           

Announcements

Coming Soon:

Debugging Notebook: Essential Concepts, WinDbg Commands and Tools

Crash Dump Analysis for System Administrators and Support Engineers

New Magazines:

Debugged! MZ/PE: MagaZine for/from Practicing Engineers

New Books:

Memory Dump Analysis Anthology, Volume 3

First Fault Software Problem Solving: A Guide for Engineers, Managers and Users

x64 Windows Debugging: Practical Foundations

Also available:

Windows Debugging: Practical Foundations

DLL List Landscape: The Art from Computer Memory Space

Dumps, Bugs and Debugging Forensics: The Adventures of Dr. Debugalov

WinDbg: A Reference Poster and Learning Cards

Memory Dump Analysis Anthology, Volume 2

Memory Dump Analysis Anthology, Volume 1

New Children's Book:

Baby Turing

This entry was posted on Wednesday, February 21st, 2007 at 1:50 am and is filed under Crash Dump Analysis, Debugging. You can follow any responses to this entry through the RSS 2.0 feed. You can leave a response, or trackback from your own site.