avoiding FNERR_BUFFERTOOSMALL

Hi, in the GetOpenFileName, I want to allow multiple files selection:

Code:

UINT CALLBACK CWnd::OFNHookProc( HWND hDlg, UINT message, WPARAM wParam, LPARAM lParam )
{
	switch(message)
	{
		case WM_INITDIALOG:
        {
            SetProp(GetParent(hDlg), "OFN", (void *)lParam);
            break;
        }
		case WM_NOTIFY:
        {
            LPOFNOTIFY  lpof = reinterpret_cast<LPOFNOTIFY>(lParam);
            DWORD neededSize = 0;
            if(lpof->hdr.code == CDN_SELCHANGE)
            {
                neededSize = CommDlg_OpenSave_GetSpec(GetParent(hDlg), NULL, 0);
                neededSize += MAX_PATH;
                LPOPENFILENAME pOfn = reinterpret_cast<LPOPENFILENAME>(GetProp(GetParent(hDlg), "OFN"));
                if(pOfn->nMaxFile < neededSize)
                {
                   // need to put comments because it hangs and crush
                    /*LPTSTR lpsz = pOfn->lpstrFile;
                    if (lpsz)
                    {
                        HeapFree(GetProcessHeap(),0,lpsz);
                        lpsz = (TCHAR*)HeapAlloc(GetProcessHeap(),HEAP_ZERO_MEMORY,neededSize);
                        if (lpsz)
                        {
                            pOfn->lpstrFile = lpsz;
                            pOfn->nMaxFile = neededSize;
                        }
                    }*/
                }
            }
            break;
        }
		case WM_DESTROY:
        {
            RemoveProp(GetParent(hDlg), "OFN");
            break;
        }
    }
    return 0;
}

bool CWnd::OpenFileNames(HWND h,const TCHAR *filter,TCHAR *buffer,int buffer_size,int *nMorethenOne)
{
    OPENFILENAME ofn = {0};
    buffer[0] = 0;
	ofn.lStructSize = sizeof(OPENFILENAME);
	ofn.Flags = OFN_EXPLORER|OFN_FILEMUSTEXIST|OFN_HIDEREADONLY|OFN_ALLOWMULTISELECT|OFN_ENABLEHOOK;
	ofn.hwndOwner = h;
	ofn.lpstrFile = buffer;
	ofn.nFilterIndex = 0;
	ofn.lpstrFilter = filter;
	ofn.nMaxFile = buffer_size;
	ofn.lpfnHook = OFNHookProc;
	ofn.lCustData = reinterpret_cast<LPARAM>(&ofn);
	if (!GetOpenFileName(&ofn)) return false;
	*nMorethenOne = ofn.nFileExtension;
	return true;
}

// .. later
case IDM_SOMEMENUITEM:
        {
            TCHAR sz_FileNames[MAX_PATH];
            int off_set = 0,nFiles = 0;
            if (OpenFileNames(hwnd,MakeFilter().c_str(),sz_FileNames,sizeof(sz_FileNames)+1,&off_set))
            {
                if (!off_set)
                {
                    // We have more then 1 files selected
                    
                }
                else
                {
                    // use single file 
                }
                nFiles++;
            }
            break;
        }

it crushes when I choose one file
I'm trying to grow the max file size so the user can grab more files...any ideas?

> OpenFileNames(hwnd,MakeFilter().c_str(),sz_FileNam es,sizeof(sz_FileNames)+1
Well lying about buffer sizes can't be good for a start.

Also reading the manual (you did this right?)

Originally Posted by msdn

If the buffer is too small, the function returns FALSE and the CommDlgExtendedError function returns FNERR_BUFFERTOOSMALL. In this case, the first two bytes of the lpstrFile buffer contain the required size, in bytes or characters.

OPENFILENAME Structure (Windows)

If you get that error (specifically), you allocate the amount of space it says you need and you call it again.

Originally Posted by Salem

Also reading the manual (you did this right?)

OPENFILENAME Structure (Windows)

If you get that error (specifically), you allocate the amount of space it says you need and you call it again.

I can guess why, but surely nobody actually uses that behaviour in their software do they? I can't imagine anybody continuing to use a piece of software which always prompts you to select files twice

Code:

BOOL IsAddressInCurrentThreadStack(PVOID pData)
{
    NT_TIB* tib = (NT_TIB*)NtCurrentTeb();
    return ((pData < tib->StackBase) && (pData > tib->StackLimit));
}

UINT_PTR CALLBACK OFNHookProc(HWND hdlg, UINT uiMsg, WPARAM wParam, LPARAM lParam)
{
    // this isn't thread safe, 
    // but when are you gonna be displaying two open/save dialogs at the same time?
    static OPENFILENAME* pOfn = NULL;
    switch(uiMsg)
    {
        case WM_INITDIALOG:
        {
            pOfn = reinterpret_cast<OPENFILENAME*>(lParam);
        }
        break;
        case WM_NOTIFY:
        {
            NMHDR* pHdr = reinterpret_cast<NMHDR*>(lParam);
            switch(pHdr->code)
            {
                case CDN_SELCHANGE:
                {
                    HWND hRealDlg = GetParent(hdlg);
                    // +2 = one for slash, one for null
                    UINT lenReqd = CommDlg_OpenSave_GetFolderPath(hRealDlg, 0, NULL) + 2;
                    // +2 = one for intervening null, one for final null
                    lenReqd += CommDlg_OpenSave_GetFilePath(hRealDlg, 0, NULL) + 2;
                    if(lenReqd > pOfn->nMaxFile)
                    {
                        if(!IsAddressInCurrentThreadStack(pOfn->lpstrFile))
                        {
                            delete [] pOfn->lpstrFile;
                        }
                        pOfn->lpstrFile = new TCHAR[lenReqd];
                        pOfn->nMaxFile = lenReqd;
                    }
                }
                break;
            }
        }
        break;
    }
    return 0;
}

int main(int argc, wchar_t** argv)
{
    TCHAR stackBuffer[10] = {0}; // sized for illustration
    OPENFILENAME ofn = {sizeof(ofn), 0};
    ofn.Flags = OFN_EXPLORER | OFN_ALLOWMULTISELECT | OFN_ENABLEHOOK | OFN_FILEMUSTEXIST;
    ofn.lpfnHook = &OFNHookProc;
    ofn.lpstrFilter = L"All Files\0*.*\0";
    ofn.lpstrInitialDir = L"C:\\Test";
    ofn.lpstrFile = stackBuffer;
    ofn.nMaxFile = ARRAYSIZE(stackBuffer);
    DWORD ret = GetOpenFileName(&ofn);
    if(!ret)
    {
        ret = CommDlgExtendedError();
    }
    // use ofn.lpstrFile
    if(stackBuffer != ofn.lpstrFile)
    {
        delete [] ofn.lpstrFile;
    }
    return 0;
}

Hi adeyblue, I adopted your code with mine:

Code:

BOOL CWnd::IsAddressInCurrentThreadStack(PVOID pData)
{
    NT_TIB* tib = (NT_TIB*)NtCurrentTeb();
    return ((pData < tib->StackBase) && (pData > tib->StackLimit));
}

UINT CALLBACK CWnd::OFNHookProc( HWND hDlg, UINT message, WPARAM wParam, LPARAM lParam )
{
    static OPENFILENAME* pOfn = NULL;
	switch(message)
	{
		case WM_INITDIALOG:
        {
            pOfn = reinterpret_cast<OPENFILENAME*>(lParam);
            break;
        }
		case WM_NOTIFY:
        {
            NMHDR* pHdr = reinterpret_cast<NMHDR*>(lParam);
            switch(pHdr->code)
            {
                case CDN_SELCHANGE:
                {
                    HWND hRealDlg = GetParent(hDlg);
                    UINT lenReqd = CommDlg_OpenSave_GetFolderPath(hRealDlg, 0, 0) + 2;
                    lenReqd += CommDlg_OpenSave_GetFilePath(hRealDlg, 0, 0) + 2;
                    if(lenReqd > pOfn->nMaxFile)
                    {
                        if (!IsAddressInCurrentThreadStack(pOfn->lpstrFile))
                        {
                            delete [] pOfn->lpstrFile;
                        }
                        pOfn->lpstrFile = new TCHAR[lenReqd];
                        pOfn->nMaxFile = lenReqd;
                    }
                    break;
                }
            }
            break;
        }
    }
    return 0;
}

int CWnd::OpenFileNames(const TCHAR *filter)
{
    int retval = 0;
    OPENFILENAME ofn = {0};
    TCHAR buffer[MAX_PATH] = {0}; // sized for illustration
    buffer[0] = 0;
	ofn.lStructSize = sizeof(OPENFILENAME);
	ofn.Flags = OFN_EXPLORER|OFN_FILEMUSTEXIST|OFN_HIDEREADONLY|OFN_ALLOWMULTISELECT|OFN_ENABLEHOOK;
	ofn.hwndOwner = hwnd;
	ofn.lpstrFile = buffer;
	ofn.nFilterIndex = 0;
	ofn.lpstrFilter = filter;
	ofn.nMaxFile = MAX_PATH;
	ofn.lpfnHook = &OFNHookProc;
	ofn.lCustData = reinterpret_cast<LPARAM>(&ofn);
	if (GetOpenFileName(&ofn))
    {
        if (!ofn.nFileExtension)
        {
            // We have more then 1 file selected
            TCHAR dir[MAX_PATH];
            BOOL bRoot = FALSE;
            int iRoot = wsprintf(dir,"%s",ofn.lpstrFile);
            if (iRoot < 4) bRoot = TRUE;
            TCHAR *file = ofn.lpstrFile;
            file = file + strlen(file) + 1;
            while(*file != '\0')
            {
                TCHAR szFilePath[MAX_PATH];
                wsprintf(szFilePath,bRoot ? "%s%s" : "%s\\%s",dir,file);
                AddtoList(szFilePath);
                file = file + strlen(file) + 1;
            }
        }
        else
        {
            // single selection
            AddtoList(ofn.lpstrFile);
        }
        retval++;
    }
    DWORD dwError = CommDlgExtendedError();
	if (dwError)
    {
        TCHAR szError[MAX_PATH];
        wsprintf(szError,TEXT("Error code: %i"),dwError);
        MessageBox(hwnd,szError,TEXT("CommDlgExtendedError"),MB_OK|MB_ICONERROR);
    }
    if(buffer != ofn.lpstrFile)
    {
        delete [] ofn.lpstrFile;
    }
	return retval;
}

This are the results:

1.- No hang while selecting multiple files, that's a good news.
2.- I can choose a single file and it's added to my list.
3.- I can choose a few files and also added to the list.
4.- A very large amount of files returns 12291 (FNERR_BUFFERTOOSMALL)

What I'm doing wrong?

Code:

    TCHAR buffer[MAX_PATH] = {0}; // sized for illustration

MAX_PATH is defined as 260 in Windows.

Try a reasonable size... 2048 or 4096 if you're selecting multiple files.

> but surely nobody actually uses that behaviour in their software do they? I can't imagine anybody
> continuing to use a piece of software which always prompts you to select files twice
Did you actually try it, or did you presume the result?

I don't know (I'm not a windows programmer), but I figured those M$ folk would have figured out something. There are quite a few examples in the Win32 API which return this kind of result and give you another shot at trying the same call with a larger buffer.

But I do agree with you, if the dialog disappears when you get the insufficient buffer result, it would indeed suck.

Either that, or you just allocate some stupidly large buffer and hope for the best.

Originally Posted by Salem

> but surely nobody actually uses that behaviour in their software do they? I can't imagine anybody
> continuing to use a piece of software which always prompts you to select files twice
Did you actually try it, or did you presume the result?

I don't know (I'm not a windows programmer), but I figured those M$ folk would have figured out something. There are quite a few examples in the Win32 API which return this kind of result and give you another shot at trying the same call with a larger buffer.

But I do agree with you, if the dialog disappears when you get the insufficient buffer result, it would indeed suck.

Either that, or you just allocate some stupidly large buffer and hope for the best.

When you return from a multiple selection dialog box, you get a specially formatted string...

OPENFILENAME Structure (Windows)

If the OFN_ALLOWMULTISELECT flag is set and the user selects multiple files, the buffer contains the current directory followed by the file names of the selected files. For Explorer-style dialog boxes, the directory and file name strings are NULL separated, with an extra NULL character after the last file name.
...
If the buffer is too small, the function returns FALSE and the CommDlgExtendedError function returns FNERR_BUFFERTOOSMALL. In this case, the first two bytes of the lpstrFile buffer contain the required size, in bytes or characters.

So lets asume for a second that your user has a folder "D:\Freds Data\Older Pictures\2008\Kids photos\Cris" with 1000 files with names like "Christines Birthday Party 001.jpeg" This is not uncommon in todays "long filename" world... So buddy goes in there and selects ALL of them (with ctrl-A) and clicks "OK"... Rough calculation suggests this could easily hit 50 + ( 1000 * 40) = 40050 characters or ~80K bytes (if using unicode).

And... to make it even more of an eye opener... Folders with 5000+ files are not all that uncommon.

While it is true the ofnHook receives CDN_FOLDERCHANGE messages that could be used to scan the new folder and calculate buffer size by adding up the length of the filenames, you will run into the problem that realloc() or GlobalRealloc() will invalidate the pointer passed in when you opened the dialog box, causing a memory leak and since the struct is not updated internally before returning you end up writing to the original location causing buffer overflows.

So the solution is to "just allocate some stupidly large buffer and hope for the best" as Salem says.

Either that or disallow multiselect... it's rarely needed anyway.

Originally Posted by Salem

> but surely nobody actually uses that behaviour in their software do they? I can't imagine anybody
> continuing to use a piece of software which always prompts you to select files twice
Did you actually try it, or did you presume the result?

Educated guess, but a quick check and yeah, it does display the dialog twice.

Originally Posted by Joelito

What I'm doing wrong?

Turns out that CommDlg_OpenSave_GetFilePath only gets the file name of the hot item (that is, the item with the actual selection rectangle around it, and not every selected item). Swap it out for CommDlg_OpenSave_GetSpec, which gets the size of the text in the edit control, which handily lists all the file selections.

Originally Posted by CommonTater

you will run into the problem that realloc() or GlobalRealloc() will invalidate the pointer passed in when you opened the dialog box, causing a memory leak and since the struct is not updated internally before returning you end up writing to the original location causing buffer overflows.

Originally Posted by Salem

Did you actually try it or did you presume the result...

...You know, given there's an example of it working fine up there and everything

Code:

switch(pHdr->code)
            {
                case CDN_SELCHANGE:
                {
                    HWND hRealDlg = GetParent(hDlg);
                    UINT lenReqd = CommDlg_OpenSave_GetFolderPath(hRealDlg, 0, 0) + 2;
                    lenReqd += CommDlg_OpenSave_GetSpec(hRealDlg, 0, 0) + 2;
                    if(lenReqd > pOfn->nMaxFile)
                    {
                        if (!IsAddressInCurrentThreadStack(pOfn->lpstrFile))
                        {
                            delete [] pOfn->lpstrFile;
                        }
                        pOfn->lpstrFile = new TCHAR[lenReqd];
                        pOfn->nMaxFile = lenReqd;
                    }
                    break;
                }
            }

Same error