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?

Never assume that example code is bug free. Be it from Microsoft or from someone else. By using example code, all bugs are automatically yours. — j6t, Feb 02 '17 at 17:31
I don't, hence this post, but I'd expect a better standard from Microsoft in the code examples that come with their API documentation, as this code is often taken as the idiomatic way in which the API should be used. As the other answers here illustrate, there is quite a lot that is wrong with this code. — SmacL, Feb 02 '17 at 18:45
This is not Windows API documentation. It's MFC, a library, that has always had documentation meeting lower standards than the Windows API documentation. Regardless, one would hope, that MFC documentation and sample code were correct as well. I'd file a defect report if I had any hope, that the input got incorporated. I'm not gullible enough to maintain that hope. — IInspectable, Feb 04 '17 at 09:58
Fair point, and possibly a bit naive of me to expect better, having had plenty of 'fun and games' with MFC over the years. — SmacL, Feb 04 '17 at 10:25

score 3 · Answer 1 · edited May 23 '17 at 12:01

3

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:^*

Replace wchar_t* pBufEnd = p + FILE_LIST_BUFFER_SIZE - 2; with const wchar_t* pBufEnd = fileName.GetString() + FILE_LIST_BUFFER_SIZE - 2;.
Replace wchar_t* start = p; with const wchar_t* start = fileName.GetString();
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.

edited May 23 '17 at 12:01

Community

1
1

answered Feb 02 '17 at 13:26

IInspectable

46,945
8
85
181

The whole pBufEnd thing is a nonsense too, given that CFileDialog provides the explicit methods 'GetStartPosition' and 'GetNextPathName' to extract names of selected files. The most important thing you need to do to fix the sample code is to move the fileName.ReleaseBuffer to the end of the code, to stop invalid memory access. Calling GetBuffer and ReleaseBuffer explicitly is also not exactly automatic memory management. – SmacL Feb 02 '17 at 13:45
@ShaneMacLaughlin: There are lots of fixes needed to make this code legal, yet moving the `ReleaseBuffer` call (while possible) is not a good solution. My answer outlined how to leave the `ReleaseBuffer` call where it is, and still use pointers into the controlled sequence. `GetBuffer`/`ReleaseBuffer` are artifacts necessary for interacting with a C library. In between those calls, memory management is indeed manual, but once `ReleaseBuffer` returns, you get all the automatic memory management back. There is merit in using `CString` but the sample gets it mostly wrong. – IInspectable Feb 02 '17 at 13:50
if you leave ReleaseBuffer where it is and then go on to use p, you are accessing memory that in theory could change in that according to the documentation it is no longer valid. While in practise it will only likely get deallocated by the the CString 'dtor or moved by changing the CString content, it still seems like a terrible way to approach this. – SmacL Feb 02 '17 at 14:10
1

@ShaneMacLaughlin: Correct. Accessing `p` after calling `ReleaseBuffer` is undefined. Updated my answer to fix the final remaining uses of `p`. – IInspectable Feb 02 '17 at 14:23

score 2 · Answer 2 · answered Feb 02 '17 at 13:26

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

Does this Microsoft CFileDialog example lead to a potential memory violation

2 Answers2