Crashes when filters delete unused pins rather than Releasing them

[GoogleCodeExporter]

What steps will reproduce the problem?

1. Start under debugger 
2. Create graph of async file source with MXF file connected to MXF parser
3. disconnect MXF parser input pin

What is the expected output? What do you see instead?

Crashes because MXF parser output pin IPin appears to become an invalid pointer 
after calling Disconnect on the input IPin even though it had a large reference 
count before the call.

May be able to work around this by clearing a Filters cached IPin pointers 
before connecting or disconnecting one of its pins.

Anyone else seeing problems like this?

Original issue reported on code.google.com by [email protected] on 15 Feb 2013 at 10:53

[GoogleCodeExporter]

This is likely happening because pins in this particular filter don't have the 
same lifetime as the filter and the pin is being deleted by its parent filer 
rather than released. 

CBasePin assumes that the pin has the same reference count and lifetime as its 
parent filter and this has to be specifically coded around if not true (see the 
MSDN inftee sample filter).

Not sure if it's worth changing GraphStudioNext to allow for filters that have 
these sort of bugs.

Original comment by [email protected] on 19 Nov 2013 at 6:58

[GoogleCodeExporter]

This bug still happens in the OpenCube MXF parser 2.6.20131016

To reproduce need to be viewing a property page of a pin that has disappeared. 

Closing this as fixing it would require major changes - not holding IPin 
references but referencing pins by filter and pin index, ID or name - none of 
these are foolproof.

Original comment by [email protected] on 23 Apr 2014 at 10:39

• Changed title: Crashes when filters delete unused pins rather than freeing them
• Changed state: WontFix

[GoogleCodeExporter]

Also cf issue #239

Original comment by [email protected] on 11 Jun 2014 at 1:38

[GoogleCodeExporter]

OpenCube MXF parser crash possibly fixed by r424 which closes property pages 
for pins which are no longer valid (cf issue #239). Needs further testing.

Original comment by [email protected] on 10 Nov 2014 at 5:48

• Changed state: Accepted

[GoogleCodeExporter]

Crash no longer occurs. Appears to have been fixed by r424

Original comment by [email protected] on 4 Dec 2014 at 5:57

• Changed state: Fixed

[GoogleCodeExporter]

Original comment by [email protected] on 4 Dec 2014 at 6:05

• Changed state: Accp

[GoogleCodeExporter]

Crash still occurs with following steps

Create graph of async file source with MXF file connected to OpenCube MXF 
parser filter
Show property page of MXF parser filter
Disconnect MXF parser input pin
Click on tab for any output pin of MXF parser
Crash!

As part of refreshing the graph need to notify filter property pages when one 
of the pins is no longer valid. Alternatively we could close the property page 
for filters where any pins have been removed in case other property pages 
reference the deleted pins.

Original comment by [email protected] on 4 Dec 2014 at 6:08

• Changed state: Accepted

[GoogleCodeExporter]

Original comment by [email protected] on 13 Jan 2015 at 11:41

• Changed title: Crashes when filters delete unused pins rather than Releasing them

[mikecopperwhite]

This crash can be reproduced by an any filter where pins can be removed during the filter lifetime and those pins inherit from CBasePin without overriding NonDelegatingAddRef and NonDelegatingAddRelease.

The default CBasePin implementation uses the filter reference count which implicitly forces the lifetime of the filter and all its pins to be the same.

One alternative filter implementation is to AddRef the pin on creation and Release the pin rather than delete when it is removed and override NonDelegatingAddRef and NonDelegatingAddRelease to call the CUnknown implementation. This creates some risk that the pin will reference a deleted filter if the pin lifetime exceeds the filter lifetime but this doesn't happen in practice in Graph Studio Next. This technique is used by the GDCL mp4 mux and demux filters. The inftee sample filter uses a similar but more complex technique.

For robustness it would be better if Graph Studio Next did not cache IPin and other interface pointers for pins and retrieved these interface pointers on demand (perhaps by caching the pin ID). However this also requires that all the calling code needs to check for NULL interfaces on every pin access - this change would be hard to test thoroughly.

[roman380]

The challenge here is that detached pin should start acting as
standalone COM object, when its filter keeps living its own life.

To be able to implement this safely, I suppose at the very least pins
should start having their own reference counter so that at detach time
we have information how many outstanding references the pin has. Then
with positive references, the pin should add an extra reference to owner
filter (if such exists). When pin is being detached from filter this
extra reference should go away.

On 4/22/2015 7:24 PM, Mike Copperwhite wrote:

This crash can be reproduced by an any filter where pins can be
removed and where those pins inherit from CBasePin without overriding
NonDelegatingAddRef and NonDelegatingAddRelease.

The default CBasePin implementation uses the filter reference count
which implicitly forces the lifetime of the filter and all its pins to
be the same.

One alternative filter implementation is to AddRef the pin on creation
and Release the pin rather than delete when it is removed and override
NonDelegatingAddRef and NonDelegatingAddRelease to call the CUnknown
implementation. This creates some risk that the pin will reference a
deleted filter if the pin lifetime exceeds the filter lifetime but
this doesn't happen in practice in Graph Studio Next.

For robustness it would be better if Graph Studio Next did not cache
IPin and other interface pointers for pins and retrieve these
interface pointers on demand (perhaps by caching the pin ID). However
this also requires that all the calling code needs to check for NULL
interfaces on every pin access - this change would be hard to test
thoroughly.

—
Reply to this email directly or view it on GitHub
#115 (comment).

[mikecopperwhite]

Modifying the pin to have normal COM reference count and lifetime independent of its parent filter and having the filter Release the pin rather than delete works OK. The pin can't AddRef its parent filter though as this would cause a circular reference count (the filter already has to AddRef the pins it creates) and the filter reference count would never fall to zero. This technique seems robust enough for MSDN sample code and other sample code I've seen. Client code that calls pin interfaces other than AddRef, Release after the filter has been deleted can still cause crashes though (Graph Studio Next doesn't appear to use its cached pin interfaces in dangerous ways like this).

In the theory the CBasePin::m_pFilter member could be set to NULL when the pin is no longer needed but this requires extensive checking as the baseclasses do not check for a NULL m_pFilter pointer and generally expect it to be immutable. It would also require locking to make sure it was safe to set/get m_pFilter.

[roman380]

Pin's AddRef to filter is OK as long as every Release has a check that
all outstanding AddRefs belong to pins, in which case filter+pins
construction starts disassembing itself.

The question is whether holding only pin reference should keep filter
alive or not. The safest approach is that it is okay. One can obtain
filter interface back from pin and use this filter further as uaually.
In this case AddRef from pin to filter is a must.

Otherwise with a pin only reference the pin has to detach from filter
and pin interface is valid in COM terms and crash-safe, however filter
is already unusable.

I suppose both approaches are workable.

On 4/23/2015 12:50 PM, Mike Copperwhite wrote:

Modifying the pin to have normal COM reference count and lifetime
independent of its parent filter and having the filter Release the pin
rather than delete works OK. The pin can't AddRef its parent filter
though as this would cause a circular reference count (the filter
already has to AddRef the pins it creates) and the filter reference
count would never fall to zero. This technique seems robust enough for
MSDN sample code and other sample code I've seen. Client code that
calls pin interfaces other than AddRef, Release after the filter has
been deleted can still cause crashes though (Graph Studio Next doesn't
appear to use its cached pin interfaces in dangerous ways like this).

In the theory the CBasePin::m_pFilter member could be set to NULL when
the pin is no longer needed but this requires extensive checking as
the baseclasses do not check for a NULL m_pFilter pointer and
generally expect it to be immutable. It would also require locking to
make sure it was safe to set/get m_pFilter.

—
Reply to this email directly or view it on GitHub
#115 (comment).

[mikecopperwhite]

Interesting idea for reference counting, thanks.

I suppose the remaining question is whether Graph Studio Next should be made robust against fragile third party filters which delete rather than Release pins before the filter dies (like the OpenCube MXF parser mentioned above). It's quite a big change and not easy to test thoroughly.

[roman380]

I agree that changing core COM implementation is a kind of a problem and then we're again dealing with issues in external modules and not on our side.

It looks like standard implementation of CBasePin::NonDelegatingAddRef/NonDelegatingRelease referencing m_pFilter suggests that all IPin interfaces should be treated as temporary, which is what you started from. I thought that maybe we could keep not just IPin but IPin+IBaseFilter interfaces to extend lifetime of hosting filter, but it won't fix the problem since pin might be deleted by filter and its outstanding interface pointer becomes invalid anyway.