CodeSOD: Concrapenate Strings

As oft discussed, null-terminated C-style strings are an endless source of problems. But there's no problem so bad that it can't be made worse by a sufficiently motivated developer.

Today's rather old code comes from Mike, who inherited an old, MFC application. This code is responsible for opening a file dialog, and the key goal of the code is to configure the file filter in that dialog. In MFC, this is done by passing a delimited string containing a caption and a glob for filtering. E.g., "Text Files (.txt) | *.txt" would open a dialog for finding text files.

char                    szFileName[MAX_FILE_NAME];
char                    szDlgTitle[] = "Save File As";
char                    szDefExt[] = "log";
char*                   szPos;
WORD                    nFileOffset;
WORD                    nFileExtension;
char                    szFileFilter[MAX_FILE_NAME];
char                    szBuffer[128];
std::ifstream           inFile;

memset(szFileFilter, NULL, MAX_FILTER_SIZE);
memset(szFileName, NULL, MAX_FILE_NAME);

strcpy_s(szFileFilter, sizeof(szFileFilter), "*.log - Debug Log File|");
szPos = szFileFilter + strlen("*.log - Debug Log File|") + 1;
strcpy_s(szPos, sizeof( szPos), "*.log");

if(!OpenSaveFile(TRUE, szFileName, MAX_FILE_NAME, szFileFilter, szDlgTitle,
                                nFileOffset, nFileExtension, szDefExt, hWndInstance, hWndMain))
                                  break;

The impressive thing about this code is that this was released, sent to customers, and crashed consistently- until people started using the 64-bit build, when it started working again.

After declaring some variables, we start by using memset to null out some character arrays. This isn't particularly necessary, but it's mostly harmless- or at least it would be if they actually read their own code.

szFileFilter is declared using the size MAX_FILE_NAME, but when set to null, a space equal to MAX_FILTER_SIZE is used. If MAX_FILTER_SIZE is less than or equal to MAX_FILE_NAME this is fine- but if it's ever larger- welp, we've got an out of bounds access.

That's not what guarantees a crash, that's just bad practice.

Next, we use strcpy_s to copy the caption into our szFileFilter array. Then we calculate an offset within that array, to store in szPos. We then use strcpy_s again, copying our filter in to the end of the string.

This is the line that's guaranteed to crash. Because note that they pass a size to strcpy_s- sizeof(szPos). szPos is a pointer, not an array. So unlike all the other strings used in this example, sizeof won't tell you its length, it'll just tell you how much memory a pointer takes up. On 32-bit, that'd be 4 bytes. On 64-bit, that'd be 8. And that's why it suddenly started working when people changed builds.

Also, the ideal-world version of Hungarian notation is that, by specifying the types in the variable name, you can defend against these errors- but because they used sz for all strings, whether stored in allocated arrays or as pointers, they didn't have the information they needed.

Also, instead of doing all this weird string offset stuff with multiple copies, or doing any strcat_ss, they could have just…

strcpy_s(szFileFilter, sizeof(szFileFilter), "*.log - Debug Log File|*.log);

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.

This post originally appeared on The Daily WTF.