Oct 31

Smart Pointers Aren’t Quite Smart Enough…

C++, COM Add comments

What’s wrong with this code? (It is adapted from something I wrote yesterday)

  1. void f()
  2. {
  3.     HRESULT hr;
  4.  
  5.     hr = ::CoInitialize(NULL);
  6.     if (SUCCEEDED(hr)) {
  7.         MSXML2::IMXWriterPtr spMXWriter;
  8.         hr = spMXWriter.CreateInstance(__uuidof(MSXML2::MXXMLWriter30));
  9.         if (SUCCEEDED(hr)) {
  10.             // Use spMXWriter
  11.         }
  12.  
  13.         ::CoUninitialize();
  14.     }
  15. }

The post subject line probably gave it away. The problem is that the MSXML2::IMXWriterPtr’s destructor is executed at the end of the block, after CoUninitialize was called. In other words, the MSXML2::IMXWriterPtr instance was still alive after COM was shut down. While the bug may not observably manifest itself, the code is nonetheless incorrect.

One potential fix is to add an extra scoping block:

  1. void f()
  2. {
  3.     HRESULT hr;
  4.  
  5.     hr = ::CoInitialize(NULL);
  6.     if (SUCCEEDED(hr)) {
  7.         { // Add block to properly scope COM objects lifetime
  8.             MSXML2::IMXWriterPtr spMXWriter;
  9.             hr = spMXWriter.CreateInstance(__uuidof(MSXML2::MXXMLWriter30));
  10.             if (SUCCEEDED(hr)) {
  11.                 // Use spMXWriter
  12.             }
  13.         }
  14.  
  15.         ::CoUninitialize();
  16.     }
  17. }

This is ugly and error-prone. A cleaner solution is to wrap the COM initialization context in its own C++ object, as in:

  1. class COMInitCtxt
  2. {
  3. public:
  4.     COMInitCtxt()
  5.     {
  6.         HRESULT hr = ::CoInitialize(NULL);
  7.         if (FAILED(hr))
  8.             throw hr; // In real code I’d probably throw something
  9.                       // derived from std::exception because of <a href="http://www.deez.info/sengelha/2006/01/30/c-exception-handling/">this</a>
  10.     }
  11.  
  12.     ~COMInitCtxt()
  13.     {
  14.         ::CoUninitialize();
  15.     }
  16. };
  17.  
  18. void f()
  19. {
  20.     HRESULT hr;
  21.  
  22.     COMInitCtxt ctxt;
  23.  
  24.     MSXML2::IMXWriterPtr spMXWriter;
  25.     hr = spMXWriter.CreateInstance(__uuidof(MSXML2::MXXMLWriter30));
  26.     if (SUCCEEDED(hr)) {
  27.         // Use spMXWriter
  28.     }
  29. }

… or use the ScopeGuard equivalent:

  1. void f()
  2. {
  3.     HRESULT hr;
  4.  
  5.     hr = ::CoInitialize(NULL);
  6.     if (FAILED(hr))
  7.         throw hr; // TODO: Throw a better exception
  8.     ON_BLOCK_EXIT(CoUninitialize);
  9.  
  10.     MSXML2::IMXWriterPtr spMXWriter;
  11.     hr = spMXWriter.CreateInstance(__uuidof(MSXML2::MXXMLWriter30));
  12.     if (SUCCEEDED(hr)) {
  13.         // Use spMXWriter
  14.     }
  15. }

Comments are closed.

WP Theme & Icons by N.Design Studio
Entries RSS Comments RSS Log in