New Safety Rules in C++ Code Analysis

Hwi-sung Im

October 2nd, 202013 0

In Visual Studio version 16.8 Preview 3,  we are adding a few safety rules to C++ Code Analysis that can find some common mistakes, which can lead to bugs ranging from simple broken features to costly security vulnerabilities. These new rules are developed around issues discovered in production software via security reviews and incidents requiring costly servicing. Every shipping piece of software in Microsoft runs these rules as part of security and compliance requirements.

This blog post will introduce new rules related to VARIANT and its sibling types – such as VARIANTARG, or PROPVARIANT. To help with the new rules, we have built a code analysis extension, called VariantClear, that detects violations of these new rules in code.  It is named VariantClear because the primary rule it detects is about misuse of VariantClear function.

The VariantClear extension detects and reports the following warnings:

  • C33001: VARIANT ‘var’ was cleared when it was uninitialized 
  • C33004: VARIANT ‘var’, which is marked as Out was cleared before being initialized  
  • C33005: VARIANT ‘var’ was provided as an input or input/output parameter but was not initialized 

While Visual Studio version 16.8 Preview 3 already has the VariantClear extension included, it is not yet enabled by default. To enable this extension, please add the following lines either to your project file or to the Microsoft.CodeAnalysis.Extensions.props file under MSBuild\Microsoft\VC\v160 folder in the Visual Studio installation location:

If you want to add this to individual project file, add it after all other <PropertyGroup> elements:

<PropertyGroup Condition="'$(ConfigurationType)'!='Utility' and '$(ConfigurationType)'!='Makefile'">
    <EspXtensions Condition="'$(EnableVariantClear)'!='false'">VariantClear.dll;$(EspXtensions)</EspXtensions>
</PropertyGroup>

If you want to modify your Visual Studio installation, you can add this to the Microsoft.CodeAnalysis.Extensions.props file, after the similar element for HResultCheck:

<EspXtensions Condition="'$(EnableVariantClear)'!='false'">VariantClear.dll;$(EspXtensions)</EspXtensions>

Please note that this will likely be overwritten if you repair or reinstall Visual Studio, or upgrade to a later release. Please stay tuned for update when we have this extension enabled in Visual Studio.

VariantClear Rules 

VARIANT is a very convenient structure, allowing exchange of many different types of data using a single struct type. At any given time, it can hold either one of the alternative types, or no value. Type of the contained data or the fact that it contains no value is identified by the VARIANT::vt member. 

A VARIANT object needs to be explicitly initialized before use or passed to some other code. Otherwise, this will cause random data to be accessed and used, causing different problems depending on what is accessed and how it is used. 

A VARIANT object also needs to be cleared when it is no longer needed. Otherwise, it can leave some resources behind, leaking resources or letting others mistakenly access and use the resource after its intended lifetime. 

Initialization of a VARIANT object is usually done through calling VariantInit function. Clean up of a VARIANT object is mostly done through calling VariantClear function. 

There are some wrapper types for VARIANT struct to make it easier and safer to use, e.g. CComVariant  and _variant_t. Their default constructors initialize the instances being created and mark them as having no value, usually by calling VariantInit, passing the current instance. Their destructors clear the instances being destructed and mark them as having no value, usually by calling VariantClear, passing the current instance. 

VariantClear rules try to enforce the general rules of proper initialization of VARIANT instances before their use, including cleaning them up. 

Warning C33001 

This warning is triggered when an uninitialized VARIANT is passed into an API that clears a VARIANT such as VariantClear. These APIs expect the VARIANT is initialized before they can be cleared. Unfortunately, developers often forget this step.

Here is a simplified example: 

#include <Windows.h> 

HRESULT foo(bool some_condition) 
{
    VARIANT var; 
  
    if (some_condition) 
    { 
        //... 
        VariantInit(&var); 
        //... 
    } 
    VariantClear(&var);     // C33001 
}

This code will trigger a C33001 warning because the VARIANT var is conditionally initialized only if some_condition is true. If the condition is false, it will not be initialized when it is passed to VariantClear function. To fix this problem, we have to make sure that we are calling VariantClear only for the VARIANTs that have been initialized: 

#include <Windows.h> 

HRESULT foo(bool some_condition) 
{ 
    VARIANT var; 
  
    if (some_condition) 
    { 
        //... 
        VariantInit(&var); 
        //... 
        VariantClear(&var);     // C33001 
    } 
}

Warning C33004 

This warning is triggered when a VARIANT parameter with _Out_ SAL (source-code annotation language) annotation, which may not be to be initialized on input, is passed to an API such as VariantClear that expects an initialized VARIANT. 

A parameter that is annotated as _Out_ is not required to have been initialized when calling the function. It will be initialized upon return from the function. For more details of SAL annotations, please refer to SAL Annotations. 

During code analysis, an _Out_ VARIANT parameter is assumed to be uninitialized, to be on the safer side. If this parameter is passed to a function such as VariantClear that expects an initialized VARIANT object, it will try to clean up or use a random type of data, possibly at random memory location. Here is a simplified example: 

#include <Windows.h> 
  
HRESULT t2(_Out_ VARIANT* pv) 
{ 
    // ...... 
    VariantClear(pv);   // C33004. pv is assumed uninitialized. 
    // ...... 
  
    return S_OK; 
}

To fix this problem, we have to make sure to initialize the _Out_ VARIANT parameter before using it or passing it to another function that expects an initialized VARIANT instance: 

#include <Windows.h> 
  
void t2(_Out_ VARIANT* pv) 
{ 
    VariantInit(pv); 
    // ...... 
    VariantClear(pv);   // OK 
    // ...... 
}

Warning C33005 

This warning is triggered when an uninitialized VARIANT is passed to a function as input only or input/output parameter – for example, a parameter of const VARIANT* type. Here is an example: 

#include <Windows.h> 
  
void bar(VARIANT* v);   // v is assumed to be input/output 
  
void foo() 
{ 
    VARIANT v; 
    bar(&v);            // C33005 
    // ...... 
    VariantClear(&v);   // OK, assumed to be initialized by bar 
}

Please note that the checker assumes a function that takes a non-const VARIANT* parameter would initialize the VARIANT object upon return from the function, to avoid generating noisy warnings. 

Again, to fix this problem, we simply need to make sure to initialize the VARIANT object before passing it to another function as an input-only or input-output parameter: 

#include <Windows.h> 
  
void bar(VARIANT* v);   // v is assumed to be input/output 
  
void foo() 
{ 
    VARIANT v; 
    VariantInit(&v); 
    bar(&v);            // OK 
    // ...... 
    VariantClear(&v);   // OK, assumed to be initialized by bar 
} 

With the understanding of C33005 rule, it should be clearer why C33004 is reported only for an output-only (that is, annotated with _Out_ SAL annotation) parameter. For an input-only or input-output parameter, passing an uninitialized VARIANT will be a violation of rule C33005. 

Enabling new rules in Visual Studio 

You can enable these rules in Visual Studio as follows by selecting different ruleset for your project: 

Rule ID  Extension  Native Minimum Rules  Native Recommended Rules  All Rules 
C33001  VariantClear  X  X  X 
C33004  VariantClear     X  X 
C33005  VariantClear     X  X 

 

Give us your feedback 

Check out these newly added rules and let us know if they help you write safer C++. Stay tuned as we add more safety rules in future releases of Visual Studio. 

Download Visual Studio 2019 version 16.8 Preview 3 today and give it a try. We would love to hear from you to help us prioritize and build the right features for you. We can be reached via the comments below, Developer Community, and Twitter (@VisualC). The best way to file a bug or suggest a feature is via Developer Community. 

 

Hwi-sung Im Senior software engineer at Microsoft, C++ Static Analysis Team

Read next

13 comments

Discussion is closed. Login to edit/delete existing comments.

  • Alexander Riccio 0

    So, would you say that SAL is not dead yet? 🙂

    Very glad to see that you guys are continuously adding new checks and improving on code analysis. Maybe there is hope in the world for safer programs.

  • Mike Diack 0

    I tried this with VS 2019 Update 8 Preview 3.2 with both native recommended rules and all rules – I didn’t see any warnings…
    Am I do something wrong, or is the checker not actually in the Update 8 Preview 3 build?

    I can send the solution zipped up if you need to take a look?

    Bug raised at:
    https://developercommunity.visualstudio.com/content/problem/1208887/new-variant-checks-in-c-code-analysis-vs-2019-upda.html

    • Mike Diack 0

      Can you confirm if I’m right – that the checks don’t actually work? I’ve had no feedback from the bug triage team?

      • Hwi-sung ImMicrosoft employee 0

        Thanks a lot for trying out the feature and bringing the issue to our attention. It is verified that the feature is still not enabled by default. The change to enable has been missing in the released product. I will come up with a workaround until we have the fix added to the product in an upcoming release.

        • Hwi-sung ImMicrosoft employee 0

          We are working on enabling VariantClear extension by default. Meanwhile, a workaround for enabling VariantClear extension for build-time, compile-time, and menu-invoked code analysis is added to the body of the blog post, in the introduction section. Please give it a try and let me know if it works for you. Background Code Analysis requires a binary file update, and this workaround won’t enable VariantClear for Background Code Analysis.

          • Mike Diack 0

            The workaround of placing a PropertyGroup entry directly in the .vcxproj file does work for me. Thanks for the workaround.

  • Mike Diack 0

    I can confirm the workaround of using a PropertyGroup in the .vcxproj file does work. Thanks for the workaround.

  • Koby Kahane 0

    C33005 appears to have poor support for _variant_t, making it noisy and nearly unusable.
    Given the following code:

    #include <comutil.h>
#include <wbemcli.h>

static _variant_t getVariant()
{
    _variant_t v(123);
    return v;
}

static bool f(IWbemClassObject* wco)
{
    _variant_t v = getVariant();

    HRESULT hr = wco->Put(L"foo", 0, v.GetAddress(), 0);

    return SUCCEEDED(hr);
}

    A C33005 warning is incorrectly issued:
    warning C33005: VARIANT ‘&v’ was provided as an _In_ or _InOut_ parameter but was not initialized (expression ‘&v’)

    • Koby Kahane 0

      Another case:

      static _variant_t f1()
{
    _variant_t v;
    return v;
}

static _variant_t f2()
{
    _variant_t result = f1();
    return result;
}

      results in a C33005 for the line with “return result;”.

      • Hwi-sung ImMicrosoft employee 0

        Thanks a lot for brining this to our attention. These are very valuable feedback to make our toolset better. To track the issue better, could you please create a feedback ticket in our Developer Community, in the C++ space?
        https://developercommunity.visualstudio.com/spaces/62/index.html

        • Alex Ivanoff 0

          We are having the same issue with _variant_t. I submitted an issue, see C33005 false positive with _variant_t.

  • Keannu Cruz 0

    I am searching for some good research blog pages. I searched through the search engines and found a website for your blog.http://www.gadsdenhealth.org/

  • Juan Feeney 0

    It’s a little bit complicated at first. But, when I read all of it. It was very informative. Good thing my friend from “Airport Service” told me this forum.

Feedback usabilla icon