GetOpenFileName() crash - faulty OPENFILENAME string?

[Michel Rouzic]

Hi all,

In my program I call GetOpenFileName(), but often times it crashes
right when I call it. GDB reports it crashes lstrlenA(), which makes
me think that there must be something wrong with one of my strings in
my code.

Here is my code :

char *open_file_dialog(HINSTANCE hInst, HWND hwnd)
{
OPENFILENAME ofn;

#define DIALOGFILTER "All supported image formats (PNG, JPG, BMP,
GIF...)
\0.bmp;*cut;*dds;*doom;*exr;*hdr;*gif;*ico;*jp2;*jpg;*jpeg;*lbm;*mdl;*mng;*pal;*pbm;*pcd;*pcx;*pgm;*pic;*png;*ppm;*psd;*psp;*raw;*sgi;*tga;*tif;*tiff
\0All files (*.*)\0*.*\0"
char *diag_path_buffer;
char *diag_file_name;

diag_path_buffer = calloc (_MAX_PATH, sizeof(char));
diag_file_name = calloc (_MAX_FNAME + _MAX_EXT, sizeof(char));

// Details on the fields at http://msdn.microsoft.com/en-us/library/ms646839(VS.85).aspx
ofn.lStructSize = sizeof (OPENFILENAME);
ofn.hwndOwner = hwnd;
ofn.hInstance = hInst;
ofn.lpstrFilter = (LPTSTR) DIALOGFILTER;
ofn.lpstrCustomFilter = (LPTSTR) NULL;
ofn.nMaxCustFilter = 0L;
ofn.nFilterIndex = 1L;
ofn.lpstrFile = (LPTSTR) diag_path_buffer;
ofn.nMaxFile = _MAX_PATH;
ofn.lpstrFileTitle = diag_file_name;
ofn.nMaxFileTitle = _MAX_FNAME + _MAX_EXT;
ofn.lpstrInitialDir = NULL;
ofn.lpstrTitle = "Open";
ofn.nFileOffset = 0;
ofn.nFileExtension = 0;
ofn.lCustData = 0;
ofn.Flags = OFN_PATHMUSTEXIST | OFN_HIDEREADONLY |
OFN_FILEMUSTEXIST;

free(diag_file_name);

if (GetOpenFileName(&ofn))
return diag_path_buffer;
else
{
free (diag_path_buffer);
return NULL;
}
}

And here's GDB's backtrace :

#0 0x7c80c6fe in lstrlenA () from C:\WINDOWS\system32\kernel32.dll
#1 0x7c80c6e0 in SetHandleCount () from C:\WINDOWS
\system32\kernel32.dll
#2 0x7634684a in comdlg32!GetOpenFileNameW ()
from C:\WINDOWS\system32\comdlg32.dll
#3 0x7634313a in comdlg32!GetOpenFileNameW ()
from C:\WINDOWS\system32\comdlg32.dll
#4 0x004024ef in open_file_dialog (hInst=0x400000, hwnd=0xa078a) at
io.c:90
#5 0x004020fd in WinMain (hInst=0x400000, hPrev=0x0,
lpCmdLine=0x241f02 "",
nCmdShow=10) at main.c:269
#6 0x00402ab8 in main ()

It also may be relevant to note that I use the Allegro library's
<winalleg.h> in the place of <windows.h>, which is supposed to be the
same thing, but resolve a few conflicts between the Allegro library
and windows.h. But I doubt it's relevant.

Thanks a lot in advance.

[Grzegorz Wróbel]

Michel Rouzic wrote:
> Hi all,
>
> In my program I call GetOpenFileName(), but often times it crashes
> right when I call it. GDB reports it crashes lstrlenA(), which makes
> me think that there must be something wrong with one of my strings in
> my code.
>
> Here is my code :
>
> char *open_file_dialog(HINSTANCE hInst, HWND hwnd)
> {
> OPENFILENAME ofn;
>
> #define DIALOGFILTER "All supported image formats (PNG, JPG, BMP,
> GIF...)
> \0.bmp;*cut;*dds;*doom;*exr;*hdr;*gif;*ico;*jp2;*jpg;*jpeg;*lbm;*mdl;*mng;*pal;*pbm;*pcd;*pcx;*pgm;*pic;*png;*ppm;*psd;*psp;*raw;*sgi;*tga;*tif;*tiff
> \0All files (*.*)\0*.*\0"

You have a typo here in the line above which results in .bmp files not
being shown in the filter. You need *.bmp instead of .bmp

> char *diag_path_buffer;
> char *diag_file_name;
>
> diag_path_buffer = calloc (_MAX_PATH, sizeof(char));
> diag_file_name = calloc (_MAX_FNAME + _MAX_EXT, sizeof(char));
>
> // Details on the fields at http://msdn.microsoft.com/en-us/library/ms646839(VS.85).aspx

I would set entire ofn struct to 0 now, it's safer (in case if you
forgot to zero manually some field) and shorter (you don't need all
those zero assignments later), ie:

memset(&ofn,0,sizeof(ofn));

> ofn.lStructSize = sizeof (OPENFILENAME);
> ofn.hwndOwner = hwnd;
> ofn.hInstance = hInst;
> ofn.lpstrFilter = (LPTSTR) DIALOGFILTER;
> ofn.lpstrCustomFilter = (LPTSTR) NULL;
> ofn.nMaxCustFilter = 0L;
> ofn.nFilterIndex = 1L;

lpstrFile is used to initialize the File Name edit control, if you don't
want to initalize it to anything first characters of lpstrFile needs to
NULL (this might be causing the crash), ie:

diag_path_buffer[0]=0;

> ofn.lpstrFile = (LPTSTR) diag_path_buffer;
> ofn.nMaxFile = _MAX_PATH;
> ofn.lpstrFileTitle = diag_file_name;
> ofn.nMaxFileTitle = _MAX_FNAME + _MAX_EXT;
> ofn.lpstrInitialDir = NULL;
> ofn.lpstrTitle = "Open";
> ofn.nFileOffset = 0;
> ofn.nFileExtension = 0;
> ofn.lCustData = 0;
> ofn.Flags = OFN_PATHMUSTEXIST | OFN_HIDEREADONLY |
> OFN_FILEMUSTEXIST;
>
> free(diag_file_name);
>
> if (GetOpenFileName(&ofn))
> return diag_path_buffer;
> else
> {
> free (diag_path_buffer);
> return NULL;
> }
> }
>
> And here's GDB's backtrace :
>
> #0 0x7c80c6fe in lstrlenA () from C:\WINDOWS\system32\kernel32.dll
> #1 0x7c80c6e0 in SetHandleCount () from C:\WINDOWS
> \system32\kernel32.dll
> #2 0x7634684a in comdlg32!GetOpenFileNameW ()

Ermm, this isn't UNICODE app, is it?

> from C:\WINDOWS\system32\comdlg32.dll
> #3 0x7634313a in comdlg32!GetOpenFileNameW ()
> from C:\WINDOWS\system32\comdlg32.dll
> #4 0x004024ef in open_file_dialog (hInst=0x400000, hwnd=0xa078a) at
> io.c:90
> #5 0x004020fd in WinMain (hInst=0x400000, hPrev=0x0,
> lpCmdLine=0x241f02 "",
> nCmdShow=10) at main.c:269
> #6 0x00402ab8 in main ()
>

--
Grzegorz Wróbel
677265676F727940346E6575726F6E732E636F6D

[Alexander Grigoriev]

Make a habit NOT to use unnecessary casts. Instead, make sure you use
correct types. This will save your butt many times.

In your case, you casted the allocated buffer of MAX_PATH chars (bytes) to a
pointer to CHAR (wchar_t, because you compile it for UNICODE) and tell
GetOFN that it is MAX_PATH of wchar_t items big, which is twice as big as
actual buffer.

DROP ALL (LPTSTR) CASTS.

"Michel Rouzic" <Miche...@yahoo.fr> wrote in message
news:83182afe-ffab-477a...@2g2000hsn.googlegroups.com...

[Michel Rouzic]

Thanks a lot. I applied the modifications you suggested. I had no idea
about that whole UNICODE thing, so I changed every related string to
wchar_t * and forced UNICODE by defining _UNICODE, using a
OPENFILENAMEW struct instead of OPENFILENAME and directly calling
GetOpenFileNameW() rather than just GetOpenFileName(). I know it would
probably have been more desirable to use the Win API's types rather
than forcing the wchar_t format but considered the TCHAR type isn't
standard it's better for the rest of my code to be able to use
standard stuff without having to use more platform-specific code than
necessary.

Anyways, it now works great, thank you!

[Michel Rouzic]

Alexander Grigoriev wrote:
> Make a habit NOT to use unnecessary casts. Instead, make sure you use
> correct types. This will save your butt many times.
>
> In your case, you casted the allocated buffer of MAX_PATH chars (bytes) to a
> pointer to CHAR (wchar_t, because you compile it for UNICODE) and tell
> GetOFN that it is MAX_PATH of wchar_t items big, which is twice as big as
> actual buffer.
>
> DROP ALL (LPTSTR) CASTS.

I had no idea that UNICODE could be used there, and as for the casts
they came with the code I copied from the Web. As I detailed in my
reply to the other poster I forced the use of UNICODE on the whole
thing, made the appropriate modifications, the whole without any cast.
Thanks for the help!

[Chris Becke]

"Michel Rouzic" <Miche...@yahoo.fr> wrote in message news:1d52c66c-4da8-446b...@b1g2000hsg.googlegroups.com...

> Thanks a lot. I applied the modifications you suggested. I had no idea
> about that whole UNICODE thing, so I changed every related string to
> wchar_t * and forced UNICODE by defining _UNICODE, using a
> OPENFILENAMEW struct instead of OPENFILENAME and directly calling
> GetOpenFileNameW() rather than just GetOpenFileName(). I know it would
> probably have been more desirable to use the Win API's types rather
> than forcing the wchar_t format but considered the TCHAR type isn't
> standard it's better for the rest of my code to be able to use
> standard stuff without having to use more platform-specific code than
> necessary.

If you are this concerned about adhering to the c++ standard, you should know that the Windows API deals with 2 types of strings: CHAR and WCHAR. TCHAR is a synonym for one of these, depending on whether on not UNICODE is defined before <Windows.h> is. Its usually best to set UNICODE in your project settings so as to avoid having to ensure that its set in each cpp file that includes windows.h

The next point of interest, c++ standard related, is that wchar_t is NOT the same as a WCHAR. WCHAR is a windows API type, that windows API functions take, and it is 16bits in size. wchar_t is a c++ defined type, and as the c++ standard does not really set types, can be 16 or 32 bits on common compilers that target windows. As such dont use wchar_t to hold strings that must be passed to windows APIs. Use the windows type WCHAR or its string forms: LPWSTR and the const string LPCWSTR. Its actually more portable to use the platform specific types where needed.