Windows Firewall C++ API - How to correctly clean up the COM resources?

Question

I am trying to use the COM-based Windows Firewall API for traversing the existing Firewall rules and find out if one specific rule exists among them.

Currently I have difficulties with understanding what is going on in the Cleanup part of this example (https://learn.microsoft.com/en-us/previous-versions/windows/desktop/ics/c-enumerating-firewall-rules):

/********************************************************************++
Copyright (C) Microsoft. All Rights Reserved.

Abstract:
    This C++ file includes sample code for enumerating Windows Firewall
    rules using the Microsoft Windows Firewall APIs.

********************************************************************/

#include <windows.h>
#include <stdio.h>
#include <comutil.h>
#include <atlcomcli.h>
#include <netfw.h>

#pragma comment( lib, "ole32.lib" )
#pragma comment( lib, "oleaut32.lib" )

#define NET_FW_IP_PROTOCOL_TCP_NAME L"TCP"
#define NET_FW_IP_PROTOCOL_UDP_NAME L"UDP"

#define NET_FW_RULE_DIR_IN_NAME L"In"
#define NET_FW_RULE_DIR_OUT_NAME L"Out"

#define NET_FW_RULE_ACTION_BLOCK_NAME L"Block"
#define NET_FW_RULE_ACTION_ALLOW_NAME L"Allow"

#define NET_FW_RULE_ENABLE_IN_NAME L"TRUE"
#define NET_FW_RULE_DISABLE_IN_NAME L"FALSE"


// Forward declarations
void        DumpFWRulesInCollection(INetFwRule* FwRule);
HRESULT     WFCOMInitialize(INetFwPolicy2** ppNetFwPolicy2);


int __cdecl main()
{
    HRESULT hrComInit = S_OK;
    HRESULT hr = S_OK;

    ULONG cFetched = 0; 
    CComVariant var;

    IUnknown *pEnumerator;
    IEnumVARIANT* pVariant = NULL;

    INetFwPolicy2 *pNetFwPolicy2 = NULL;
    INetFwRules *pFwRules = NULL;
    INetFwRule *pFwRule = NULL;

    long fwRuleCount;

    // Initialize COM.
    hrComInit = CoInitializeEx(
                    0,
                    COINIT_APARTMENTTHREADED
                    );

    // Ignore RPC_E_CHANGED_MODE; this just means that COM has already been
    // initialized with a different mode. Since we don't care what the mode is,
    // we'll just use the existing mode.
    if (hrComInit != RPC_E_CHANGED_MODE)
    {
        if (FAILED(hrComInit))
        {
            wprintf(L"CoInitializeEx failed: 0x%08lx\n", hrComInit);
            goto Cleanup;
        }
    }

    // Retrieve INetFwPolicy2
    hr = WFCOMInitialize(&pNetFwPolicy2);
    if (FAILED(hr))
    {
        goto Cleanup;
    }

    // Retrieve INetFwRules
    hr = pNetFwPolicy2->get_Rules(&pFwRules);
    if (FAILED(hr))
    {
        wprintf(L"get_Rules failed: 0x%08lx\n", hr);
        goto Cleanup;
    }

    // Obtain the number of Firewall rules
    hr = pFwRules->get_Count(&fwRuleCount);
    if (FAILED(hr))
    {
        wprintf(L"get_Count failed: 0x%08lx\n", hr);
        goto Cleanup;
    }

    wprintf(L"The number of rules in the Windows Firewall are %d\n", fwRuleCount);

    // Iterate through all of the rules in pFwRules
    pFwRules->get__NewEnum(&pEnumerator);

    if(pEnumerator)
    {
        hr = pEnumerator->QueryInterface(__uuidof(IEnumVARIANT), (void **) &pVariant);
    }

    while(SUCCEEDED(hr) && hr != S_FALSE)
    {
        var.Clear();
        hr = pVariant->Next(1, &var, &cFetched);

        if (S_FALSE != hr)
        {
            if (SUCCEEDED(hr))
            {
                hr = var.ChangeType(VT_DISPATCH);
            }
            if (SUCCEEDED(hr))
            {
                hr = (V_DISPATCH(&var))->QueryInterface(__uuidof(INetFwRule), reinterpret_cast<void**>(&pFwRule));
            }

            if (SUCCEEDED(hr))
            {
                // Output the properties of this rule
                DumpFWRulesInCollection(pFwRule);
            }
        }
    }

Cleanup:

    // Release pFwRule
    if (pFwRule != NULL)
    {
        pFwRule->Release();
    }

    // Release INetFwPolicy2
    if (pNetFwPolicy2 != NULL)
    {
        pNetFwPolicy2->Release();
    }

    // Uninitialize COM.
    if (SUCCEEDED(hrComInit))
    {
        CoUninitialize();
    }

    return 0;
}


// Output properties of a Firewall rule 
void DumpFWRulesInCollection(INetFwRule* FwRule)
{
    variant_t InterfaceArray;
    variant_t InterfaceString;  

    VARIANT_BOOL bEnabled;
    BSTR bstrVal;

    long lVal = 0;
    long lProfileBitmask = 0;

    NET_FW_RULE_DIRECTION fwDirection;
    NET_FW_ACTION fwAction;

    struct ProfileMapElement 
    {
        NET_FW_PROFILE_TYPE2 Id;
        LPCWSTR Name;
    };

    ProfileMapElement ProfileMap[3];
    ProfileMap[0].Id = NET_FW_PROFILE2_DOMAIN;
    ProfileMap[0].Name = L"Domain";
    ProfileMap[1].Id = NET_FW_PROFILE2_PRIVATE;
    ProfileMap[1].Name = L"Private";
    ProfileMap[2].Id = NET_FW_PROFILE2_PUBLIC;
    ProfileMap[2].Name = L"Public";

    wprintf(L"---------------------------------------------\n");

    if (SUCCEEDED(FwRule->get_Name(&bstrVal)))
    {
        wprintf(L"Name:             %s\n", bstrVal);
    }

    if (SUCCEEDED(FwRule->get_Description(&bstrVal)))
    {
        wprintf(L"Description:      %s\n", bstrVal);
    }

    if (SUCCEEDED(FwRule->get_ApplicationName(&bstrVal)))
    {
        wprintf(L"Application Name: %s\n", bstrVal);
    }

    if (SUCCEEDED(FwRule->get_ServiceName(&bstrVal)))
    {
        wprintf(L"Service Name:     %s\n", bstrVal);
    }

    if (SUCCEEDED(FwRule->get_Protocol(&lVal)))
    {
        switch(lVal)
        {
            case NET_FW_IP_PROTOCOL_TCP: 

                wprintf(L"IP Protocol:      %s\n", NET_FW_IP_PROTOCOL_TCP_NAME);
                break;

            case NET_FW_IP_PROTOCOL_UDP: 

                wprintf(L"IP Protocol:      %s\n", NET_FW_IP_PROTOCOL_UDP_NAME);
                break;

            default:

                break;
        }

        if(lVal != NET_FW_IP_VERSION_V4 && lVal != NET_FW_IP_VERSION_V6)
        {
            if (SUCCEEDED(FwRule->get_LocalPorts(&bstrVal)))
            {
                wprintf(L"Local Ports:      %s\n", bstrVal);
            }

            if (SUCCEEDED(FwRule->get_RemotePorts(&bstrVal)))
            {
                wprintf(L"Remote Ports:      %s\n", bstrVal);
            }
        }
        else
        {
            if (SUCCEEDED(FwRule->get_IcmpTypesAndCodes(&bstrVal)))
            {
                wprintf(L"ICMP TypeCode:      %s\n", bstrVal);
            }
        }
    }

    if (SUCCEEDED(FwRule->get_LocalAddresses(&bstrVal)))
    {
        wprintf(L"LocalAddresses:   %s\n", bstrVal);
    }

    if (SUCCEEDED(FwRule->get_RemoteAddresses(&bstrVal)))
    {
        wprintf(L"RemoteAddresses:  %s\n", bstrVal);
    }

    if (SUCCEEDED(FwRule->get_Profiles(&lProfileBitmask)))
    {
        // The returned bitmask can have more than 1 bit set if multiple profiles 
        //   are active or current at the same time

        for (int i=0; i<3; i++)
        {
            if ( lProfileBitmask & ProfileMap[i].Id  )
            {
                wprintf(L"Profile:  %s\n", ProfileMap[i].Name);
            }
        }
    }

    if (SUCCEEDED(FwRule->get_Direction(&fwDirection)))
    {
        switch(fwDirection)
        {
            case NET_FW_RULE_DIR_IN:

                wprintf(L"Direction:        %s\n", NET_FW_RULE_DIR_IN_NAME);
                break;

            case NET_FW_RULE_DIR_OUT:

                wprintf(L"Direction:        %s\n", NET_FW_RULE_DIR_OUT_NAME);
                break;

            default:

                break;
        }
    }

    if (SUCCEEDED(FwRule->get_Action(&fwAction)))
    {
        switch(fwAction)
        {
            case NET_FW_ACTION_BLOCK:

                wprintf(L"Action:           %s\n", NET_FW_RULE_ACTION_BLOCK_NAME);
                break;

            case NET_FW_ACTION_ALLOW:

                wprintf(L"Action:           %s\n", NET_FW_RULE_ACTION_ALLOW_NAME);
                break;

            default:

                break;
        }
    }

    if (SUCCEEDED(FwRule->get_Interfaces(&InterfaceArray)))
    {
        if(InterfaceArray.vt != VT_EMPTY)
        {
            SAFEARRAY    *pSa = NULL;

            pSa = InterfaceArray.parray;

            for(long index= pSa->rgsabound->lLbound; index < (long)pSa->rgsabound->cElements; index++)
            {
                SafeArrayGetElement(pSa, &index, &InterfaceString);
                wprintf(L"Interfaces:       %s\n", (BSTR)InterfaceString.bstrVal);
            }
        }
    }

    if (SUCCEEDED(FwRule->get_InterfaceTypes(&bstrVal)))
    {
        wprintf(L"Interface Types:  %s\n", bstrVal);
    }

    if (SUCCEEDED(FwRule->get_Enabled(&bEnabled)))
    {
        if (bEnabled)
        {
            wprintf(L"Enabled:          %s\n", NET_FW_RULE_ENABLE_IN_NAME);
        }
        else
        {
            wprintf(L"Enabled:          %s\n", NET_FW_RULE_DISABLE_IN_NAME);
        }
    }

    if (SUCCEEDED(FwRule->get_Grouping(&bstrVal)))
    {
        wprintf(L"Grouping:         %s\n", bstrVal);
    }

    if (SUCCEEDED(FwRule->get_EdgeTraversal(&bEnabled)))
    {
        if (bEnabled)
        {
            wprintf(L"Edge Traversal:   %s\n", NET_FW_RULE_ENABLE_IN_NAME);
        }
        else
        {
            wprintf(L"Edge Traversal:   %s\n", NET_FW_RULE_DISABLE_IN_NAME);
        }
    }
}


// Instantiate INetFwPolicy2
HRESULT WFCOMInitialize(INetFwPolicy2** ppNetFwPolicy2)
{
    HRESULT hr = S_OK;

    hr = CoCreateInstance(
        __uuidof(NetFwPolicy2), 
        NULL, 
        CLSCTX_INPROC_SERVER, 
        __uuidof(INetFwPolicy2), 
        (void**)ppNetFwPolicy2);

    if (FAILED(hr))
    {
        wprintf(L"CoCreateInstance for INetFwPolicy2 failed: 0x%08lx\n", hr);
        goto Cleanup;        
    }

Cleanup:
    return hr;
}

In particular, these lines confuse me:

    // Release pFwRule
    if (pFwRule != NULL)
    {
        pFwRule->Release();
    }

The pFwRule pointer gets overwritten on each iteration, so here we are explicitly Releaseing only the last Rule that was obtained via QueryInterface in the while loop above.

Releaseing a pointer obtained from a successfull call to QueryInterface is logical, because QueryInterface calls AddRef before returning (which is explicitly stated in the documentation).

But what I cannot understand, is:

1. Why don't we Release all the previously traversed Rules before querying a next one in the loop? Have they been released implicitly somewhere? Does QueryInterface call Release undercover in case a non-null pointer is passed to it?

2. Why don't we call Release on pFwRules? Doesn't the INetFwPolicy2::get_Rules function give us a new pointer to a COM object, which is AddRef'ed before being returned to us (and thus must be Released by the caller in the end)?

3. The same question about the pEnumerator pointer obtained from get__NewEnum: why don't we Release this one as well?

Answer 1

The code is indeed leaking COM memory.

Every call to an interface's AddRef() method must have a matching call to its Release() method. ANY function call that outputs an interface pointer must call AddRef() on it before exit, and the caller must then call Release() on it afterwards.

The general rule is, for any function that allocates and returns memory to the caller, the caller must free it when done using it.

So, to answer your questions:

• Yes, there are missing calls to Release() in this code, so there are COM interfaces being leaked - specifically: pFwRules, pEnumerator, and pFwRule are not being Release()'d properly.

• DumpFWRulesInCollection() is also leaking COM memory as well. It is not freeing any of the BSTR strings that are output by FwRule's methods. And also, when it calls SafeArrayGetElement() in a loop, it is not clearing the InterfaceString on each iteration.

• No, QueryInterface() does not implicitly Release() a non-null pointer. Just as SafeArrayGetElement() does not clear the element being written to.

Answer 2

It's a reasonable reaction to be confused when studying the sample code. It does indeed leak resources.

Why don't we Release all the previously traversed Rules before querying a next one in the loop? Have they been released implicitly somewhere? Does QueryInterface call Release undercover in case a non-null pointer is passed to it?

No. QueryInterface unconditionally overwrites the value pointed to by its ppvObject argument, either with a NULL pointer, if the COM object does not implement the requested interface, or with a pointer to the requested interface. Not calling Release is a resource leak.

Why don't we call Release on pFwRules? Doesn't the INetFwPolicy2::get_Rules function give us a new pointer to a COM object, which is AddRef'ed before being returned to us (and thus must be Released by the caller in the end)?

Correct again. get_Rules returns a resource the caller is responsible for. Not calling Release on the returned interface is a resource leak.

The same question about the pEnumerator pointer obtained from get__NewEnum: why don't we Release this one as well?

The same rules apply here as well: The caller is responsible for cleaning up the iterator it received. This, too, is a resource leak.

---

Special note on MSDN samples: Although they are tagged "C++", the majority of code samples for COM are actually written in C. Unlike C++, C doesn't have much to offer with respect to automatic resource management.

If you are using C++, you can take advantage of automatic resource management, and employ one of the provided smart pointer types (e.g. ATL's CComPtr, or Visual C++' _com_ptr_t).