What's wrong with this code, part 14 - the answers

Yesterday's post was a classic example of Joel Spolsky's Law of Leaky Abstractions.

Why?  Well, because it was an example of conflicting contracts.

In COM, the contract for an API is defined  by the APIs interface.  In this case, it was:

[     object,     uuid("0A0DDEDC-C422-4BB3-9869-4FED020B66C5"),     pointer_default(unique) ] __interface IFoo : IUnknown{    HRESULT GetFooConfig([out] struct FooConfig **ReturnedFooConfig);};

A straightforward contract - there's a function named "GetFooConfig" that fills in a pointer with a pointer to a FooConfig structure.  Nothing exciting there.

In this case, the author of the interface chose to return a pointer to a global variable for the output value.  Again, nothing exciting, there's no reason that it wouldn't work - if the _GlobalFooConfig variable is constant, there aren't even multi-threaded access issues (it could be put in a read-only section, for example).

But there's a problem.  The thing is that COM (actually the RPC runtime library, but it's easier to blame COM) has an additional requirement for [out] pointers.  This requirement is that if the type of an [out] parameter isn't a scalar quantity (in other words if it's a structure or anything more complicated than a int or float), then the memory pointed to by the [out] parameter needs to be allocated either by MIDL_user_allocate (for RPC) or CoTaskMemAlloc (for COM).

This is a leaky abstraction - the abstract interface says nothing about this additional requirement, so the COM requirement leaks through to the implementation.  And what's worse, the leak doesn't show up until the RPC marshaller gets involved in the process - until that happens, the interface works perfectly.

So why did this work until the new tests were rolled out?  Well, it was because up until then, the COM object had always been executed in-proc.  But the new test case instantiated the COM object out-of-proc and the RPC runtime library tried to marshal the code and...  Boom.

Kudos:

First off, bao caught a stupid mistake in GetFooConfig() that was unintentional.

ThaleseC correctly called out the fact that we were returning a static variable, but the pieces didn't get fully put together until vrk caught the marshaling issue.

Ralf's comment about not checking for a null ReturnedFooConfig is interesting - the contract for GetFooConfig specifies that the ReturnedFooConfig parameter isn't optional (because it's flagged as [out]).  The RPC runtime guarantees that ReturnedFooConfig is always valid.  And in-proc, crashing on a null parameter is probably as useful as checking for a null pointer - that way programming errors on the part of the client of the interface get caught quickly.

Other comments: Alex pointed out that this wasn't pure ansi C++ because it used the Microsoft C++ annotation extensions.  He's right.

Mike pointed out that my stylistic choice of _Xxx for global variables (and member variables) conflicts with the C++ standard - he's right, they are reserved.

rederick Slijkerman pointed out that CFoo should derive from IUnknown.  This is true, and it happens already, since IFoo derives from IUnknown.  Adding an explicit interface isn't necessary (and can potentially lead to ambiguity)

Finally, mirobin commented on the fact that the call returns a pointer to a data structure - as I pointed out, this isn't necessarily a problem, assuming that the rules for the FooConfig are clearly understood.

On a related note, I'm insanely busy right now so I suspect I'll be rather short on new posts for the next week or so...