Your program assumes that COM output pointers are initialized on failure; you just don’t realize it yet


We saw last time that the COM rules for output pointers are that they must be initialized on return from a function, even if the function fails. The COM marshaller relies on this behavior, but then again, so do you; you just don't realize it yet.

If you use a smart pointer library (be it ATL or boost or whatever), you are still relying on output pointers being NULL when not valid, regardless of whether or not the call succeeded. Let's look at this line of code from that article about IUnknown::QueryInterface:

CComQIPtr<ISomeInterface> spsi(punkObj);
...
// spsi object goes out of scope

If the IUnknown::QueryInterface method puts a non-NULL value in spsi on failure, then when spsi is destructed, it's going to call IUnknown::Release on itself, and something bad happens. If you're lucky, you will crash because the thing lying around in spsi was a garbage pointer. But if you're not lucky, the thing lying around in spsi might be a pointer to a COM object:

// wrong!
HRESULT CObject::QueryInterface(REFIID riid, void **ppvObj)
{
  *ppvObj = this; // assume success since it almost always succeeds
  if (riid == IID_IUnknown || riid == IID_IOtherInterface) {
    AddRef();
    return S_OK;
  }
  // forgot to set *ppvObj = NULL
  return E_NOINTERFACE;
}

Notice that this code optimistically sets the output pointer to itself, but if the interface is not supported, it changes its mind and returns E_NOINTERFACE without setting the output pointer to NULL. Now you have an elusive reference counting bug, because the destruction of spsi will call CObject::Release, which will manifest itself by CObject object being destroyed prematurely because you just over-released the object. If you're lucky, that'll happen relative soon; if you're not lucky, it won't manifest itself for another half hour.

Okay, sure, maybe this is too obvious a mistake for CObject::QueryInterface, but any method that has an output parameter can suffer from this error, and in those cases it might not be quite so obvious:

// wrong!
HRESULT CreateSurface(const SURFACEDESC *psd,
                      ISurface **ppsf)
{
 *ppsf = new(nothrow) CSurface();
 if (!*ppsf) return E_OUTOFMEMORY;
 HRESULT hr = (*ppsf)->Initialize(psd);
 if (SUCCEEDED(hr)) return S_OK;
 (*ppsf)->Release(); // throw it away
 // forgot to set *ppsf = NULL
 return hr;
}

This imaginary function takes a surface description and tries to create a surface that matches it. It does this by first creating a blank surface, and then initializing the surface. If that succeeds, then we succeed; otherwise, we clean up the incomplete surface and fail.

Except that we forgot to set *ppsf = NULL in our failure path. If initialization fails, then we destroy the surface, and the pointer returned to the caller points to the surface that we abandoned. But the caller shouldn't be looking at that pointer because the function failed, right?

Well, unless the caller called you like this:

CComPtr<ISurface> spsf;
if (SUCCEEDED(CreateSurface(psd, &spsf))) {
 ...
}

If the surface fails to initialize, then spsf contains a pointer to a surface that has already been deleted. When the spsf is destructed, it's going to call ISurface::Release on some point that is no longer valid, and bad things are going to happen. This can get particularly insidious when spsf is not a simple local variable but rather a member of class which itself doesn't get destroyed for a long time. The bad pointer sits in m_spsf like a time bomb.

Although all the examples I gave here involve COM interface pointers, the rule applies to all output parameters.

CComBSTR bs;
if (SUCCEEDED(GetName(&bs)) { ... }

// -or-

CComVariant var;
if (SUCCEEDED(GetName(&var)) { ... }

In the first case, the the GetName method had better not leave garbage in the output BSTR on failure, because the CComBSTR is going to SysFreeString in its destructor. Similarly in the second case with CComVariant and VariantClear.

So remember, if your function doesn't want to return a value in an output pointer, you still have to return something in it.

Comments (16)
  1. […you are still relying on output pointers *begin* NULL when not valid…]

    Typo for “being”?

    [Fixed, thanks. -Raymond]
  2. Logan says:

    This is part of the reason I loathe the operator& overload on those smart pointers. The ship, as they say, has sailed but I really wish we were forced to write

    CComPtr<IFoo> smartptr;

    void *dumbptr;

    if( S_OK == thing->QueryInterface(IID_IFoo, &dumbptr) ) smartptr.Attach( (IFoo*)dumbptr );

    or similar.

  3. Simon Buchan says:

    Com smart pointers’ operator& will clear themselves for this exact reason, so you have to explicitly initialize the output to a bad value, rather than fail to initialize it to a good value, which is far more common.

    Ahh yes. The CreateFoo function, how I loath writing you. I would now write it creating dependencies before the object so the ctor can be nothrow:

    HRESULT CreateFoo(FooParams params, IFoo** outFoo) {

       CFoo::InitParams initParams;

       HRESULT hr = initParams.Initialize(params);

       CFoo* foo = nullptr;

       if (SUCCEEDED(hr))

           if (!(foo = new (nothrow) CFoo(params, initParams)))

               hr = E_OUTOFMEMORY;

       *outFoo = foo;

       return hr;

    }

    Now there is only one return point and outFoo is obviously always initialized, although it does mean a bit more futzing around with moving the dependencies from InitParams in a nothrow fashion: mostly "_bar.attach(initParams.bar.detach());", or "_bar = move(initParams.bar);" now with C++0x.

  4. The problem in CComQIPtr manifests only because CComQIPtr relies on correct implementation of QueryInterface, i.e. it could support inaccurate implementations by checking the return code and setting pointer to NULL on failure; of course the second example is valid regardless.

    [The problem is not specific to CComQIPtr, it was just an example that involved the least amount of typing. The problem is common to all “smart pointer” types which release on destruction.

    CCompPtr<ISomeInterface> spsi;
    if (SUCCEEDED(punkObj->QueryInterface(IID_ISomeInterface, (void**)&spsi)) { … }

    When spsi is destructed, it will attempt to release the inner pointer even if the QI failed and set the inner pointer to a non-NULL value. -Raymond]

  5. Ken Hagan says:

     if(S_OK == thing->QueryInterface(IID_IFoo, &dumbptr))

      smartptr.Attach( (IFoo*)dumbptr );

    There’s nothing to stop you doing that, if you like typing, but since we have to do it "Raymond’s way" if we want to marshal properly and since his way is less typing, and since the run-time overhead is the same, I just don’t see the advantage myself.

    Then there are cases where a function returns two reference counted objects, but fails after setting just one of them. Your idiom starts to look messy.

    Smart pointers are garbage collection done right. In addition to the syntactic invisibility of traditional GC, they are deterministic and extensible. If your language is powerful enough to let you create smart pointers, you don’t need GC. The presence of GC in the language is therefore a pretty big clue that it is lacking in some way.

  6. GregM says:

    “We saw last time that the COM rules for output pointers are that they must be initialized on return from a function, even if the function fails. The COM marshaller relies on this behavior, but then again, so do you; you just don’t realize it yet.”

    Since the out parameters must be initialized to something valid before the function is called for marshalling to work properly, would it bs sufficient to say that the COM rules for output pointers are that they must not be set to something invalid on failure?

    [Even terser: They must not be set to something invalid ever. Note of course that if you don’t set it to NULL on failure, then your caller had better know that he’s on the hook for freeing it even on failure. -Raymond]
  7. Neil says:

    Gecko’s XPCOM layer doesn’t allow you to return values on failure (although writing 0 to output pointers is silently tolerated). This means that it can for instance have its smart QI helpers clear themselves if the QI fails. However its equivalent to the & operator does not support inout arguments (I don’t know whether CComPtr does or not.)

  8. GregM says:

    [Even terser: They must not be set to something invalid ever. Note of course that if you don’t set it to NULL on failure, then your caller had better know that he’s on the hook for freeing it even on failure. -Raymond]

    I was thinking along the lines of not even touching it on failure, that is not setting it to anything at all except on success.

    [On failure, you must set it to NULL. If you don’t touch it, then it contains uninitialized garbage on exit. (We’re talking about [out] parameters, which are uninitialiized on entry, as opposed to [in][out] parameters.) -Raymond]
  9. Logan says:

    @Ken

    The advantage is to avoid having a sneaky and surprising overload of operator&, especially since COM (by necessity) throws void* (and void**) around so much. I’m certainly not going to write it the long way, since there is really no good reason to (except maybe paranoia), while that overload exists, and at this point it is idiomatic so it would be confusing not to do it that way. That said, it strikes me as an extremenly brittle and heavy handed way to go about making using the smart pointer "look the same" as using a dumb pointer. A less verbose option could be

    if(SUCCEEDED(smartptr.QIAttach( thing )) { … }

    No surprising overload, heck it can be safer than a regular (or even a templated QI) because you can’t mix up the GUID and the type.

    Yes, this doesn’t solve the problem of functions tthat return multiple interfaces or even functions that aren’t QueryInterface.

    Maybe I should just go write a COM smart pointer library and quit complaining about the non-obviousness of operator& overloads.

  10. ulric says:

    No, I’m gonna say that our program that uses CComPtr assumes the pointer is left unchanged when QueryInterface fails.  

    The smart pointer’s NULL value is set in its constructor, of course. QueryInterface setting to NULL on failure is not necessary.  

    I’ll give you that our code assumes that the code assumes that  garbage isn’t put in the pointer on failure.  But it doesn’t assume that NULL is put there.

  11. GregM says:

    [On failure, you must set it to NULL. If you don’t touch it, then it contains uninitialized garbage on exit. (We’re talking about [out] parameters, which are uninitialiized on entry, as opposed to [in][out] parameters.) -Raymond]

    Ah, thanks, I was misremembering a quote from the previous article.

  12. Tim Smith says:

    ulric:

    See Raymond’s response to GregM.  We are talking about [out] parameters.  The current value of the pointer inside of CComPtr is meaningless when passed into QueryInterface.  In fact, if it already has a pointer to a valid object, then a reference will be leaked when QueryInterface sets the output value.

    Also, even if you are passing in an initialized CComPtr, that doesn’t mean that that pointer is what is passed into the QueryInterface in question.  If the interface has been marshaled, I’m nearly positive that the QI call has to get thunked over to the proper apartment (if required) in order to be executed.  Thus even if the initial NULL in CComPtr could be considered redundant, the QI implementation must be correct as per the standard in order for the results to be properly thunked back to the caller.

  13. Gabe says:

    Ken: Smart pointers are just reference counting done for you (in most cases). That’s kind of the default method of garbage collection for old languages like VB and Perl that were never intended to be multi-threaded.

    Once you need to support multiple threads, the overhead starts to get pretty high. And if you need to be safe in the face of signals, exceptions, and threads, good luck getting portability or performance.

    Furthermore, "smart" pointers start to look pretty dumb once you have cycles in your object graph. Of course you can manually fix them, but there goes your syntactic invisibility.

    There’s a reason new languages use traditional GC instead of ref counting.

  14. James says:

    In all the examples, the problem is that the function sets the output parameter to something on failure.  You don’t have to set the pointer to NULL if you didn’t put something there optimistically. CComQiPtr already sets it’s pointer to NULL before calling QI.  

    [But what about callers who don’t use CComQIPtr and rely on output pointers being NULL on failure? -Raymond]
  15. ulric says:

    nope, with smart pointers, by virtue of them being C++ objects with constructors and destructors, we don’t assume that the function will set the pointer on failure. We just assumes that the pointer will be untouched, i.e. remain null. (it had to be null, otherwise it would leak)

    It doesn’t matter that the parameter is defined as “[out]”  the spirit of the code is you left it alone.

    [That’s great about the “spirit of the code”, but the rules of COM is that all [out] parameters must be definitely initialized. You may not assume that they will be preinitialized to NULL on entry. Because they might not be. There’s no rule of COM that says “All QueryInterface calls must be made with smart pointers.” -Raymond]
  16. Ken Hagan says:

    "Once you need to support multiple threads, the overhead starts to get pretty high. And if you need to be safe in the face of signals, exceptions, and threads, good luck getting portability or performance."

    Nope, you’ve lost me there. The overhead with multiple threads is merely in using atomic increment or decrement, and unless you are really *hammering* the object from more than one thread you’ll struggle to measure it in even the most contrived benchmark. (Of course, unless these threads are all in the sane apartment, it isn’t possible to hammer anything because each thread will only see its proxy.)

    Signals are non-portable, outside of the rather narrow world of strict Posix. ISO C basically says "you can’t portably do anything with a signal or in a signal handler". So yeah, I’ll struggle to get portability there. Good luck with allocating, freeing, modifying or even safely reading GC-ed objects in that signal handler, by the way.

    Exceptions have zero overhead unless they are thrown, in which case unwinding objects takes the same time as it would during a garbage collection pass, and finding those objects takes considerably less time since the compiler knows exactly where they are.

    "There’s a reason new languages use traditional GC instead of ref counting."

    Yes. Their designers didn’t understand the above points, so didn’t give the programmer the tools to build proper smart pointer classes. That makes them deficient in my book.

Comments are closed.

Skip to main content