[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: [e-mail]
        ReportedBy: [e-mail]
    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)

[Bug 28737] shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?

http://bugs.winehq.org/show_bug.cgi?id=28737

Piotr Caban  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from Piotr Caban  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).

[Bug 28737] shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?

http://bugs.winehq.org/show_bug.cgi?id=28737

--- Comment #2 from Michael Mc Donnell  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.

[Bug 28737] shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?

http://bugs.winehq.org/show_bug.cgi?id=28737

--- Comment #3 from Dan Kegel  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?

[Bug 28737] shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?

http://bugs.winehq.org/show_bug.cgi?id=28737

--- Comment #4 from Austin English  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@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));

[Bug 28737] shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?

http://bugs.winehq.org/show_bug.cgi?id=28737

Michael Mc Donnell  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

[Bug 28737] shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?

http://bugs.winehq.org/show_bug.cgi?id=28737

Michael Mc Donnell  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #36987|0                           |1
        is obsolete|                            |

--- Comment #5 from Michael Mc Donnell  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.

[Bug 28737] shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?

http://bugs.winehq.org/show_bug.cgi?id=28737

--- Comment #6 from Michael Mc Donnell  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?

[Bug 28737] shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?

http://bugs.winehq.org/show_bug.cgi?id=28737

--- Comment #7 from Piotr Caban  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.

[Bug 28737] shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?

http://bugs.winehq.org/show_bug.cgi?id=28737

--- Comment #8 from Piotr Caban  2011-10-19 14:45:25 CDT ---
There should be 'change' instead 'check'.

[Bug 28737] shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?

http://bugs.winehq.org/show_bug.cgi?id=28737

--- Comment #9 from Michael Mc Donnell  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.

[Bug 28737] shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?

http://bugs.winehq.org/show_bug.cgi?id=28737

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |26f3c14d6b95e42b47b2e153be3
                   |                            |b6a26d64b87ed
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #10 from Austin English  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...

[Bug 28737] shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?

http://bugs.winehq.org/show_bug.cgi?id=28737

--- Comment #11 from Michael Mc Donnell  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.

[Bug 28737] shell32: invalid free in BrsFolderDlgProc in func_brsfolder in "make brsfolder.ok"?

http://bugs.winehq.org/show_bug.cgi?id=28737

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #12 from Alexandre Julliard  2011-10-21 13:50:55 CDT ---
Closing bugs fixed in 1.3.31.