A co-worker came by to ask what he thought was a coding "style" question that turned into a correctness issue, and I thought I'd share it.
Someone had defined two COM interfaces:
interface IFoo : IUnknown
{
HRESULT FooMethod1();
HRESULT FooMethod2();
}
They also had a factory interface IBar which had a method HRESULT GetFoo(IFoo **ppFoo).
As a result of new work, the team that owned IFoo wanted to extend IFoo. To do this, they defined a new interface, IFoo2 that inherited from IFoo:
interface IFoo2 : IFoo
{
HRESULT Foo2Method1();
HRESULT Foo2Method2();
}
The team that owned IFoo (and IBar) decided that they didn't want to change the IBar interface to add a GetFoo2 method, feeling that the GetFoo method was "good enough".
My co-worker wanted to call GetFoo and cast the resulting IFoo object into an IFoo2 object (he knew that the IFoo he got was always going to be an IFoo2). He was worried about the stylistic implications of doing the cast.
The problem with doing this turns out not to be a style issue, but instead to be correctness issue. Here's the problem.
Somewhere under the covers, there's a class CFoo that implements IFoo and IFoo2. This is the object that will be returned by the IBar::GetFoo method.
When the compiler lays out this object, the compiler will lay out the data for the class as follows:
| 1 |
CFoo vtable |
| 2 |
IFoo vtable with 1*sizeof(void *) adjustor thunk |
| 3 |
IFoo2 vtable with 2*sizeof(void *) adjustor thunk |
| 4 |
CFoo member variable 1 storage |
| 5 |
CFoo member variable 2 storage |
| 6 |
: |
| : |
: |
When IBar::GetFoo returns, it returns a pointer to the 2nd element of the class (the adjustor thunk will ensure that the right thing happens when you call into the member functions).
The IFoo vtable is laid out in memory like this:
| 1 |
QueryInterface() |
| 2 |
AddRef() |
| 3 |
Release() |
| 4 |
FooMethod1() |
| 5 |
FooMethod2() |
The IFoo2 vtable on the other hand is laid out in memory like this:
| 1 |
QueryInterface() |
| 2 |
AddRef() |
| 3 |
Release() |
| 4 |
FooMethod1() |
| 5 |
FooMethod2() |
| 6 |
Foo2Method1() |
| 7 |
Foo2Method2() |
When the caller calls into a method on IFoo, the compiler will index into the vtable to find the pointer to the code that implements the specified method. By casting from an IFoo to an IFoo2, my co-worker was telling the compiler "I know that this thing is also an IFoo2, so you should act like the vtable is really an IFoo2 vtable.
The first time that he called into Foo2Method1 or Foo2Method2 using this mechanism, if he was lucky, his code would crash. If he was unlucky (and the random chunk of memory sitting at the end of the vtable for IFoo happened to be code that matched the function signature of Foo2Method2, he would simply corrupt memory.
All-in-all, a bad thing to do.
In addition, if the particular class implementation of IFoo chose to implement IFoo2 via COM aggregation (in other words, using the delegator pattern), his cast would defeat that.
The right thing to do in this case is to call QueryInterface on the IFoo object looking for IFoo2, then release the IFoo object since it's no longer needed.
Edit: Fixed typos.