[Bug 20553] New: Invalid read in LISTVIEW_NCDestroy in chromium unit_tests.exe in TableViewTest.Sort?
http://bugs.winehq.org/show_bug.cgi?id=20553
Summary: Invalid read in LISTVIEW_NCDestroy in chromium unit_tests.exe in TableViewTest.Sort? Product: Wine Version: 1.1.32 Platform: PC OS/Version: Linux Status: UNCONFIRMED Keywords: download Severity: normal Priority: P2 Component: comctl32 AssignedTo: wine-bugs@winehq.org ReportedBy: dank@kegel.com
Valgrinding the chrome unit tests yields the warning
Invalid read of size 4 at is_valid (imagelist.c:85) by ImageList_Destroy (imagelist.c:695) by LISTVIEW_NCDestroy (listview.c:9676) by LISTVIEW_WindowProc (listview.c:11017) by ??? (library.h:159) by call_window_proc (winproc.c:469) by CallWindowProcW (winproc.c:2321) by views::TableView::TableWndProc (table_view.cc:744) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_call_window (winproc.c:2214) by call_window_proc (message.c:1635) Address 0x7f052de8 is not stack'd, malloc'd or (recently) free'd
Line 9676 is ImageList_Destroy(infoPtr->himlSmall); Is himlSmall not properly initialized?
To repeat:
mkdir demo cd demo wget -c http://kegel.com/wine/chromium/chromium-tests.tar.bz2 tar -xjvf chromium-tests.tar.bz2 valgrind --trace-children=yes wine src/chrome/Debug/unit_tests.exe --gtest_filter=TableViewTest.Sort
(That download is a doozy, sorry...)
It's possible this is a chromium bug, but we've been running purify on the chromium tests for a while, and they're probably fairly clean.
http://bugs.winehq.org/show_bug.cgi?id=20553
--- Comment #1 from Nikolay Sivov bunglehead@gmail.com 2009-11-02 10:25:50 --- Hi, Dan.
Does your test free imagelists before ListView destruction? How LVS_SHAREIMAGELISTS is used?
The cause here is a dereference to himl->magic (or whatever it's called), himl isn't null obviously, but is in freed memory block.
(I'm not able to download it currently, it's to fat for my connection)
http://bugs.winehq.org/show_bug.cgi?id=20553
Dan Kegel dank@kegel.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |NEW Ever Confirmed|0 |1
--- Comment #2 from Dan Kegel dank@kegel.com 2009-11-12 00:02:03 --- Saw this in a much simpler testcase, NullModelTableViewTest.NullModel.
I'll attach the output of
WINEDEBUG=warn+heap,+message,+relay,+comctrl ~/wine-git/wine src/chrome/Debug/unit_tests.exe --gtest_filter=NullModelTableViewTest.NullModel
with the line
static inline BOOL is_valid(HIMAGELIST himl) { + ERR("is_valid: %p, magic %x\n", himl, himl ? himl->magic : -1);
added to imagelist.c.
This shows the problem even without valgrind. Here's the interesting bit:
$ egrep -i 'err:|comctl|ExitProcess' log ... 0028:Call comctl32.ImageList_Create(00000010,00000010,00000021,00000000,00000020) ret=7e1a2653 0028:Ret comctl32.ImageList_Create() retval=00167e40 ret=7e1a2653 0028:Call comctl32.ImageList_Create(00000020,00000020,00000021,00000000,00000020) ret=7e1a268a 0028:Ret comctl32.ImageList_Create() retval=00168860 ret=7e1a268a 0028:Call comctl32.ImageList_SetBkColor(00167e40,ffffffff) ret=7e1a26af err:imagelist:is_valid is_valid: 0x167e40, magic 53414d58 0028:Ret comctl32.ImageList_SetBkColor() retval=ffffffff ret=7e1a26af 0028:Call comctl32.ImageList_SetBkColor(00168860,ffffffff) ret=7e1a26ca err:imagelist:is_valid is_valid: 0x168860, magic 53414d58 0028:Ret comctl32.ImageList_SetBkColor() retval=ffffffff ret=7e1a26ca 0028:Call comctl32.334(00167c28,00007fff,00169290) ret=7e1a2000 0028:Ret comctl32.334() retval=00000000 ret=7e1a2000 0028:Call comctl32.ImageList_ReplaceIcon(00167e40,ffffffff,00001106) ret=7e1a2066 err:imagelist:is_valid is_valid: 0x167e40, magic 53414d58 0028:Ret comctl32.ImageList_ReplaceIcon() retval=00000000 ret=7e1a2066 0028:Call comctl32.ImageList_ReplaceIcon(00168860,ffffffff,0000110e) ret=7e1a208b err:imagelist:is_valid is_valid: 0x168860, magic 53414d58 0028:Ret comctl32.ImageList_ReplaceIcon() retval=00000000 ret=7e1a208b 0028:Call comctl32.334(00167c28,00007fff,001692f8) ret=7e1a2000 0028:Ret comctl32.334() retval=00000001 ret=7e1a2000 0028:Call comctl32.ImageList_ReplaceIcon(00167e40,ffffffff,00001106) ret=7e1a2066 err:imagelist:is_valid is_valid: 0x167e40, magic 53414d58 0028:Ret comctl32.ImageList_ReplaceIcon() retval=00000001 ret=7e1a2066 0028:Call comctl32.ImageList_ReplaceIcon(00168860,ffffffff,0000110e) ret=7e1a208b err:imagelist:is_valid is_valid: 0x168860, magic 53414d58 0028:Ret comctl32.ImageList_ReplaceIcon() retval=00000001 ret=7e1a208b err:imm:ImmAssociateContextEx Unknown dwFlags 0x0 0028:Call comctl32.ImageList_Create(00000012,00000012,00000020,00000002,00000002) ret=018b74b5 0028:Ret comctl32.ImageList_Create() retval=00178f90 ret=018b74b5 0028:Call comctl32.ImageList_ReplaceIcon(00178f90,ffffffff,0000114e) ret=018b7529 err:imagelist:is_valid is_valid: 0x178f90, magic 53414d58 0028:Ret comctl32.ImageList_ReplaceIcon() retval=00000000 ret=018b7529 0028:Call comctl32.ImageList_ReplaceIcon(00178f90,ffffffff,0000114e) ret=018b7548 err:imagelist:is_valid is_valid: 0x178f90, magic 53414d58 0028:Ret comctl32.ImageList_ReplaceIcon() retval=00000001 ret=018b7548 err:imagelist:is_valid is_valid: 0x178f90, magic 53414d58 err:imm:ImmAssociateContextEx Unknown dwFlags 0x0 0028:Call comctl32.ImageList_Destroy(00178f90) ret=018b843b err:imagelist:is_valid is_valid: 0x178f90, magic 53414d58 0028:Ret comctl32.ImageList_Destroy() retval=00000001 ret=018b843b err:imagelist:is_valid is_valid: (nil), magic ffffffff err:imagelist:is_valid is_valid: 0x178f90, magic aaaaaaaa <---- bammo! err:imagelist:is_valid is_valid: (nil), magic ffffffff 0028:Call KERNEL32.ExitProcess(00000000) ret=009fefda
I don't think LVS_SHAREIMAGELISTS is used? You can see the code at
http://src.chromium.org/viewvc/chrome/trunk/src/views/controls/table/table_v... http://src.chromium.org/viewvc/chrome/trunk/src/views/controls/table/table_v...
http://bugs.winehq.org/show_bug.cgi?id=20553
--- Comment #3 from Dan Kegel dank@kegel.com 2009-11-12 00:03:15 --- Created an attachment (id=24688) --> (http://bugs.winehq.org/attachment.cgi?id=24688) rzipped log; decompress with rzip -d
http://bugs.winehq.org/show_bug.cgi?id=20553
--- Comment #4 from Dan Kegel dank@kegel.com 2009-11-13 18:26:19 --- yum. Compiled wine with -O0, now unit_tests.exe crashes in this area after the invalid reads with
Jump to the invalid address stated on the next line at 0x0: ??? by LISTVIEW_NCDestroy (listview.c:9863) by LISTVIEW_WindowProc (listview.c:11204) by ??? (library.h:159) by call_window_proc (winproc.c:469) by CallWindowProcW (winproc.c:2321) by views::TableView::TableWndProc (table_view.cc:744) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_call_window (winproc.c:2214) by call_window_proc (message.c:1635) by send_message (message.c:2482) by SendMessageW (message.c:2605) by WIN_DestroyWindow (win.c:759) by WIN_DestroyWindow (win.c:741) by WIN_DestroyWindow (win.c:741) by DestroyWindow (win.c:1704) by views::WidgetWin::CloseNow (widget_win.cc:175)
http://bugs.winehq.org/show_bug.cgi?id=20553
--- Comment #5 from Dan Kegel dank@kegel.com 2009-11-13 18:28:17 --- Oh, that crash was in test NullModelTableViewTest.NullModel, btw. Guess I'll disable the affected tests until this is sorted; still no idea whether this is a wine or a chromium problem.
http://bugs.winehq.org/show_bug.cgi?id=20553
--- Comment #6 from Nikolay Sivov bunglehead@gmail.com 2009-11-13 21:37:17 --- So it looks like a problem here:
--- if (!(infoPtr->dwStyle & LVS_SHAREIMAGELISTS)) { ImageList_Destroy(infoPtr->himlNormal); ImageList_Destroy(infoPtr->himlSmall); <- ImageList_Destroy(infoPtr->himlState); } ---
Looking at chromium I see the following:
--- void TableView::OnDestroy() { if (table_type_ == ICON_AND_TEXT) { HIMAGELIST image_list = ListView_GetImageList(GetNativeControlHWND(), LVSIL_SMALL); DCHECK(image_list); if (image_list) ImageList_Destroy(image_list); } } ---
Such thing could certainly crash on Wine - you attach an imagelist to ListView without LVS_SHAREIMAGELISTS (at least I don't see it in cc file). After that you free imagelist and ListView tries to free it again on WM_NCDESTROY.
Thing you should to test: - what ImageList_Destroy() does for obviously invalid pointer passed, maybe we just should protect it with some exception handler to check if a whole structure size is valid starting HIMAGELIST pointer.
Actually I think it's a chromium bug (or not clean use) too. MSDN says: --- LVM_SETIMAGELIST --- The current image list will be destroyed when the list-view control is destroyed unless the LVS_SHAREIMAGELISTS style is set. If you use this message to replace one image list with another, your application must explicitly destroy all image lists other than the current one. ---
http://bugs.winehq.org/show_bug.cgi?id=20553
--- Comment #7 from Dan Kegel dank@kegel.com 2009-11-13 22:58:30 --- Thanks for the analysis! I see this is not the first time this has come up; see http://www.mail-archive.com/wine-devel@winehq.com/msg15702.html
(I'll close this as invalid once I verify the fix, since it looks like an app error, but that doesn't mean we shouldn't harden our code if we can.)
http://bugs.winehq.org/show_bug.cgi?id=20553
--- Comment #8 from Nikolay Sivov bunglehead@gmail.com 2009-11-14 10:08:54 --- (In reply to comment #7)
Thanks for the analysis! I see this is not the first time this has come up; see http://www.mail-archive.com/wine-devel@winehq.com/msg15702.html
Here was another problem discussed in this thread (corrupting heap overwriting area marked free), and it doesn't exist any more. Currently I see a possible fix for that using following condition:
--- HeapSize(GetProcessHeap(), 0, himl) >= sizeof(void*) ---
to determine if required field available to read. This will prevent from reading out of allocated area. Failure will be indicated by -1 return value. This check should be next after checking himl for null.
P.S. I'm using sizeof(void*) here instead of sizeof(DWORD) cause of bug 20696 where I think it's possible to use vtable pointer as a magic (HIMAGELIST isn't reusable for another process anyway).
http://bugs.winehq.org/show_bug.cgi?id=20553
--- Comment #9 from Alexandre Julliard julliard@winehq.org 2009-11-14 14:10:09 --- (In reply to comment #8)
Here was another problem discussed in this thread (corrupting heap overwriting area marked free), and it doesn't exist any more. Currently I see a possible fix for that using following condition:
HeapSize(GetProcessHeap(), 0, himl) >= sizeof(void*)
to determine if required field available to read. This will prevent from reading out of allocated area. Failure will be indicated by -1 return value. This check should be next after checking himl for null.
P.S. I'm using sizeof(void*) here instead of sizeof(DWORD) cause of bug 20696 where I think it's possible to use vtable pointer as a magic (HIMAGELIST isn't reusable for another process anyway).
HeapSize is not a reliable way to test for a valid handle. If you want to check vtable or magic you can do that just as well without a HeapSize first.
http://bugs.winehq.org/show_bug.cgi?id=20553
--- Comment #10 from Nikolay Sivov bunglehead@gmail.com 2009-11-14 20:05:25 --- (In reply to comment #9)
(In reply to comment #8)
Here was another problem discussed in this thread (corrupting heap overwriting area marked free), and it doesn't exist any more. Currently I see a possible fix for that using following condition:
HeapSize(GetProcessHeap(), 0, himl) >= sizeof(void*)
to determine if required field available to read. This will prevent from reading out of allocated area. Failure will be indicated by -1 return value. This check should be next after checking himl for null.
P.S. I'm using sizeof(void*) here instead of sizeof(DWORD) cause of bug 20696 where I think it's possible to use vtable pointer as a magic (HIMAGELIST isn't reusable for another process anyway).
HeapSize is not a reliable way to test for a valid handle. If you want to check vtable or magic you can do that just as well without a HeapSize first.
Then it's a application bug here. I thought this invalid read caused by accessing first member (magic or vtable). If there's no way to do such check it's a chromium problem.
http://bugs.winehq.org/show_bug.cgi?id=20553
--- Comment #11 from Alexandre Julliard julliard@winehq.org 2009-11-15 04:05:20 --- The first thing to do would be to clear the magic in ImageList_Destroy, otherwise there's no point in checking it. That should be good enough to prevent crashes. It won't prevent the valgrind warning but that's a chromium bug.
http://bugs.winehq.org/show_bug.cgi?id=20553
--- Comment #12 from Nikolay Sivov bunglehead@gmail.com 2009-11-16 19:47:43 --- (In reply to comment #11)
The first thing to do would be to clear the magic in ImageList_Destroy, otherwise there's no point in checking it. That should be good enough to prevent crashes. It won't prevent the valgrind warning but that's a chromium bug.
At Wine side tested field is cleared now as of commit 7caa61fde648ad7c3275e9d4358ec10b182c2e6b.
http://bugs.winehq.org/show_bug.cgi?id=20553
Dan Kegel dank@kegel.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution| |FIXED
--- Comment #13 from Dan Kegel dank@kegel.com 2009-11-27 16:34:29 --- And, for the record, the app is now fixed, too: http://src.chromium.org/cgi-bin/gitweb.cgi?p=chromium.git;a=commitdiff;h=cf3...
Marking fixed since both app and wine behave better now.
http://bugs.winehq.org/show_bug.cgi?id=20553
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #14 from Alexandre Julliard julliard@winehq.org 2009-12-04 12:16:23 --- Closing bugs fixed in 1.1.34.
http://bugs.winehq.org/show_bug.cgi?id=20553
Nikolay Sivov bunglehead@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |7caa61fde648ad7c3275e9d4358 | |ec10b182c2e6b
-
wine-bugs＠winehq.org