[Bug 28737] New: shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?
http://bugs.winehq.org/show_bug.cgi?id=28737 Bug #: 28737 Summary: shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"? Product: Wine Version: 1.3.30 Platform: x86 OS/Version: Linux Status: NEW Severity: normal Priority: P2 Component: shell32 AssignedTo: wine-bugs(a)winehq.org ReportedBy: dank(a)kegel.com Classification: Unclassified While running "make brsfolder.ok" in shell32/tests, valgrind complains: fixme:shell:BrsFolder_OnCreate flags BIF_NEWDIALOGSTYLE partially implemented Invalid free() / delete / delete[] at notify_free (heap.c:262) by RtlFreeHeap (heap.c:1748) by IMalloc_fnFree (ifs.c:262) by CoTaskMemFree (ifs.c:411) by SHFree (shellole.c:330) by BrsFolderDlgProc (brsfolder.c:462) by ??? (in dlls/user32/user32.dll.so) by call_dialog_proc (winproc.c:263) by WINPROC_CallDlgProcW (winproc.c:1045) by DefDlgProcW (defdlg.c:425) by ??? (in dlls/user32/user32.dll.so) by call_window_proc (winproc.c:242) by WINPROC_call_window (winproc.c:899) by call_window_proc (message.c:2211) by send_message (message.c:3084) by SendMessageW (message.c:3264) by TREEVIEW_SendTreeviewNotify (treeview.c:502) by TREEVIEW_SendExpanding (treeview.c:3213) by TREEVIEW_Expand (treeview.c:3374) by TREEVIEW_WindowProc (treeview.c:3549) by ??? (in dlls/user32/user32.dll.so) by call_window_proc (winproc.c:242) by WINPROC_call_window (winproc.c:899) by call_window_proc (message.c:2211) by send_message (message.c:3084) by SendMessageW (message.c:3264) by BrsFolderDlgProc (brsfolder.c:275) by ??? (in dlls/user32/user32.dll.so) by call_dialog_proc (winproc.c:263) by WINPROC_CallDlgProcW (winproc.c:1045) by DefDlgProcW (defdlg.c:425) by ??? (in dlls/user32/user32.dll.so) by call_window_proc (winproc.c:242) by WINPROC_call_window (winproc.c:899) by call_window_proc (message.c:2211) by send_message (message.c:3084) by SendMessageW (message.c:3264) by DIALOG_CreateIndirect (dialog.c:694) by DialogBoxParamW (dialog.c:870) by SHBrowseForFolderW (brsfolder.c:1129) by SHBrowseForFolderA (brsfolder.c:1092) by test_selection (brsfolder.c:344) by func_brsfolder (brsfolder.c:364) by run_test (test.h:556) by main (test.h:624) Address 0x7f012958 is 0 bytes inside a block of size 14 free'd at notify_free (heap.c:262) by RtlFreeHeap (heap.c:1748) by WineEngInit (freetype.c:1349) by DllMain (gdiobj.c:582) -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=28737 Piotr Caban <piotr.caban(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |piotr.caban(a)gmail.com --- Comment #1 from Piotr Caban <piotr.caban(a)gmail.com> 2011-10-18 08:58:34 CDT --- It happens because IEnumIDList_Next(lpe,1,&pidlTemp,&ulFetched): - return S_FALSE - sets pidlTemp to some incorrect pointer - sets ulFetched to 0 Probably it should not change pidlTemp value (but it needs some more testing). -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=28737 --- Comment #2 from Michael Mc Donnell <michael(a)mcdonnell.dk> 2011-10-18 14:47:41 CDT --- Created attachment 36987 --> http://bugs.winehq.org/attachment.cgi?id=36987 Patch that sets pointer to NULL after free to avoid double free This patch fixes the valgrind warning by setting the pointer to NULL in case of failure. I'm not going to send this to wine-patches because it really needs a test as Piotr Caban says. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=28737 --- Comment #3 from Dan Kegel <dank(a)kegel.com> 2011-10-18 14:58:31 CDT --- That patch has whitespace changes (so many it's hard to see the real change). Can you regenerate it without the whitespace changes? -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=28737 --- Comment #4 from Austin English <austinenglish(a)gmail.com> 2011-10-18 15:25:49 CDT --- (In reply to comment #3)
That patch has whitespace changes (so many it's hard to see the real change). Can you regenerate it without the whitespace changes?
austin(a)aw25 ~/wine-git/dlls/shell32 $ git diff -w . | more diff --git a/dlls/shell32/shfldr_unixfs.c b/dlls/shell32/shfldr_unixfs.c index 9649df8..ce30d52 100644 --- a/dlls/shell32/shfldr_unixfs.c +++ b/dlls/shell32/shfldr_unixfs.c @@ -2440,6 +2440,7 @@ static HRESULT WINAPI UnixSubFolderIterator_IEnumIDList_Next(IEnumIDList* iface, !UNIXFS_is_pidl_of_type(rgelt[i], This->m_fFilter)) { SHFree(rgelt[i]); + regelt[i] = NULL; continue; } memset(((PBYTE)rgelt[i])+rgelt[i]->mkid.cb, 0, sizeof(USHORT)); -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=28737 Michael Mc Donnell <michael(a)mcdonnell.dk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |michael(a)mcdonnell.dk -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=28737 Michael Mc Donnell <michael(a)mcdonnell.dk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36987|0 |1 is obsolete| | --- Comment #5 from Michael Mc Donnell <michael(a)mcdonnell.dk> 2011-10-19 14:19:49 CDT --- Created attachment 37016 --> http://bugs.winehq.org/attachment.cgi?id=37016 Patch that sets pointer to NULL after free to avoid double free Fixed white space changes. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=28737 --- Comment #6 from Michael Mc Donnell <michael(a)mcdonnell.dk> 2011-10-19 14:23:45 CDT --- (In reply to comment #2)
Created attachment 36987 [details] Patch that sets pointer to NULL after free to avoid double free
This patch fixes the valgrind warning by setting the pointer to NULL in case of failure. I'm not going to send this to wine-patches because it really needs a test as Piotr Caban says.
On second thought. I don't think this patch will hurt anything because the pointer it sets to NULL have already been set to a garbage value. A tests to show if the value is changed in the first place would however be nice. Any thoughts? -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=28737 --- Comment #7 from Piotr Caban <piotr.caban(a)gmail.com> 2011-10-19 14:44:22 CDT --- I don't think a test is needed for this (probably a correct implementation should not check the value that was there earlier). While I was writing that it needs some more testing, I meant I only spent few minutes on it. Not enough to find where the problem exactly is. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=28737 --- Comment #8 from Piotr Caban <piotr.caban(a)gmail.com> 2011-10-19 14:45:25 CDT --- There should be 'change' instead 'check'. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=28737 --- Comment #9 from Michael Mc Donnell <michael(a)mcdonnell.dk> 2011-10-20 10:50:22 CDT --- (In reply to comment #7)
I don't think a test is needed for this (probably a correct implementation should not check the value that was there earlier).
While I was writing that it needs some more testing, I meant I only spent few minutes on it. Not enough to find where the problem exactly is.
Oh ok. I'll send this patch then. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=28737 Austin English <austinenglish(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |26f3c14d6b95e42b47b2e153be3 | |b6a26d64b87ed Status|NEW |RESOLVED Resolution| |FIXED --- Comment #10 from Austin English <austinenglish(a)gmail.com> 2011-10-20 16:30:37 CDT --- (In reply to comment #9)
(In reply to comment #7)
I don't think a test is needed for this (probably a correct implementation should not check the value that was there earlier).
While I was writing that it needs some more testing, I meant I only spent few minutes on it. Not enough to find where the problem exactly is.
Oh ok. I'll send this patch then.
http://source.winehq.org/git/wine.git/commitdiff/26f3c14d6b95e42b47b2e153be3... -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=28737 --- Comment #11 from Michael Mc Donnell <michael(a)mcdonnell.dk> 2011-10-21 10:01:23 CDT --- (In reply to comment #10)
(In reply to comment #9)
(In reply to comment #7)
I don't think a test is needed for this (probably a correct implementation should not check the value that was there earlier).
While I was writing that it needs some more testing, I meant I only spent few minutes on it. Not enough to find where the problem exactly is.
Oh ok. I'll send this patch then.
http://source.winehq.org/git/wine.git/commitdiff/26f3c14d6b95e42b47b2e153be3...
Works for me. I don't get the valgrind warning any longer with the current tree. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=28737 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #12 from Alexandre Julliard <julliard(a)winehq.org> 2011-10-21 13:50:55 CDT --- Closing bugs fixed in 1.3.31. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
participants (1)
-
wine-bugs＠winehq.org