Archived

This topic is now archived and is closed to further replies.

PaulCesar

Weird Windows Problem (comdlg)

Recommended Posts

PaulCesar    524
Hello, On one of my programming jobs, There has been reported a strange bug on several systems, the problem is... im just not getting the bug to occour on my system. The bug is this, when the user clicks on the download button, a load dialog box pops up, but for some reason, I guess a few computers crash when the load dialog is called. (before the user even gets a chance to make his selection) This is really strange concidering they are WINDOWS COMMON DIALOGS. I cant seem to reproduce the bugs, and I was told that it occured on an XP and 2000 machine (I have personaly tested fine on my development machine, which is an XP) Here is the relevent code, if someone cares to help me out. Some of the filename referances are special global variables stored, written to by an individual thread (one thread at a time, mutex proof). The error was supposidly something along the lines of ''myprogram.exe has generated errors and will been closed by Windows. You will need to restart the program. An error log is being created.''. Here is the code: case 5: // Load File window OPENFILENAME fn; fn.lStructSize = sizeof(OPENFILENAME); fn.hwndOwner = hWnd; fn.lpstrTitle = "Select torrent to load"; fn.Flags = OFN_FILEMUSTEXIST; fn.lpstrInitialDir = NULL; // this is the starting directory fn.nFilterIndex = 1; fn.lpstrFilter = "BitTorrent File\0*.torrent"; fn.lpstrFile = tpfilename; fn.nMaxFile = sizeof(tpfilename); fn.lpstrFileTitle = tfilename; fn.nMaxFileTitle = sizeof(tfilename); // Open download window if (GetOpenFileName(&fn)!=0) // User has selected a filename & clicked OK { realGlobalFilename = new char[255]; strcpy((char *)realGlobalFilename, tpfilename); hLoadFileThread = CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE)LoadFileThread, 0, 0, NULL); delete realGlobalFilename; } else // User hasn''t selected a file (Clicked on cancel) { MessageBox(NULL, "No file selected", "No file selected", MB_OK|MB_ICONEXCLAMATION); } break;

Share this post


Link to post
Share on other sites
hplus0603    11356
I see three problems.

1) You should always zero out all structs you pass to windows before you fill them in. Thus, add a memset( &fn, 0, sizeof(fn) ) before you set lStructSize.

2) The name/pattern string should be terminated with TWO nul characters. The way you write it, it''s only guaranteed to have a single terminating zero. Add a \0 right before the closing quote (and the compiler will add the second). This is likely why it crashes when the dialog is shown.

3) After CreateThread(), it''s not guaranteed that the thread you created has yet read the global you use to communicate the data. Thus, after you delete it, the thread may read it, which will cause crashing. This is a serious race condition in your program. A slightly better alternative is to have the THREAD delete the global string when it''s done with it, rather than the creator. However, if the user selects two files in quick succession, it''s still possible to overwrite the global before the first thread gets around to reading it (albeit quite unlikely). Thus, it''s best to not use a global, but instead pass the string pointer in as argument to the thread, or as an alternative if you really want the global, protect it with a lock which the creator acquires, but the thread releases when it''s done with the value (note: this has to be a semaphore, critical section can''t be used like that).

Share this post


Link to post
Share on other sites
PaulCesar    524
Hello,

Sorry, I just got around to checking this post. Thanks alot for your helpfull advice, I made the corrections, and I guess Ill see if it works.

Richard

Share this post


Link to post
Share on other sites
Halo7    172
Have you looked into analyzing the crash dump? You could get the crash dump from a user, then use windbg to help locate where the problem lies.

Windows Debugging Tools

I don''t know if anyone else here has used it before, but it''s helped me at work several times.

Halo7

Share this post


Link to post
Share on other sites