Does this Microsoft CFileDialog example lead to a potential memory violation

Question

I've been experiencing a number of random crashes using the MFC CFileDialog class so I had a look at their example code from this page which reads as follows;

#define MAX_CFileDialog_FILE_COUNT 99
#define FILE_LIST_BUFFER_SIZE ((MAX_CFileDialog_FILE_COUNT * (MAX_PATH + 1)) + 1)

CString fileName;
wchar_t* p = fileName.GetBuffer( FILE_LIST_BUFFER_SIZE );
CFileDialog dlgFile(TRUE);
OPENFILENAME& ofn = dlgFile.GetOFN( );
ofn.Flags |= OFN_ALLOWMULTISELECT;
ofn.lpstrFile = p;
ofn.nMaxFile = FILE_LIST_BUFFER_SIZE;

dlgFile.DoModal();
fileName.ReleaseBuffer();  

wchar_t* pBufEnd = p + FILE_LIST_BUFFER_SIZE - 2;  
wchar_t* start = p;
while( ( p < pBufEnd ) && ( *p ) )
  p++;
if( p > start )
{
  _tprintf(_T("Path to folder where files were selected:  %s\r\n\r\n"), start );
  p++;

  int fileCount = 1;
  while( ( p < pBufEnd ) && ( *p ) )
  {
    start = p;
    while( ( p < pBufEnd ) && ( *p ) )
      p++;
    if( p > start )
      _tprintf(_T("%2d. %s\r\n"), fileCount, start );
    p++;
    fileCount++;
  }
}

By my reading of it, the statement fileName.ReleaseBuffer(); makes the memory pointed to in the buffer variable pinvalid, such that the remaining code is liable to experience memory violations. At the same time, I'd also assume that Microsoft would have checked such examples prior to publishing them. Am I missing something obvious here? Is there any reason for the use of a CString here over a simple new followed by a delete after the buffer is no longer required?

Answer 1

Sample code isn't formal documentation. This sample is wrong. The documentation is right:

The address returned by GetBuffer may not be valid after the call to ReleaseBuffer because additional CSimpleStringT operations can cause the CSimpleStringT buffer to be reallocated.

The sample uses CString (over raw pointers and manual memory management) for automatic memory management and exception safety. The latter is a lot harder to get right with manual memory management (although this sample doesn't get exception safety right, either).

If you want to fix the sample code to adhere to the contract, the following changes need to be made:^*

1. Replace wchar_t* pBufEnd = p + FILE_LIST_BUFFER_SIZE - 2; with const wchar_t* pBufEnd = fileName.GetString() + FILE_LIST_BUFFER_SIZE - 2;.
2. Replace wchar_t* start = p; with const wchar_t* start = fileName.GetString();
3. Replace all remaining occurrences of p in the code after the dialog invocation with a new variable, initialized as const wchar_t* current = fileName.GetString();).

This is a common error. Whenever a developer thinks they need a char* of sorts, they overlook that they need a const char* instead, which pretty much every string type supplies by means of a member function.

---

Note that there are other bugs in the sample code, that have not been explicitly addressed in this answer (like the mismatch of character types as explained in another answer).

---

^* A C++ implementation that retrieves the list of selected files can be found in this answer.

Answer 2

You might be noticing a difference between specification and implementation. The code above works because the CString implementation allows it, even though the CString specification bans it.

And to highlight the quality of the example: it mixes TCHAR and wchar_t. In tprintf("%s", start) the string start has to be a TCHAR* but the example uses wchar_t* start