Fix crashes on disconnected pins in wmdks topologies.

[cme]

Odd hardware or broken driver installations encountered by some Hydrogen users on Windows cause a crash in the application on initialising PortAudio. The WMDKs topology filters on these Windows machines contain pins which are not connected.

The code asserts that this can never happen. It turns out, it does indeed happen, consistently and repeatably but only on certain machines, causing much frustration to a small number of users (and a smaller number of developers trying to figure out why their app crashes on machines they can't get access to :D ).

The fix removes the erroneous asserts and passes the error condition to the caller, and allows the caller to just consider the topology unusable.

https://music.columbia.edu/pipermail/portaudio/2014-August/016249.html

[sagamusix]

I have encountered these assertions myself with the ESI Juli@ PCI sound card and I can confirm that PortAudio works as intended when removing the assertions. We have removed them in OpenMPT as well and this solved issues others users had.

[philburk]

I generally approve of this change. I think we should avoid asserts on conditions that are outside our control, like device configuration. Also it seems like a very low risk change.

[philburk]

Thanks!

[philburk]

This may be related to #245
which mentions an assert(FALSE) and also an infinite loop involving finding Connections.

[cme]

This may be related to #245
which mentions an assert(FALSE) and also an infinite loop involving finding Connections.

Ah, yes, I saw that before also. I suspect the infinite looping is probably because of a broken topology that contains a cycle, so similar issue but won't be fixed by this fix.

I think the takeaway message is that WMDKS topologies can't be trusted (I think they're stored in the registry, so are user-malleable and should be treated very defensively)

[sagamusix]

There was a similar issue with Kernel Streaming on Windows XP that was fixed a long while ago; maybe the infinite loop with WaveRT requires a similar fix?

[cme]

Aha, that does look very much like the topology contains a mux which feeds back to itself. It's probably not supposed to do that, but again I think PortAudio should deal with the possibility.

A simple array indexed on id of visited nodes is enough to safely detect a cycle and avoid it.

[cme]

@sagamusix it looks like the infinite looping issue has been fixed, as there's a limit set on the loop, added in: d7085cb2 . Less elegant than a map of visited nodes, but it works.

[sagamusix]

I didn't have a deep look but is it really fixed for both WaveRT and KS or just KS? Both APIs are handled in the same file but newer systems will use WaveRT. Maybe this discussion should be continued though on the related issue.

[RossBencina]

Hi all. I'm not overly familiar with PA/WDMKS, but I agree that calling assert() based on external input is to be avoided. This patch corrects that. However I am not (as yet) convinced that this patch resolves the issue -- because the indirect callers of FindStartConnectionFrom/FindStartConnectionTo also have asserts in them (see below). What do you think?

Analysis: The current patch removes asserts from FindStartConnectionFrom and FindStartConnectionTo. In both cases this causes a return of 0/NULL to GetConnectedPin, which in turn causes GetConnectedPin to return KSFILTER_NODE. Now GetConnectedPin has four call sites (listed below). Given that we now consider it normal behavior to return NULL from FindStartConnection..., thus returning KSFILTER_NODE from GetConnectedPin, then the call sites need to deal with it. To my mind, cases 1, and 4 below look fishy. 1884-1886 contain the text "No TOPO pin id ??? This is bad." and line 1974 is "assert(FALSE)". I don't understand the 1884 code -- is it correct? But the assert(FALSE) at 1974 should be replaced by some kind of error logging and cause the function to return paUnanticipatedHostError. What do you think?

#1 Line 1603

portaudio/src/hostapi/wdmks/pa_win_wdmks.c

Line 1603 in 8119355

ULONG topoPinId = GetConnectedPin(pinId, (pin->dataFlow == KSPIN_DATAFLOW_IN), parentFilter, -1, NULL, NULL);

leads to line 1985 "No TOPO pin id ??? This is bad":

portaudio/src/hostapi/wdmks/pa_win_wdmks.c

Line 1984 in 8119355

PA_DEBUG(("PinNew: No topology pin id found. Bad...\n"));

#2 Line 1737

portaudio/src/hostapi/wdmks/pa_win_wdmks.c

Line 1737 in 8119355

endpointPinId = GetConnectedPin(pcPin, TRUE, pin->parentFilter->topologyFilter, -1, NULL, NULL);

leads to line 1741 which results in return of paUnanticipatedHostError

portaudio/src/hostapi/wdmks/pa_win_wdmks.c

Line 1741 in 8119355

result = paUnanticipatedHostError;

#3 Line 1811

portaudio/src/hostapi/wdmks/pa_win_wdmks.c

Line 1811 in 8119355

endpointPinId = GetConnectedPin(pcPin,

lead's to line 1822 "we're done, break;"

portaudio/src/hostapi/wdmks/pa_win_wdmks.c

Line 1822 in 8119355

break;

#4 Line 1919

portaudio/src/hostapi/wdmks/pa_win_wdmks.c

Line 1919 in 8119355

endpointPinId = GetConnectedPin(pcPin,

leads to "assert(FALSE)" at 1974

portaudio/src/hostapi/wdmks/pa_win_wdmks.c

Line 1974 in 8119355

assert(FALSE);

[RossBencina]

I've prepared an additional fix to handle PinNew() failing: #309

We still need to fix PinNew() as outlined above.

[RossBencina]

@cme @sagamusix Hey guys, Phil and I discussed this issue. The four code call sites that I identified remain a concern (lines 1603, 1737, 1811, 1919). We'd like to hear your opinion before deciding whether to merge this PR or not.

[sagamusix]

All I can say is that all the devices that I expect to show up are present in PortAudio's device list, although some of them seem to be missing a "friendly name" probably due to the code paths taken, so the name as returned by PortAudio is e.g. "Input (Juli@ Multi-4ch)" for one input device while for another it's "Juli@ Ch12 (Juli@ Ch12)". I tried to dive into the WDM-KS code but I can't quite wrap my head around it without spending a whole lot of time on it. But as mentioned before - We have used this patch for years without problems, so I would assume it's fine.

[cme]

I'll try and find some time to review this weekend.

[cme]

Agreed, the path from PinNew should use the existing error handling rather than just asserting, so I've incorporated that change.

I've had a quick look through some of the other existing asserts in that code and there are a few others that look as if they're too pessimistic and would be better served by return an error than asserting, but I think addressing those is a separate topic.