Help with 6120d7cc145, causing bugs

[Jeff Cook]

Just wondering if I could get a bit more review on commit 6120d7cc145 
which was committed in early August. Here's a link: 
http://source.winehq.org/git/wine.git/?a=commit;h=6120d7cc14522983fbc38026ab...

There have been a few bug reports filed against it recently, see 
http://bugs.winehq.org/show_bug.cgi?id=23902 and 
http://bugs.winehq.org/show_bug.cgi?id=24131 .

It seems that in some relatively rare sound setups, an inappropriate 
sound element gets fed in to filllines_no_master which leads to a crash 
on audio initialize. To me this is weird because the program is 
currently written so that it flows into filllines_no_master only if 
there no is master control. A patch adding else if (micelem) instead of 
just else was recently committed and fixed at least one person's 
problem. Some people on the bug report are unable to provide full debug 
information so it makes this harder.

If you have the time, please review the patch and let me know what 
errors you notice that may be causing the aforementioned bug. I am new 
to Wine dev so please forgive anything really stupid. :)

Thanks
From
Jeff.

> There have been a few bug reports...
> http://bugs.winehq.org/show_bug.cgi?id=23902 and 
> http://bugs.winehq.org/show_bug.cgi?id=24131 .
>
> It seems that in some relatively rare sound
> setups, an inappropriate sound element gets fed in to
> filllines_no_master which leads to a crash on audio initialize.

Hi,

i put eyes & fingers into a code, which is bad. People like me,
should never do such things, but it just shows my desperateness
around this problem. Am i correct when saying:

HEAP is allocated begining at 0x004e2b58 and 1260bytes long:
0009:Call ntdll.RtlAllocateHeap(00110000,00000008,000004ec)
ret=7dfb31c2
0009:Ret  ntdll.RtlAllocateHeap() retval=004e2b58 ret=7dfb31c2

so HEAP ends up at 0x004E3043 followed by 12 "safety bytes"
(starting at 0x004e3044), because i run the app with
WINEDEBUG=warn+heap.

This allocation is shortly followed and used by:
0009:Call KERNEL32.MultiByteToWideChar(0000fdf2,00000000,7d5807e8
"Headphone",ffffffff,004e2b58,00000020) ret=7dfb2382
0009:Ret  KERNEL32.MultiByteToWideChar() retval=0000000a
ret=7dfb2382

...

0009:Call KERNEL32.MultiByteToWideChar(0000fdf2,00000000,7d6d8b18
"Digital",ffffffff,004e3044,00000020) ret=7dfb268b
0009:Ret  KERNEL32.MultiByteToWideChar() retval=00000008
ret=7dfb268b

So this "D" is written to a place (0x004e3044), where it shouldn't
be ("safety bytes").

At the end of application run, the wine complains:
0009:Call ntdll.RtlFreeHeap(00110000,00000000,004e2b58) ret=7dfb3778
err:heap:HEAP_ValidateInUseArena Heap 0x110000: block 0x4e2b58 tail
overwritten at 0x4e3044 (byte 0/12 == 0x44)

And this 0x44 stands for "D" from ascii? Mentioned address 0x4e2b58
appears only in the begining (allocation) and at the end (free).

Is this helpful somehow? What should i do something next?

Thanks in advance for any help. Longer log attached.
W.

Also from log previously attached, following string is written to
address out of this word, isn't? Look like some return address than
HEAP, right?

0009:Call KERNEL32.MultiByteToWideChar(0000fdf2,00000000,0021cac8
"HDA NVidia",ffffffff,7dfc9ee4,00000020) ret=7dfb2ae4
0009:Ret  KERNEL32.MultiByteToWideChar() retval=0000000b
ret=7dfb2ae4

[Henri Verbeet]

On 27 August 2010 14:04,   wrote:
> Also from log previously attached, following string is written to
> address out of this word, isn't? Look like some return address than
> HEAP, right?
>
> 0009:Call KERNEL32.MultiByteToWideChar(0000fdf2,00000000,0021cac8
> "HDA NVidia",ffffffff,7dfc9ee4,00000020) ret=7dfb2ae4
> 0009:Ret  KERNEL32.MultiByteToWideChar() retval=0000000b
> ret=7dfb2ae4

Well, it would be this one:
> 0009:Call KERNEL32.MultiByteToWideChar(0000fdf2,00000000,7d6d8b18 "Digital",ffffffff,004e3044,00000020) ret=7dfb268b
> 0009:Ret  KERNEL32.MultiByteToWideChar() retval=00000008 ret=7dfb268b
> 0009:Call ntdll.RtlAllocateHeap(00110000,00000008,000004ec) ret=7dfb31c2
> 0009:Ret  ntdll.RtlAllocateHeap() retval=004e2b58 ret=7dfb31c2

This allocates 0x4ec bytes at 0x4e2b58, so that's 0x4e2b58 ->
0x4e3044. MultiByteToWideChar() writing 0x20 wide chars at 0x4e3044 is
clearly outside that range. At first sight I'd say there's an error in
the calculation for "mixdev[mixnum].chans" somewhere, that's what
ultimately determines the size of the block that's allocated above,
and should correspond to the number of lines filled in filllines(), 16
in the log you attached.

[Henri Verbeet]

On 27 August 2010 14:33, Henri Verbeet  wrote:
> clearly outside that range. At first sight I'd say there's an error in
> the calculation for "mixdev[mixnum].chans" somewhere, that's what
> ultimately determines the size of the block that's allocated above,
> and should correspond to the number of lines filled in filllines(), 16
> in the log you attached.

Actually, looking a bit at what
6120d7cc14522983fbc38026ab4fcb6e4a68cdf0 actually does, does the
attached patch make any difference for you?

> Actually, looking a bit at what
> 6120d7cc14522983fbc38026ab4fcb6e4a68cdf0 actually
> does, does the attached patch make any difference for you?

Yes! :-D

For crashing tested on: Warcarft 3, 3DMark 2001 SE, Harbinger

For HEAP corruption tested on: Demoscene debris

All apps are working perfectly! Could i kindly ask you for sending
to patches today? It looks like i will have spare time for testing,
so it will help me to not to mess with revering.

Thank you for quick analysis and patch.

W.

[Henri Verbeet]

On 27 August 2010 15:14,   wrote:
> All apps are working perfectly! Could i kindly ask you for sending
> to patches today? It looks like i will have spare time for testing,
> so it will help me to not to mess with revering.

It's probably not quite correct. In particular, before 6120d7cc145
it's likely the Mic element would be counted in capcontrols as well,
and skipped for the chan count in some cases. Also, the channel count
for USB mics will probably end up as 2 now, which would make it hit
the "No channels found, skipping device!\n" path again. I think I'd
prefer the original author or Maarten to have a look at this, the code
looks somewhat complicated to me, perhaps more so than really needed.

[Jeff Cook]

Wylda reported success with the attached patch on bug 23902. Does it
seem OK to everyone else? In this case, the extra channel needed for
capture is not added when a micelem is found, because micelem should
only be found when there is no other suitable playback or capture
controls (i.e. "Mic" element only). Though I'm not sure if that's
working, so the micelem detection could probably still stand for some
reworking, but this seems to fix things for now.

On Fri, Aug 27, 2010 at 7:51 AM, Henri Verbeet  wrote:
> On 27 August 2010 15:14,   wrote:
> > All apps are working perfectly! Could i kindly ask you for sending
> > to patches today? It looks like i will have spare time for testing,
> > so it will help me to not to mess with revering.
>
> It's probably not quite correct. In particular, before 6120d7cc145
> it's likely the Mic element would be counted in capcontrols as well,
> and skipped for the chan count in some cases. Also, the channel count
> for USB mics will probably end up as 2 now, which would make it hit
> the "No channels found, skipping device!\n" path again. I think I'd
> prefer the original author or Maarten to have a look at this, the code
> looks somewhat complicated to me, perhaps more so than really needed.

[sudemon]

On 27.08.2010 18:47, Henri Verbeet wrote:
> 6120d7cc14522983fbc38026ab4fcb6e4a68cdf0 actually does, does the
> attached patch make any difference for you?

Thanks, patch solved the problem.