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 mike.cop...@gmail.com on 15 Feb 2013 at 10:53

[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.