New valgrind results, this time with overrun checks and lots of new errors.
http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/ is the first full run with the heap tail check enabled. Here are the first few new problems it found. Somehow, it found a bunch of invalid reads in http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-advapi32_crypt.txt all in a function called test_incorrect_api_usage(). Offhand that seems like something to suppress. The invalid write I already posted about is in http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/diff-advapi32_lsa.txt http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-advapi32_lsa.txt http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/diff-advpack_files.txt http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-advpack_files.txt show an off-by-one buffer size issue (forgot to include space for nul char) in advpack (fix should be easy, anybody can grab this): Invalid write of size 2 at strcpyW (unicode.h:224) by lstrcpyW (string.c:104) by AdvInstallFileW (files.c:283) by AdvInstallFileA (files.c:216) by test_AdvInstallFile (files.c:513) Address 0x7f03d12e is 0 bytes after a block of size 30 alloc'd at notify_alloc (heap.c:279) by RtlAllocateHeap (heap.c:1521) by AdvInstallFileW (files.c:282) by AdvInstallFileA (files.c:216) by test_AdvInstallFile (files.c:513) http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/diff-comctl32_tab.txt http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-comctl32_tab.txt show a more inscrutable error: Invalid write of size 4 at TAB_SetCurSel (tab.c:255) by TAB_WindowProc (tab.c:3367) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_CallProcAtoW (winproc.c:1023) by CallWindowProcA (winproc.c:2299) by tabSubclassProcess (tab.c:404) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_call_window (winproc.c:2223) by call_window_proc (message.c:1635) by send_message (message.c:2482) by SendMessageA (message.c:2627) by test_getters_setters (tab.c:752) by func_tab (tab.c:1174) by run_test (test.h:535) by main (test.h:585) Address 0x7f044098 is not stack'd, malloc'd or (recently) free'd Nikolay touched that code back in March, perhaps he should have a look. etc. etc. Lots of triage to do. It'd be nice if somebody could volunteer to go through these, file bugs, and assign provision blame using 'git blame'... - Dan
Dan Kegel wrote:
http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/ is the first full run with the heap tail check enabled.
So you use some private patches for that, why aren't they merged?
Here are the first few new problems it found.
Somehow, it found a bunch of invalid reads in http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-advapi32_crypt.txt all in a function called test_incorrect_api_usage(). Offhand that seems like something to suppress.
The invalid write I already posted about is in http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/diff-advapi32_lsa.txt http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-advapi32_lsa.txt
http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/diff-advpack_files.txt http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-advpack_files.txt show an off-by-one buffer size issue (forgot to include space for nul char) in advpack (fix should be easy, anybody can grab this): Invalid write of size 2 at strcpyW (unicode.h:224) by lstrcpyW (string.c:104) by AdvInstallFileW (files.c:283) by AdvInstallFileA (files.c:216) by test_AdvInstallFile (files.c:513) Address 0x7f03d12e is 0 bytes after a block of size 30 alloc'd at notify_alloc (heap.c:279) by RtlAllocateHeap (heap.c:1521) by AdvInstallFileW (files.c:282) by AdvInstallFileA (files.c:216) by test_AdvInstallFile (files.c:513)
http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/diff-comctl32_tab.txt http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-comctl32_tab.txt show a more inscrutable error: Invalid write of size 4 at TAB_SetCurSel (tab.c:255) by TAB_WindowProc (tab.c:3367) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_CallProcAtoW (winproc.c:1023) by CallWindowProcA (winproc.c:2299) by tabSubclassProcess (tab.c:404) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_call_window (winproc.c:2223) by call_window_proc (message.c:1635) by send_message (message.c:2482) by SendMessageA (message.c:2627) by test_getters_setters (tab.c:752) by func_tab (tab.c:1174) by run_test (test.h:535) by main (test.h:585) Address 0x7f044098 is not stack'd, malloc'd or (recently) free'd
Nikolay touched that code back in March, perhaps he should have a look.
Yeah, this one is pretty obvious, I'll send a patch for tomorrow commit set.
etc. etc. Lots of triage to do. It'd be nice if somebody could volunteer to go through these, file bugs, and assign provision blame using 'git blame'... - Dan
On Thu, Nov 19, 2009 at 7:37 AM, Nikolay Sivov <bunglehead(a)gmail.com> wrote:
http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/ is the first full run with the heap tail check enabled.
So you use some private patches for that, why aren't they merged?
'Cause I just wrote it an hour before that message? I already posted it to wine-devel for the curious. I'll try to post to wine-patches today or so. In the meantime I've updated http://wiki.winehq.org/Valgrind to link to the patch. I'm not sure about the user (i.e. developer) interface for turning the heap check on. Should it be automatic when one does warn+heap? Or should it need an environment variable or registry setting to turn it on? How should the user control the size of the redzone (a feature Microsoft doesn't seem to offer)? For what it's worth, Windows seems to turn the debug heap on by default when running apps under a debugger, and lets you disable it by setting the environment variable _NO_DEBUG_HEAP=1. That makes me think we should turn this feature on automatically under warn+heap. For finer control, setting _NO_DEBUG_HEAP=1 would disable it, and setting WINE_HEAP_REDZONE would let you pick how big a redzone to use. (Usually 16 bytes is enough, but I've seen bugs that misses but 64 bytes catches.) - Dan
Dan Kegel wrote:
On Thu, Nov 19, 2009 at 7:37 AM, Nikolay Sivov <bunglehead(a)gmail.com> wrote:
http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/ is the first full run with the heap tail check enabled.
So you use some private patches for that, why aren't they merged?
'Cause I just wrote it an hour before that message? I already posted it to wine-devel for the curious.
Actually I didn't spot that, sorry, I just was interested :).
I'll try to post to wine-patches today or so.
In the meantime I've updated http://wiki.winehq.org/Valgrind to link to the patch.
I'm not sure about the user (i.e. developer) interface for turning the heap check on. Should it be automatic when one does warn+heap? Or should it need an environment variable or registry setting to turn it on? How should the user control the size of the redzone (a feature Microsoft doesn't seem to offer)?
If I got it right it makes sense to enable this only running under valgrind control?
For what it's worth, Windows seems to turn the debug heap on by default when running apps under a debugger, and lets you disable it by setting the environment variable _NO_DEBUG_HEAP=1. That makes me think we should turn this feature on automatically under warn+heap. For finer control, setting _NO_DEBUG_HEAP=1 would disable it, and setting WINE_HEAP_REDZONE would let you pick how big a redzone to use. (Usually 16 bytes is enough, but I've seen bugs that misses but 64 bytes catches.)
That's enough maybe, daily script could simply run twice or so with different zone length, results could be a diff to default run (16 bytes) results.
- Dan
On Thu, Nov 19, 2009 at 8:38 PM, Nikolay Sivov <bunglehead(a)gmail.com> wrote:
I'm not sure about the user (i.e. developer) interface for turning the heap check on. Should it be automatic when one does warn+heap?
If I got it right it makes sense to enable this only running under valgrind control?
Nope. It's useful all on its lonesome.
daily script could simply run twice or so with different zone length, results could be a diff to default run (16 bytes) results.
I think one would want to use the biggest redzone one could without hurting performance... the bigger the redzone, the more bugs one finds. The right value will depend on the app. 16 is probably a good default, but conformance tests are tiny, they can probably tolerate huge ones. - Dan
On Thu, Nov 19, 2009 at 11:16 PM, Dan Kegel <dank(a)kegel.com> wrote:
On Thu, Nov 19, 2009 at 8:38 PM, Nikolay Sivov <bunglehead(a)gmail.com> wrote:
I'm not sure about the user (i.e. developer) interface for turning the heap check on. Should it be automatic when one does warn+heap?
If I got it right it makes sense to enable this only running under valgrind control?
Nope. It's useful all on its lonesome.
FWIW, I run with warn+heap daily when running winetest.exe for http://test.winehq.org (without valgrind), so if you want me to run with an extra environment variable to get more warnings/failures for people to fix, let me know :-). -- -Austin
Dan Kegel wrote:
http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/diff-comctl32_tab.txt http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-comctl32_tab.txt show a more inscrutable error: Invalid write of size 4 at TAB_SetCurSel (tab.c:255) by TAB_WindowProc (tab.c:3367) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_CallProcAtoW (winproc.c:1023) by CallWindowProcA (winproc.c:2299) by tabSubclassProcess (tab.c:404) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_call_window (winproc.c:2223) by call_window_proc (message.c:1635) by send_message (message.c:2482) by SendMessageA (message.c:2627) by test_getters_setters (tab.c:752) by func_tab (tab.c:1174) by run_test (test.h:535) by main (test.h:585) Address 0x7f044098 is not stack'd, malloc'd or (recently) free'd This shouldn't be a case for today git - see commit 3371ba9f73367572693589905ed57af87b747b54.
And thanks a lot for these test results, I'm waiting for automated daily runs a lot...
On Fri, Nov 20, 2009 at 9:02 AM, Nikolay Sivov <bunglehead(a)gmail.com> wrote:
Dan Kegel wrote:
http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/diff-comctl32_tab.txt http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-comctl32_tab.txt show a more inscrutable error: Invalid write of size 4 at TAB_SetCurSel (tab.c:255) by TAB_WindowProc (tab.c:3367) ... This shouldn't be a case for today git - see commit 3371ba9f73367572693589905ed57af87b747b54.
And thanks a lot for these test results, I'm waiting for automated daily runs a lot...
Here's the next run: http://kegel.com/wine/valgrind/logs/2009-11-26-06.08/vg-comctl32_header.txt Conditional jump or move depends on uninitialised value(s) at winetest_vok (test.h:306) by winetest_ok (test.h:331) by test_hds_nosizing (header.c:1303) by func_header (header.c:1869) by run_test (test.h:535) by main (test.h:585) Uninitialised value was created by a stack allocation at test_hds_nosizing (header.c:1261) Conditional jump or move depends on uninitialised value(s) at HEADER_IsItemFixed (header.c:226) by HEADER_InternalHitTest (header.c:674) by HEADER_HitTest (header.c:1292) by HEADER_WindowProc (header.c:2065) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_CallProcAtoW (winproc.c:1023) by CallWindowProcA (winproc.c:2299) by header_subclass_proc (header.c:415) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_call_window (winproc.c:2223) by call_window_proc (message.c:1635) by send_message (message.c:2482) by SendMessageA (message.c:2627) by test_hds_nosizing (header.c:1328) by func_header (header.c:1869) by run_test (test.h:535) by main (test.h:585) Uninitialised value was created by a stack allocation at test_hds_nosizing (header.c:1261)
participants (3)
-
Austin English -
Dan Kegel -
Nikolay Sivov