The ways people mess up IUnknown::QueryInterface, episode 2


Sadly, I get to add another entry to The ways people mess up IUnknown::QueryInterface:

Blindly responding to everything.

Some people are just too eager to please.

HRESULT CSomething::QueryInterface(REFIID riid, void **ppvObj)
{
  *ppvObj = this;
  this->AddRef();
  return S_OK;
}

No matter what the interface is, they say, “Sure, we do that!”

Furthermore, no matter what you ask for, they always return the same interface. Even if it’s not what you asked for.

Exercise: Some people say that “these problems wouldn’t be there if Microsoft Windows had enforced correct behavior to begin with.” My exercise to you, commenter “foo”, is to come up with a list of all bugs in IUnknown::QueryInterface which Windows should enforce and describe how to enforce it. (If your response is “You should have designed it differently so these bugs are impossible to write”, please outline that alternate bug-resistant design. Remember that your design must be language-independent, while still supporting things like tear-offs, aggregation, and remote objects. Oh and it needs to work on typical PC-class computers of the early 1990’s, which ran at 25MHz with 4MB of memory.)

Comments (35)
  1. Alexandre Grigoriev says:

    If I knew how to enforce correct behavior in third party code, I would probably be rich…

  2. someone else says:

    The correct tool to enforce correct behavior in third party code is a solid 2×4. To the head. Repeatedly.

  3. Sunil Joshi says:

    You could require the IID follow the v-ptr in the class layout (before the instance data) and then declare the interfaces like this:

    struct IUnkown

    {

    QueryInterface etc.

    REFIID iid;

    };

    After calling QueryInterface (or more often if it is paranoid), the code could check that iid is correct.

    I guess the layout of objects would then look like this

    v-ptr1

    iid1

    v-ptr2

    iid2

    etc.

    This would reduce the chance of people making this particular mistake (and several related ones.)

  4. bahbar says:

    @Sunil: Right, plus it makes implementing an IID that inherits from another a whole lot easier now. Let’s see, IUNKNOWN gets one vtable, IBASEOBJ another one, ISHAPE another one, IRECTANGLE…

    Brilliant!

  5. Koro says:

    The simple solution to this whould have been to not let people code it themselves.

    Aka: The QISearch helper function should have been there since the very beginning, and people strongly encouraged by the docs to never, ever code QueryInterface themselves, just delegate the work to this API.

    Even better, but slightly more complicated, would have been to get rid of QueryInterface completly and use a "pointer to interface table" embedded directly in the object, encoding IID’s and vptr offsets… Although this would not work for proxy and remote objects.

  6. Sunil Joshi says:

    @dgt:

    If the object implements more than one interface (which all do i.e IUnknown + something else)

    this->iid = riid;

    is ambiguous. They only way they could get it right (in C++) would be to match riid to the correct interface and perform the correct cast (if they then go ahead and return the wrong pointer – there’s only so much you can do to save people.)

    Besides, writing this->iid = riid; suggests they know they’re doing it wrong.

    My understanding of the problem presented above is that the coder has assumed (incorrectly) they would only get Queried for the interface they have actually implemented. It would never occur to such a period that they would need to write this->iid = riid;

    @bahbar

    Sure, that’s the trade-off.

    @Koro

    I actually like your idea of "pointer to interface table" better.

  7. HJB417 says:

    Maybe MS can release a test harness so we can verify our COM objects against these gotchas.

  8. Alexandre Grigoriev says:

    Folks,

    No matter how smart is verification, there is always a dumb coder who will screw it up.

    This is where old good code review should help. Don’t let your undercoders check in code! Do a review first!

  9. gotcha says:

    Don’t you have to get the same (IUnknown*) if you query for IID_IUnknown, regardless which subinterface you start from?

  10. porter says:

    > Don’t you have to get the same (IUnknown*) if you query for IID_IUnknown, regardless which subinterface you start from?

    You most certainly do, and in theory the same is true of IDispatch, which can make implementing multiple dual interfaces interesting.

    While on the QI implementation, throw in IMultiQI and also being an "inner" object of some "outer" object.

  11. Koro says:

    "Don’t you have to get the same (IUnknown*) if you query for IID_IUnknown, regardless which subinterface you start from?"

    Oops. Seems like I learned something today.

    Time to go check my code for violations of this rule.

  12. MS says:

    HJB417: Or, maybe you could write your own unit tests.  But lets face it, there are enough inventive idiots out there that are going to screw up in ways unimagined in third party code.

  13. dgt says:

    modification of QI specifically for Sunil Joshi:

    HRESULT CSomething::QueryInterface(REFIID riid, void **ppvObj)

    {

     *ppvObj = this;

     this->AddRef();

     this->iid = riid;

     return S_OK;

    }

  14. Koro says:

    I just realized that by my “interface table” idea I just totally described the QITAB structure.

    Here’s an improvement on this idea though, and on Sunil’s “IID after vtbl” idea: why not make it so…

    struct IUnknown

    {

     struct IUnknownVtbl

     {

        void* fpAddRef;

        void* fpRelease;

        QITAB* pInterfaces;

     } lpVtbl;

    }

    (I can’t be bothered to look up the proper types, so “void*”‘s the function pointer type for those declarations)

    Now the advantage of it, is that:

    - Vtables can inherit again.

    - The QITAB array can be compiler-generated (maybe with a custom __declspec(cominterface) on the class to tell the compiler to put it there).

    - In cases of objects with “dynamic” interface tables, the object can still, if it wishes, dynamically generate this table, allowing proxying and remoting (just copy the source object’s table and modify to your will).

    - No hassle of ever having to code QueryInterface.

    - A separate API, CoQueryInterface, could be provided to do the hard work.

    [You’d just have different blog entries about applications which fill in their QITAB incorrectly. Oh, and your QITAB needs to have an additional entry to specify the offset from the IUnknown instance to the base of the object. And I bet most people will just put 0 there, and the code will work fine until they add multiple inheritance to their class. (Assuming the compiler folks will supoprt this natively is already contradicted by evidence: It’s been over a decade and we still don’t have __declspec(cominterface) that would auto-implement QI. What makes you think that in this alternate universe, the compiler folks would have a __declspec(cominterface) that auto-implements QI?) Note also that CoQueryInterface means that you can’t have COM without ole32.dll. In its purest form, COM can be implemented without any helper libraries, and in fact many people did exactly that when memory was scarce. Oh, and C++ vtables can’t have data pointers so you already lost the C++ people. -Raymond]
  15. Anonymous Coward says:

    The only way to really prevent people from doing things like this in all scenarios is by using a virtual machine, and that just moves the problem around a bit. It crashes/throws and exception a bit earlier, which may be nice, but not nice enough to want to force everyone to write solely in Java or Dotnet. At the end of the day, you simply can’t force people to stop writing buggy code.

  16. Chris says:

    <blockquote>Remember that your design must be language-independent</blockquote>

    How so? You’re assuming a C++ vtable-like model, and your answers imply that you expect a few minor tweaks to existing (circa 1993) C++ compilers to ensure the COM-style layout. Anything not C++ needs to adapt in some way. You can code COM in C with some pain, but you can code anything in C with varying degrees of pain. What does language-independent mean in this context?

    If you truly mean language independence, one answer would be Objective-C, the contemporary version of which you could’ve coded in raw C with far less fuss than the equivalent COM incantations. A few more cycles for dynamic lookup, but a lot less programmer time wasted over the years.

    [By “language-independent” I mean something that was amenable to being used by multiple languages without too much difficulty. You’d be surprised how much COM code was written in straight C back in the days before C++ hit the big time. It wasn’t really all that bad. p->Method(x) became p->lpVtbl->Method(p, x), that’s about it. -Raymond]
  17. porter says:

    > What does language-independent mean in this context?

    The ABI of course. An object implemented in one language can be used by another, no matter how painful it was to write in the first place.

  18. Koro says:

    “You’d just have different blog entries about applications which fill in their QITAB incorrectly.”

    It’s harder to do this and have it all wrong, especially with proper macros.

    “What makes you think that in this alternate universe, the compiler folks would have a __declspec(cominterface) that auto-implements QI?”

    Don’t they already “support COM” by making sure the vtable layout is always as described in the declarations? Don’t they already support SEH, which is another Windows-only construct? Generating data tables from (already known) data during compilation is an easier task than generating proper QI code.

    “your QITAB needs to have an additional entry to specify the offset from the IUnknown”

    Nope, all you need to know, is how to “go to” one interface to another… imagine this scenario:

    public class Blah : IInterface1, IInterface2

    Class layout will be

    struct Blah

    {

     IInterface1Vtbl*;

     IInterface2Vtbl*;

     <object fields>

    };

    IInterface1Vtbl->pQITab will be

    {

     IID_IUnknown, 0

     IID_IInterface1, 0

     IID_IInterface2, 4

    }

    IInterface2Vtbl->pQITab will be

    {

      IID_IUnknown, 0

      IID_IInterface1, -4

      IID_IInterface2, 0

    }

    So a QueryInterface just gets the pQITab from this->vtbl, finds the proper IID, and adds the offset to the original this, and returns it. No need to ever store an offset to the base of the real object, that’s what adjustor thunks are for anyway.

    “Oh, and C++ vtables can’t have data pointers so you already lost the C++ people.”

    That’s the point of having compiler support here. So that it can generate this “special vtable”.

    Althout it could be replaced by a “GetInterfaceTable” function, but then it would be hard to find out which QITAB to return (because then, the “original” thisptr has been adjusted to the real one already)

    [Okay, I see, baking the offsets into the table. Though this means that an object that derives from n interfaces requires O(n^2) space for QI. Let’s hope n stays small. The nice thing about the existing model is that it matches C++ very closely – as long as you can convince the compiler to put functions in the vtable in declaration order, you’re all set – something most compilers did already anyway. “Hey, here’s a new interop framework, but you have to wait for compiler support before you can use it.” and you still need some sort of way of overriding this for tear-offs and remote object. -Raymond]
  19. Duke of New York says:

    I pity the "foo" who accepts Raymond’s challenge.

  20. Scott says:

    Just have Windows require all such objects to report a valid email you can send complains to about how poorly it was designed. Sending complaint letters won’t fix anything, but you will feel a bit better.

    I’m afraid this is the sort of thing no amount of documentation or tooling can fix. I’m ever amazed by the number of c++ classes out there that have memory leaks or exceptions if you do self assignment because people failed to consider the possibility. People need to experience the problem before they learn the correct practices.

  21. Windows 8 Team says:

    I’d just like to thank all the commenters for giving us insightful information about how to resolve these issues.

    And thank, you, Raymond, for making these people un-invisible.

    Monty thanks you

  22. Frymaster says:

    "The simple solution to this whould have been to not let people code it themselves.

    Aka: The QISearch helper function should have been there since the very beginning, and people strongly encouraged by the docs to never, ever code QueryInterface themselves, just delegate the work to this API."

    agreed, but assuming you leave them the option to use the lower level techniques, someone is going to use them and use them poorly.  And if you don’t let people use the lower level techniques,

    a) someone will complain they can’t do something interesting and possibly even useful

    b) people will complain about undocumented APIs that MS can use but they can’t.  possibly even the same people who don’t read the documentation properly in the first place

    the solution, as far as I can see, is to let someone else write your OS, and just code in java or c# or python or whatever unless you’re doing something like a real-time application or a driver, in which case you cry ;)

  23. dgt says:

    Peter: say hello to CComObject

  24. porter says:

    CoreFoundation/CFPlugInCOM.h

    See how Apple stuffed up IUnknown::QueryInterface they defined REFIID as a CFUUID, not a pointer, not a reference, so all QIs get the full 16bytes on the stack. All the samples of plugins I’ve seen have the QueryInterface allocating and releasing CFUUIDRef in order to do the IID matching using CFEqual.

  25. Peter says:

    Hello.  My name is Peter, and I wrote a non-conforming COM object just yesterday.

    Why?  Because I did’t know if the API I was targeting would even remotely work in a useful way  and it turns out that I’m not a very experienced COM programmer.  So instead of wasting tens of minutes finding correct code for addref and release, I just set them to return ‘1’.

    I needed a real QueryInterface, though.  Turns out I was asked for several different interfaces that I didn’t support.

    Here’s what Microsoft could do to make the problem easier: make a ‘CUnknown’ that implements the bare bones of a correct interface. Then Microsoft just has to update all of the examples to use the new helper class. That way people always have a reasonable solution right away.

    Peter

    (PS: I’m a novice COM programmer.  I’m really rather a good non-COM programmer)

    [The bare bones of a correct IUnknown object is just one two-line function and two one-line fuctions.
    class CUnknown : public IUnknown {
    public:
     CUnknown() : m_cRef(1) { }
     HRESULT QueryInterface(REFIID riid, void **ppv) {
      if (iid != IID_IUnknown) { *ppv = NULL; return E_NOINTERFACE; }
      *ppv = static_cast<IUnknown*>(this); AddRef(); return S_OK;
     }
     ULONG AddRef() { return ++m_cRef; }
     ULONG Release() { ULONG cRef = –m_cRef; if(!cRef) delete this; return cRef; }
    private:
     ULONG m_cRef;
    };
    

    Since it’s so simple, all the samples just inline these three methods. The problem is that nobody wants a bare-bones interface because it does nothing. It is in the process of making it do something interesting (i.e., deriving from it, responding to interfaces other than IUnknown) that the bugs are introduced. There’s also ATL, which does most of the heavy lifting for you, but using ATL in samples creates frustration for people who don’t want to use ATL. -Raymond]

  26. Simon Buchan says:

    I managed to trim down deriving from CUnknown to:

    <code>

    class CFoo : public CUnknown, public IFoo {

    void* GetInterface(REFIID riid) {

     if (IID_IFoo == riid) return static_cast<IFoo*>(this);

     return CUnknown::GetInterface(riid); } // or CFooBase…

    };

    </code>

    I did run into some problems with the double deriving from IUnknown so I was also using a macro that forwards the IUnknown methods to CUnknown, and I wanted to be able to allocate globals and on the stack, so I had Release call OnNoReference instead of ‘delete this’ directly, but overall it makes creating COM classes a breeze. Creating COM *objects*, now…

    [On the stack? What if the reference count isn’t zero when the stack object goes out of scope? I’ve had to chase down real bugs caused by people destroying objects before the reference count goes to zero. Please don’t be another one. -Raymond]
  27. peterchen says:

    @porter: Care to explain?

    Do you see any advantage of a pass-by-value of the IID, or do you comlain that most use it incorrectly?

  28. Peter says:

    [I’m the ‘Peter’ who said there should be an CUknown class and that all the samples should use it.]

    Several people helpfully pointed out that a correct implementation of IUnknown is easy and that the sample code simply implements the three methods inline.  Other people pointed to CComObject.

    So I did quick test: "IUnknown site:msdn.microsoft.com".  Of the top 10 results (via Bing, of course), not a single one shows actual sample code.  There was one sample implementation written in pseudocode.  There also was a CUnknown object (yay!) but it’s not part of mainstream windows, and confusingly doesn’t actually derive from IUnknown.  The CComObject mechanism didn’t show up in the results.

    So: there are convenience functions, but they aren’t actually findable.  

  29. porter says:

    > Do you see any advantage of a pass-by-value of the IID, or do you comlain that most use it incorrectly?

    I think it’s a mistake on Apple’s part. The COM ABI standard has REFIID as pointer to the IID. It saves memory on the stack and is fewer bytes to push. I saw an Apple document on CFPlugInCOM dated 1999 in which the text claimed that REFIID mapped to "& IID" but the header file didn’t tally. That mistake has proliferated. So the question is, does this tally with Microsoft Office on Mac? If not you have two incompatible specifications of IUnknown on the same platform. Also their spec of CFUUIDBytes is shot to bits (and hence GUID/IID/CLSID), it was possibly passable with 68k and PPC, but it is broken with i386, especially with how CFUUIDRef converts to and from string.

  30. peterchen says:

    IMO trying to make an interface unbreakable is a highway to disaster.

    An interface should be easy to use correctly, and hard to use incorrectly. Beyond that, other factors – simplicity, well-factoredness etc. – need totake over.

    Not only is an unbreakable interface about ten or 100 or infinity times as hard as one that’s robust enough. Worse, there’s a responsibility shift: If I succeed to break it, it’s suddenly not my fault anymore. That’s about the worst start you could have given me.

    I have some years of COM under my belt (It’s showing :)) and it’s hard to understand the calls for "more code to fix the existing code". We do have great documentation and the rules are rather simple.

    The worst thing MS did (and does) is using raw QueryInterface implementations even in unrelated examples. Unfortunately that’s what they have to to keep it framework-independent, But that also signals this is the way to go for many programmers. (No I don’t have an idea how to make it better. As long as fixing the *programmers* is still out of question)

  31. ulric says:

    Here’s what Microsoft could do to

    make the problem easier: make a ‘CUnknown’

    that implements the bare bones of a correct interface.

    it’s all in ATL, that’s what it’s for.

    also, in the code in SDK samples that implement COM interfaces.

    So I did quick test: "IUnknown  site:msdn.microsoft.com".

    imho It rarely makes no sense to want to implement IUnknown by itself. Usually you implement it for a reason.  

    but anyway, why don’t you be a search wiz and chnage your search to:

    "IUnknown implementation site:msdn.microsoft.com"

  32. This whole discussion about how to make the API "unbreakable" reminds me strongly of two blog posts by Rusty Russell.

    The first, at http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html, discusses good API design; it lists 10 possible classifications of a good API, from best (the do_what_i_mean() function), to worst (read the right forum posting, blog post, or e-mail archive, and you’ll get it right).

    The second, at http://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html, is far more fun to read – it’s all about ways you can misdesign an interface, from "read the right e-mail archive, and you’ll get it wrong" down to "whatever you do, you’ll get it wrong".

    They make for sobering reading, not least because it quickly becomes apparent that designing an interface well is hard. On Rusty’s scale of 10 to -10, I’d rate the COM interface at around a 4 (follow common convention, and you’ll get it right).

  33. DysgraphicProgrammer says:

    Have something in the background ask object if it supports some guaranteed non existent interface. If it says ‘yes’, automatically email Microsoft’s goon squads to go and stab the programmer in the eye.

    Granted, it won’t fix every bug, but it should improve the general pool of programming talent.

  34. goonie says:

    @DysgraphicProgrammer:

    You can be sure you wouldn’t be reading this blog…

  35. 640k says:

    COM is not an interface. It’s an interface interface.

Comments are closed.