CoGetInterfaceAndReleaseStream does not mix with smart pointers


One horrible gotcha of the Co­Get­Interface­And­Release­Stream function is that it releases the stream. This is a holdover from the old days before smart pointers. The function released the stream to save you from having to call Release yourself. But nowadays, everybody is using smart pointers, so you never had to type Release to begin with. The problem is that you can fall into a double-Release situation without realizing it.

// Code in italics is wrong
void GetTheInterface(REFIID iid, void** ppv)
{
  Microsoft::WRL::ComPtr<IStream> stream;
  GetTheStream(&stream);
  CoGetInterfaceAndReleaseStream(stream.Get(), iid, ppv);
}

void GetTheInterface(REFIID iid, void** ppv)
{
  ATL::CComPtr<IStream> stream;
  GetTheStream(&stream);
  CoGetInterfaceAndReleaseStream(stream, iid, ppv);
}

void GetTheInterface(REFIID iid, void** ppv)
{
  _com_ptr_t<IStream> stream;
  GetTheStream(&stream);
  CoGetInterfaceAndReleaseStream(stream, iid, ppv);
}

struct Releaser
{
    void operator()(IUnknown* p) { if (p) p->Release(); }
};

void GetTheInterface(REFIID iid, void** ppv)
{
  IStream* rawStream;
  GetTheStream(&rawStream);
  std::unique_ptr<IStream, Releaser> stream(rawStream);
  CoGetInterfaceAndReleaseStream(stream.get(), iid, ppv);
}

All of the code fragments above look completely natural, and they all have a bug because the smart pointer object stream is going to call Release at destruction, which will double-release the pointer because Co­Get­Interface­And­Release­Stream already released it.

This type of bug is really hard to track down.

One way to fix this is to call the function and tell the smart pointer class that you are transferring ownership of the stream to the function.

void GetTheInterface(REFIID iid, void** ppv)
{
  Microsoft::WRL::ComPtr<IStream> stream;
  GetTheStream(&stream);
  CoGetInterfaceAndReleaseStream(stream.Detach(), iid, ppv);
}

void GetTheInterface(REFIID iid, void** ppv)
{
  ATL::CComPtr<IStream> stream;
  GetTheStream(&stream);
  CoGetInterfaceAndReleaseStream(stream.Detach(), iid, ppv);
}

void GetTheInterface(REFIID iid, void** ppv)
{
  _com_ptr_t<IStream> stream;
  GetTheStream(&stream);
  CoGetInterfaceAndReleaseStream(stream.Detach(), iid, ppv);
}

void GetTheInterface(REFIID iid, void** ppv)
{
  IStream* rawStream;
  GetTheStream(&rawStream);
  std::unique_ptr<IStream, Releaser> stream(rawStream);
  CoGetInterfaceAndReleaseStream(stream.release(), iid, ppv);
}

Another way to fix this is to simply stop using Co­Get­Interface­And­Release­Stream with smart pointers, because the function was designed for dumb pointers. For smart pointers, use Co­Unmarshal­Interface.

void GetTheInterface(REFIID iid, void** ppv)
{
  Microsoft::WRL::ComPtr<IStream> stream;
  GetTheStream(&stream);
  CoUnmarshalInterface(stream.Get(), iid, ppv);
}

void GetTheInterface(REFIID iid, void** ppv)
{
  ATL::CComPtr<IStream> stream;
  GetTheStream(&stream);
  CoUnmarshalInterface(stream, iid, ppv);
}

void GetTheInterface(REFIID iid, void** ppv)
{
  _com_ptr_t<IStream> stream;
  GetTheStream(&stream);
  CoUnmarshalInterface(stream, iid, ppv);
}

void GetTheInterface(REFIID iid, void** ppv)
{
  IStream* rawStream;
  GetTheStream(&rawStream);
  std::unique_ptr<IStream, Releaser> stream(rawStream);
  CoUnmarshalInterface(stream.get(), iid, ppv);
}
Comments (14)
  1. A third way to fix this is to manually call AddRef right before calling CoGetInterfaceAndReleaseStream.

    [Have you tried it? (Hint, it doesn't compile half the time.) -Raymond]
  2. NotThisMind says:

    Dumb pointers, well, that's funny.

  3. @Maurits [MSFT]: That feels more like a workaround than a codefix.  Does work in a pinch though.

  4. Yukkuri says:

    Don't release the streams!!! More than once, anyway...

  5. Alex Cohn says:

    I like the solution with Detach() more, because it allows to release the Stream as early as possible.

  6. Bob says:

    I used a C bignum library that had a convention where all functions decremented the refcount and required you to increment it if you wanted to keep it around (i.e. the functions behaved like linear logic):

    // copy(x) { ++x.refCount; return x; }

    c = add(copy(a), copy(b));

    it was actually quite pleasant to work with compared to the regular COM convention. It vaguely mirrored what C++ style smart pointers and std::move()s look like except using plain C.

    The problem isn't so much having functions that automatically release for you, it's having both kinds functions around.

  7. Huh, weird. Previously, I submitted a comment to the effect that:

    struct Releaser

    {

       void operator(IUnknown* p) { if (p) p->Release(); }

    };

    had a missing pair of parentheses in line 3, which should instead be

       void operator()(IUnknown* p) { if (p) p->Release(); }

    It looks like the comment didn't go through, but the mistake was corrected anyway. Maybe Raymond's psychic powers are even more powerful than I had thought!

    [That's a general policy with comments that merely report typos. I fix the typo and delete the comment. -Raymond]
  8. Joshua says:

    [That's a general policy with comments that merely report typos. I fix the typo and delete the comment. -Raymond]

    I like the policy.

  9. Gabe says:

    So, in order to prevent the Release from being called on the stream, you have to call stream.release()? That seems somewhat ironic.

  10. Medinoc says:

    Yeah, the standard C++ auto_ptr<> and unique_ptr<> use "release()" instead of "Detach()". I'd call it dumb if they didn't have, IIRC, seniority.

  11. Neil says:

    I don't like the name "release" because it suggests that the smart pointer calls Release() when in fact it's used to avoid that. (The smart pointer library I'm most used to calls that method "forget".)

  12. Cesar says:

    > [That's a general policy with comments that merely report typos. I fix the typo and delete the comment. -Raymond]

    That's the problem with FixTypoAndDeleteComment. Perhaps the commenter was using a smart pointer class?

    (sorry couldn't resist)

  13. anonymouscommenter says:

    [That's a general policy with comments that merely report typos. I fix the typo and delete the comment. -Raymond]

    OK. Then please fix the order of arguments of CoGetInterfaceAndReleaseStream in the sample :-)

Comments are closed.

Skip to main content