Casting from one interface to another...
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.
Comments
Anonymous
February 15, 2008
A class that "implements IFoo and IFoo2" is hopefully just inheriting IFoo2, since that necessarily requires implementing IFoo. Every IFoo2 is also an IFoo, and if you have an IFoo2*, then you can cast to IFoo* with impunity, thanks to the magic of single inheritance. If you have an IFoo*, the only way to make it into an IFoo2* is QueryInterface(), which is, I think, the point of this post. If there's a style question in this post, it has to do with how to do the cast. I suspect the whole reason the question came up is because the co-worker was using C-style casts, and although (IFoo2*)pFoo is wrong, the compiler will let you do it. If you try to use static_cast<IFoo2*>(pFoo), the compiler will throw an error, exactly because this is wrong.Anonymous
February 15, 2008
I suspect that this also has something to do with why CreateObject calls in COM take a parameter specifying the interface of the object that you want to use. In this case, GetFoo should really be something like: HRESULT GetFoo(IUnknown **ppFoo, IID *piid) Then, the caller could specify that they wanted an IFoo2 explicitly, and if that wasn't supported, they would get an E_NO_INTERFACEAnonymous
February 15, 2008
Actually, in general given a class CFoo : IFoo2, and IFoo2 : IFoo, if you have a IFoo* that you know is actually a CFoo, it's perfectly safe to do static_cast<IFoo2*>(p). The vtable will be adjusted on the cast so it'll work fine. If you're not sure the pointer is an IFoo2 you could use a dynamic_cast which throws an exception if it isn't. In the context of COM however, you shouldn't assume that's how the interfaces are implemented, and of course you should use QueryInterface. Speaking of style, I hope nobody still actually prefixes their classes with a C. :PAnonymous
February 15, 2008
Based on the code you've provided, I'm afraid that your layout for the class in incorrect. You gave us the following assumptions: IFoo : IUnknown and IFoo2 : IFoo Now you mentioned that CFoo implements both interfaces. However, since IFoo2 inherits from IFoo, that means that it's: CFoo : IFoo2 Since this is a single-inheritance chain, then adjustor thunks are not necessary, so for a CFoo, IFoo and IFoo2 can (and do) share the same vtable. The one and only vtable is laid out the way that you laid out the IFoo2 vtable. The class will have one vtbl pointer at the top followed by the data for CFoo. Because of the single inheritance, all casts can and will work just fine. Now I do agree that depending on this is a bad thing, because if IFoo2 were broken out to have no inheritance from IFoo, then the class looks closer to what you've described. Since QueryInterface works no matter what, it is, all in all, the right solution in the long run. But your friend's code was in no danger of crashing. RyanBemrose: You're wrong. It's a perfectly valid static_cast to go from IFoo* to IFoo2*. The only time it wouldn't be a valid static_cast would be if there were multiple copies of a base class and you tried to cast to a subclass. For instance, if you had, B:A, C:A, and D:C,B, then you can't cast an A to a D, because there are two copies of A in D. SvenGroot: "The vtable will be adjusted on the cast so it'll work fine." - There is no vtable adjustment in this case, since there's only one vtable and the compiler knows it.Anonymous
February 15, 2008
The comment has been removedAnonymous
February 15, 2008
Sven: Actually lots of Windows code prefixes their classes with C - it's a legacy of the old hungarian days. And the cast isn't safe unless the <i>compiler</i> can tell that it's a CFoo - otherwise it has to assume that it's a class with nothing but a vtable.Anonymous
February 15, 2008
Mark: Because that would change the interface contract for IBar, which means that IBar's interface ID would have to change.Anonymous
February 15, 2008
QueryInterface isn't a particularly controversial topic (and I haven't heard any grumbling aboutAnonymous
February 16, 2008
The comment has been removedAnonymous
February 16, 2008
The comment has been removedAnonymous
February 16, 2008
Jack, you're not. But what happens if the class derives from IFoo and IFoo2 (I believe that it's legal). In that case, you have a case of multiple inheritance.Anonymous
February 17, 2008
Jack, if you and the compiler both know the structure of the COM object, then why are you even bothering to use COM? For the current (and past) implementations of vtables in MSVC, what you say is correct. However, what about Borland C++ or any other language that properly supports COM? COM makes no statement as to the structure of the vtables for an object supporting multiple interfaces. It only makes a statement about each interface's vtable. Since we are working with COM, we only have available the rules COM provides us. Your statements, while correct depend on both the implementation of the object and the compiler in order to prove something true. This is very dangerous in programming. In Larry's case, he is using the very same details (with a easily corrected technical error) to show why something is bad. That is perfectly legit.Anonymous
February 17, 2008
BTW Jack, YOU are giving people the wrong idea about how C++ casts work and how objects are laid out. They are very dangerous things to depend on. The things that you are saying are compiler implementation details and are NOT addressed in the C++ standard.Anonymous
February 17, 2008
The comment has been removedAnonymous
February 17, 2008
To be fair: As far as I know, given what I stated in the post, Jack is totally right. On Monday, I'm going to talk a bit about how the standard requires that a compiler handle multiple inheritance (it turns out that much of this vtable behavior is actually required by the standard). As I said however, if the implementor of the class had chosen to implement both IFoo and IFoo2, my co-worker would have been up a creek - that's way too fragile to trust.Anonymous
February 18, 2008
I'm with Jack Mathews My first reaction to the blog was "what?" This up-cast to IFoo2 works perfectly in this simple example, and there is no reason why it could crash. C++ insures this. So this is here entirely a question of style. You shouldn't mix C++ casting with COM, trying to go around QueryInterface. The programmers I've seen doing this were usually driven by misguided beliefs about performance -- which happens a lot.Anonymous
February 18, 2008
At the very beginning of the post it says "Someone had defined two COM interfaces". COM, not C++.Anonymous
February 19, 2008
The comment has been removedAnonymous
February 19, 2008
The comment has been removedAnonymous
February 20, 2008
Sys64738: http://blogs.msdn.com/larryosterman/archive/2004/11/30/272437.aspx Search for "Larry Osterman" "What Does Style Look Like" and you'll find a series of posts I did a few years ago on coding styles. There is no "microsoft" coding style - each team has its own style (sometimes each developer on the team has their own style).Anonymous
February 20, 2008
Thank you! This is what I looked for.Anonymous
February 20, 2008
The comment has been removedAnonymous
February 22, 2008
No wonder why today's code is so awfull with all those interfaces, factories and so many unknowns.Anonymous
February 24, 2008
The comment has been removedAnonymous
February 24, 2008
Igor, that's exactly why you use spaces, not tabs - because it hard codes indentation. Your alternative is that every developer has to have their editors configured identically. And not every editor can be easily configured to always insert tabs. If you use spaces, your indentation is always consistent, regardless of the settings of your editor. You're right that the if (x) { doesn't resemble function entries. But it DOES make it more obvious when you forget an opening brace (and the style requires that there always be an opening brace). And I <i>NEVER</i> gratuitously reformat someone else's code, regardless of how unreadable it is. Instead, I change my style to match the coding style of the module in which it lives. It's more important that the code in the module all look the same than that the code match your personal style.Anonymous
February 25, 2008
The comment has been removedAnonymous
February 25, 2008
The comment has been removedAnonymous
February 26, 2008
The comment has been removedAnonymous
February 26, 2008
"I had a work term student tell me once that he wasn't sure that items in an array, or rows of arrays, were contiguous in memory." Unfortunately your student has powerful forces on her/his side. For 20 years the C and C++ standards have been working very hard to cloud up this particular issue. (Or if your student meant padding to align each item or row then your student was right all along. But I think you didn't mean that because it would be too obvious ^_^)Anonymous
February 26, 2008
http://www.sellsbrothers.com/fun/#re_re_Do_Not_Mix_Cpp_Cast_and_COMAnonymous
February 27, 2008
>>Code indented by spaces can be a nuisance to edit.<< This is true, there are still some editors which do not understand the concept that n spaces = 1 tab. Therefore you have to delete and add spaces one by one. >>Code indented by tabs can be a nuisance to edit.<< Can you give an example? >>When you're done, do you reformat back to the original coder's style?<< I said I merge -- that means I just apply my coding changes to the original source file without applying my style.Anonymous
February 27, 2008
Oh there is one more thing which is part of the coding style but people don't discuss it often -- functions which return BOOL. They write such a function like this: BOOL IsReadable(BYTE *mem) { // test if readable return TRUE; } And then they call it like this: if (!IsReadable(mem)) { // goto or return } Then in order to avoid writing !IsReadable() which is awkward the write nested ifs: if (IsReadable(mem)) { if (condition2) { if (condition3) { // do something } } } Instead of doing it like this: BOOL NotReadable(BYTE *mem) { // test if NOT readable return TRUE; } And calling it like this: if (NotReadable(mem)) { // goto or return } It avoids double negations, nesting ifs, and it is much easier to read.Anonymous
February 27, 2008
"Code indented by tabs can be a nuisance to edit." 'Can you give an example?' Probably not the kind of example you want. The usual case is a VC++ source file because Visual Studio usually inserts spaces to indent lines in other languages. Sometimes I use vim instead of Visual Studio to edit a single source file. Sometimes I use vim at the same time as Visual Studio because I can do some repetitive edit faster in vim than in Visual Studio (maybe less than others could do with emacs or with Visual Studio macros or add-ins, but still sometimes the tradeoff tilts me towards vim). If parts of an expression or function header or whatever were lined up in Visual Studio then they're not lined up any more in vim. A less common case is C# or VB, for exactly the opposite reason. vim likes to indent using tabs even if the adjacent lines have spaces. Then the results come out wrong in Visual Studio. Now, one might think that if one only uses Visual Studio then this problem won't occur. But it still did, I have a VC++ source file that has some lines indented using spaces and some indented using tabs. I don't know how it happened in this one. For one project I used Wordpad to edit .rc files in a Unicode encoding, I think UTF-16.Anonymous
April 09, 2008
this problem would be avoided if IBar::GetFoo() used the REFIID/void** pattern to return the object that implements IFoo or IFoo2 (or IFoo3 for that matter). this of course has a downside, you can't read the method signature and know what types to expect. to mitigate that put a comment above the method that lists the interfaces you can expect to request.