GetOpenFileName undefined behavior

[Ganado]

This is kind of a shot in the dark, but I was wondering if anyone could point out possible undefined behavior within or surrounding my open_file_dialog function.

Specifically, I think there might be undefined behavior related to the OPENFILENAME ofn object (its per-member assignments), or the call to GetOpenFileName.

#include <iostream>
#include <string>

#define UNICODE
#include <windows.h>

/// THIS FILE REQUIRES LINKING WITH
/// -lcomdlg32

/// MODIFIED FROM:
/// https://docs.microsoft.com/en-us/windows/desktop/dlgbox/using-common-dialog-boxes
bool open_file_dialog(HWND hwnd, std::wstring& file)
{
    OPENFILENAME ofn;       // common dialog box structure
    //char szFile[260];       // buffer for file name
    wchar_t szFile[260];
    //HWND hwnd;              // owner window
    //HANDLE hf;              // file handle

    // Initialize OPENFILENAME
    ZeroMemory(&ofn, sizeof(ofn));
    ofn.lStructSize = sizeof(ofn);
    ofn.hwndOwner = hwnd;
    ofn.lpstrFile = szFile;
    // Set lpstrFile[0] to '\0' so that GetOpenFileName does not
    // use the contents of szFile to initialize itself.
    ofn.lpstrFile[0] = '\0';
    ofn.nMaxFile = sizeof(szFile);
    ofn.lpstrFilter = L"All\0*.*\0Text\0*.TXT\0";
    ofn.nFilterIndex = 1;
    ofn.lpstrFileTitle = NULL;
    ofn.nMaxFileTitle = 0;
    ofn.lpstrInitialDir = NULL;
    ofn.Flags = OFN_PATHMUSTEXIST | OFN_FILEMUSTEXIST;

    // Display the Open dialog box.

    if (GetOpenFileName(&ofn) > 0)
    {
        // copy file name
        file = ofn.lpstrFile;
        return true;
    }
    else
    {
        return false;
    }
}


int main()
{
    std::wstring file;
    bool success = open_file_dialog(nullptr, file);
	
    if (success)
    {
        std::wcout << "Opening " << file << std::endl;
    }
}

I am suspecting there is some sort of undefined behavior, because in my full program that calls this open_file_dialog function, future file writes fail after GetOpenFileName succeeds (after open_file_dialog takes the "return true;" path). Furthermore, it's a bit weird because in my actual program, using std::cout with file (instead of std::wcout) bizarrely compiles, while this minimum example correctly would not. So I'm not perfectly reproducing my environment. Regardless, it is the same behavior if I comment out line 58.

Sadly I can't make a better minimum example right now, but perhaps in the future I'll try to. If you see any red flags in the code above, I would appreciate any thoughts.

Thanks!

I'm compiling with:

C:\dev\cplusplus\FileDialog>g++ -Wall main.cpp -lcomdlg32 -o main

In the future, I plan on using the "Common Item Dialog" (or perhaps just using C#)
https://msdn.microsoft.com/en-us/library/Bb776913(v=VS.85).aspx
However, I can't right now because my somewhat-old MinGW-64's shlwapi.h does not contain some of the necessary structs/functions. I believe this has been fixed in 2018+ versions of MinGW-64.
https://www.mail-archive.com/[e-mail]/msg14978.html

Edit: Debugging on my real program (with a few modifications) shows this:

> p ofn
$1 = {
  lStructSize = 152, 
  hwndOwner = 0x9716da, 
  hInstance = 0x0, 
  lpstrFilter = 0x58cd28 <_ZL10frame_size+2348> L"\101\154", <incomplete sequence \154>, 
  lpstrCustomFilter = 0x0, 
  nMaxCustFilter = 0, 
  nFilterIndex = 1, 
  lpstrFile = 0x22f270 L"", 
  nMaxFile = 260, 
  lpstrFileTitle = 0x0, 
  nMaxFileTitle = 0, 
  lpstrInitialDir = 0x0, 
  lpstrTitle = 0x0, 
  Flags = 6144, 
  nFileOffset = 0, 
  nFileExtension = 0, 
  lpstrDefExt = 0x0, 
  lCustData = 0, 
  lpfnHook = 0x0, 
  lpTemplateName = 0x0, 
  pvReserved = 0x0, 
  dwReserved = 0, 
  FlagsEx = 0
}

Just realized I'm assigning a string literal to a non-const pointer for the filter. Not sure if that's actually the issue, but it's certainly a possible point of contention.

Edit, doing:

    wchar_t filter[260] = L"All\0*.*\0Text\0*.TXT\0";
    ofn.lpstrFilter = filter;

does not change the behavior.
I think the issue might be that "incomplete sequence" part of the filter?

Edit, doing:

    std::wstring filter = L"All|*.*|Text|*.TXT|";
    for (wchar_t& ch : filter)
    {
        if (ch == L'|')
            ch = 0;
    }

    ofn.lpstrFilter = &filter[0]; //.c_str(); 

Also makes no difference.


I think I need to do more fine-grained debugging before coming to any conclusions here.

[coder777]

The type of lpstrFilter is LPCSTR. That means you may provide a const c-string.

To detect the problem see this:

https://docs.microsoft.com/en-us/windows/desktop/api/commdlg/nf-commdlg-getopenfilenamea

Use CommDlgExtendedError() to get more details about the error.

[Ganado]

Thanks for the reply. The issue is that CommDlgExtendedError, to my understanding, is only valid if the return value of GetOpenFileName was false (0).

It's possible that the problem might not be in this function, because the library does seem to produce the correct string inside of lpstrFile. The issue actually seems to depend on the file that I choose inside of the dialog (despite that fact that I don't actually do anything with this result).

I need to work on making a better minimal example to explain my issue. I will update this if I can make one.

[Ganado]

I have partly figured it out, after making a reduced example!

Long story short: It seems that part of GetOpenFileName changes the working directory of the program!
I did not see that it does this in the documentation... maybe there's still a bug, but at least I can work around it.

My saving logic relied on folders existing in a relative path, so after my call to GetOpenFileName, "./folder/file.png" (for example) was no longer a valid path.

It only changes the working path when GetOpenFileName succeeds.

int main()
{
	std::cout << "(1)\n";
	std::system("cd");
	
	std::wstring file;
	bool success = open_file_dialog(nullptr, file);
	
	std::cout << "(2)\n";
	std::system("cd");
	
	if (success)
	{
		std::wcout << "Opening " << file << std::endl;
		std::cout << "(3)\n";
		std::system("cd");
		
	}
	else
	{
		std::wcout << "File Dialog Cancelled\n";
	}
	
	std::vector<byte> pixels = {
		255, 0, 0, 255,    0, 255, 0, 255,
	     0, 0, 255, 255,   0, 0, 0, 255
	};
	
	std::cout << "(4)\n";
	std::system("cd");
	
	
	success = saveImageToFile("test.png", pixels, 2, 2);
	
	std::cout << "(5)\n";
	std::system("cd");
		
	
	if (!success)
	{
		std::cout << "Error: Could not save test.png\n";
	}
	else
	{
		std::cout << "test.png saved successfully\n";
	}
	
}

]C:\dev\cplusplus\FileDialog>main
(1)
C:\dev\cplusplus\FileDialog
(2)
C:\dev\cplusplus\FileDialog
File Dialog Cancelled
(4)
C:\dev\cplusplus\FileDialog
Saving test.png...
(5)
C:\dev\cplusplus\FileDialog
test.png saved successfully



C:\dev\cplusplus\FileDialog>main
(1)
C:\dev\cplusplus\FileDialog
(2)
C:\dev\cpp_hello  // WORKING DIRECTORY PATH CHANGED
Opening C:\dev\cpp_hello\hello.cpp
(3)
C:\dev\cpp_hello 
(4)
C:\dev\cpp_hello
Saving test.png...
(5)
C:\dev\cpp_hello
test.png saved successfully

C:\dev\cplusplus\FileDialog>

Thus, I predict that if I add a command after the call to GetOpenFileName to set my working directory back to what it originally was, all will be good. What a pain.

EDIT:
After knowing to search for "working directory", I found the true solution here:
https://stackoverflow.com/questions/41244561/why-the-current-directory-changes

there is an OFN_NOCHANGEDIR flag for that exact purpose:

Restores the current directory to its original value if the user changed the directory while searching for files.

Despite what the documentation claims, this flag is supported in GetOpenFileName().