visual-c++mfc com smart-pointers code-analysis

Warnings C26415 / C26418 when passing a COM interface smart pointer to a function

Here is the definition of a function:

void CMSATools::SetPublisherDatesNotAvailable(MSAToolsLibrary::IAvailabilityPtr pAvailability, 
                                              std::vector<COleDateTime>& rVectorDates, 
                                              DWORD dwNumDates)
{
}

The parameter MSAToolsLibrary::IAvailabilityPtr is designed like this:

_COM_SMARTPTR_TYPEDEF(IAvailability, __uuidof(IAvailability));

This code (smart pointer) is all constructed automatically as part of the COM interface. But I am receiving two warnings about using it as a parameter to my own function:

C26415 530 Smart pointer parameter 'pAvailability' is used only to access contained pointer. Use T* or T& instead (r.30).

I understand this much from the linked article:

Using a smart pointer type to pass data to a function indicates that the target function needs to manage the lifetime of the contained object. However, if the function only uses the smart pointer to access the contained object and never actually calls any code that may lead to its deallocation (that is, never affect its lifetime), there is usually no need to complicate the interface with smart pointers. A plain pointer or reference to the contained object is preferred.

All my function does is use the pointer like this:

throw_if_fail(pAvailability->SetDatesNotAvailable(arr.parray));

The other warning is:

C26418 530 Shared pointer parameter 'pAvailability' is not copied or moved. Use T* or T& instead (r.36).

I notice it says:

If shared pointer parameter is passed by value or reference to a constant object it is expected that function will take control of its target object’s lifetime without affecting of the caller. The code should either copy or move the shared pointer parameter to another shared pointer object or pass it further to other code by invoking functions which accept shared pointers. If this is not the case, then plain pointer or reference may be feasible.

Here is a snippet of my usage of the function (not real code as such):

MSAToolsLibrary::IAvailabilityPtr pAvailability = NULL;
throw_if_fail(pPublisher->get_Availability(&pAvailability));
if (pAvailability != NULL)
{
    std::vector<COleDateTime> vectorDates;
    const DWORD dwNumDates = InitDatesNotAvailableVector(vectorDates);
    theApp.MSAToolsInterface().SetPublisherDatesNotAvailable(pAvailability, vectorDates, dwNumDates);
}

I tried:

void SetPublisherDatesNotAvailable(T* pAvailability, std::vector<COleDateTime>& rVectorDates, DWORD dwNumDates);

And it said I had a syntax error.

Solution

This one is rather involved. While it's easy to explain the code analysis diagnostic, coming up with a mitigation strategy boils down to choosing between the lesser of several evils.

COM Fundamentals

COM models the concept of shared ownership. Every interface keeps track of the number of outstanding references, and the COM object implementing that interface is destroyed when the reference count reaches zero. Lifetime management is exposed through the IUnknown interface with its AddRef() and Release() members, that increment and decrement the reference count, respectively.

Smarter COM

While lifetime management can be implemented manually, explicitly accessing the IUnknown interface, this is as tedious as it is easy to get wrong. Smart pointer types (such as _com_ptr_t) fill in that gap by providing automatic resource management. To that end the _com_ptr_t class template implements the following five special member functions: copy and move constructors, copy and move assignment operators, and a destructor. Each of these add calls to AddRef() and Release() as appropriate¹. Those calls are invisible, so it's important to know that they are there.

Motivating Example

To keep this answer self-contained, let's write a simple program that outputs the RGB color of the desktop. It exhibits the same code analysis diagnostics but is sufficiently compact to illustrate possible solutions:

#include <ShObjIdl_core.h>
#include <Windows.h>
#include <comdef.h>
#include <comip.h>

#include <iostream>

_COM_SMARTPTR_TYPEDEF(IDesktopWallpaper, __uuidof(IDesktopWallpaper));

COLORREF background_color(IDesktopWallpaperPtr wallpaper)  // line 10
{
    COLORREF cr {};
    _com_util::CheckError(wallpaper->GetBackgroundColor(&cr));
    return cr;
}

int main()
{
    _com_util::CheckError(::CoInitialize(nullptr));

    IDesktopWallpaperPtr spWallpaper { nullptr };
    _com_util::CheckError(spWallpaper.CreateInstance(CLSID_DesktopWallpaper));

    auto const color { background_color(spWallpaper) };
    std::wcout << L"R: " << GetRValue(color)
               << L"\tG: " << GetGValue(color)
               << L"\tB: " << GetBValue(color) << std::endl;
}

Running this through code analysis produces the following output:

main.cpp(10): warning C26415: Smart pointer parameter 'wallpaper' is used only to access contained pointer. Use T* or T& instead (r.30).
main.cpp(10): warning C26418: Shared pointer parameter 'wallpaper' is not copied or moved. Use T* or T& instead (r.36).

Bear in mind that the code shown above is perfectly valid. There is nothing inherently wrong with it. Every expression is well defined, all resources are properly managed, and all error cases are handled.

Code Analysis, er, Analysis

C26418 is more important here (even though C26415 is advertised as the priority issue). The code analyzer recognizes _com_ptr_t as a smart pointer type but sees that none of the special functions (copy/move constructor, copy/move assignment) are used. It thus concludes that no lifetime management is happening through the smart pointer type, and recommends to use a raw pointer/reference instead.

The observation is accurate, and the suggestion is mostly fine, too, with just one wrinkle (see below).

Applying the Suggested Fix

The suggested fix is to "use T* or T& instead". With _com_ptr_t providing implicit conversion operators to Interface& and Interface* there's a choice we have to make. Luckily, this is one of the few choices with a clear winner.

Using T*

COLORREF background_color(IDesktopWallpaper* wallpaper)
{
    if (wallpaper)
    {
        COLORREF cr {};
        _com_util::CheckError(wallpaper->GetBackgroundColor(&cr));
        return cr;
    }
    else
    {
        _com_issue_error(E_POINTER);
    }
}

Using T&

COLORREF background_color(IDesktopWallpaper& wallpaper)
{
    COLORREF cr {};
    _com_util::CheckError(wallpaper.GetBackgroundColor(&cr));
    return cr;
}

The call site (background_color(spWallpaper)) doesn't need to be changed, and exhibits identical behavior with either function call. The function implementations are different owed to the different semantics of operator Interface&() and operator Interface*() of _com_ptr_t. The former performs a null pointer check, whereas the latter doesn't, passing that responsibility on to the function implementation.

Unless you have very specific reasons to allow passing a null pointer (e.g. if you require noexcept) the implementation taking an interface reference is superior: It establishes stronger preconditions, and fails fast if the preconditions are violated. Personally, I'm a big fan of particularly the latter.

Victory

So then, that solves both code analysis warnings. Everything's good, right? Right!?

Well, no. Not really. While the code analyzer is pretty good at identifying smart pointer types, it also makes the assumption, that all lifetime management is handled through the smart pointer's special member functions. _com_ptr_t does not establish that invariant, as can be seen here:

COLORREF background_color(IDesktopWallpaper& wallpaper)
{
    COLORREF cr {};
    _com_util::CheckError(wallpaper.GetBackgroundColor(&cr));
    wallpaper.Release();  // My name is Pointer, Dangling Pointer
    return cr;
}

There's no beating around the bush, this is bad. This function borrows a reference to a COM object, but then goes on to call wallpaper.Release() as if it owned it, decrementing the reference count (that was never upped!). In the sample above, this is the only reference, and when the caller (the main function) tries to clean up it accesses memory through a pointer that no longer points to valid memory. The best case scenario is an access violation, that eventually terminates the program. In a more complex program, however, this is an instant heap corruption bug.

Code analysis warnings, you ask? None. Arguably, this is a defect in the code analyzer that, while identifying _com_ptr_t as a smart pointer, plays the Alzheimer's card when it comes to recalling COM lifetime management semantics.

Solutions?

There aren't any, to my knowledge, that are all-around safe. Nothing that can be verified statically, and ages well with the code in face of future changes. My personal take is to always pass COM interface pointers wrapped behind smart pointers by value.

This is going to be controversial, since passing a _com_ptr_t by value is going to incur a copy, that's not always strictly required. Invoking the copy constructor causes a call to AddRef() on the contained interface pointer. On function exit the copy bound to the parameter is destroyed, and the destructor calls Release() on the contained interface pointer. This may seem like an artificial reference count bump, which is, again, not strictly required.

There are good reasons to do this still:

It enables local reasoning: Any function that accepts a _com_ptr_t by value owns a shared reference for the duration of the function call. It can freely copy it, or pass it along, and be confident that code outside the function cannot invalidate the interface pointer (modulo bugs).
It allows opting out of automatic resource management. Since the function owns a reference, it can, for example, Detach the interface pointer and pass it to someone that takes ownership.
It continues to work with C++20 coroutines. In the times before C++20 it was near impossible to invalidate references passed into functions. A reference to a local object was, for all intents and purposes, valid for the entire duration of a function call, regardless of how deep that call stack got. Coroutines change all that, and references can get invalidated at the first suspension point of a coroutine. Passing a smart pointer by value is immune to local object invalidation midway through a coroutine.

Personally, I would opt to disable the code analysis warnings here, and go with the following implementation:

#pragma warning(suppress : 26415 26418)
COLORREF background_color(IDesktopWallpaperPtr wallpaper)
{
    COLORREF cr {};
    _com_util::CheckError(wallpaper->GetBackgroundColor(&cr));
    return cr;
}

Mind you, this by itself, isn't all that great either. There's still the glaring issue that you can call wallpaper->Release(), leading to the same issue as with the version that takes an interface reference. And no code analysis rule to warn you.

There's still some manual work involved in making this safe, i.e. finding and removing all explicit calls to AddRef() and Release(). Neither one is required when using smart pointers (much like new and delete aren't), and either one is a potential bug.

Conclusion

The situation here isn't great, and there's no single solution that can address all concerns. There's a lot to read through, and even more to consider. And by the end of the day it's up to you to decide which poison to pick. Sorry.

¹ The move constructor and move assignment operator are special in that they transfer ownership. Neither one calls AddRef() or Release() on their respective argument, as there is no change in reference counts. The interface pointer is merely moved into a new home.