Preventing bugs, and improving code quality with Microsoft SAL (Part 2, custom preconditions for structs & objects)

Note: At the end of part 1, I’d suggested that part 2 would be about invalid handles. This post however isn’t about invalid handles. That’ll be the next part in this series.

I very frequently come across code like this:

void dosomething( _In_ somestruct* in ) {
    //Not detected at compile time
    assert(in->some_precondition);
    }

The function, dosomething, expects/requires a specific condition. So, the author asserts it, and resolves to test every possible code path, thus verifying code correctness. When all their tests pass, they build in release mode, and the assert disappears.

Some time later, someone calls dosomething under different conditions, all of their tests pass, and they release the modified code.  Users start seeing their data mysteriously corrupt itself, and their once-reliable program is crashing in ways that nobody can reproduce. After several days worth of investigation, developers thoroughly audit the code, and spot the new (and incorrect) code path. They refactor the faulty code path, add a VERY LOUD comment to dosomething, and forget about the whole thing.

 

Some number of months later, the same thing happens. This time, a developer has a bright idea: Let’s return an error code! Whoever wrote the code without a proper check for invalid arguments is an idiot!

The function now looks like this:

 

HRESULT dosomething( _In_ somestruct* in ) {
    //Not detected at compile time
    assert(in->some_precondition);
    if ( !in->some_precondition ) {
        return E_INVALIDARG;
        }
    }

We’re better off now, or at least we think so.

Now of course, everybody has to check the return code, which means more complexity, more ways to go wrong, and more code paths to test. Since there are far too many code paths to test every single one, some end user might still get to one of the untested, and also invalid, path.

Yet a third developer comes along, and thinks: What a silly C programmer. Someone will inevitably ignore the return value and the error will go unnoticed. Even worse, because I have to check the return value every single time, I can’t compose this function, and thus it’s less practical. This is an exceptional case, so we should treat it as one.

The function now looks like this:

void dosomething( _In_ somestruct* in ) {
    //Not detected at compile time
    assert(in->some_precondition);
    if ( !in->some_precondition ) {
        throw std::invalid_argument( "Function preconditions not met!" );
        }
    }

 

We’re a little bit better off now, at least we’re pretty sure that we are.

Now, exception safety becomes an issue. We still have to test every possible code path to make sure that none of them incorrectly call dosomething. We still can’t be confident about it.

Perhaps somebody else comes along, and writes a wrapper object, thus only “allowing” correct uses of dosomething. Now everybody has to rewrite their code use that wrapper.

Complexity has increased, this function has been refactored thrice, and we still haven’t even found a way to be code-correctness-confident, at compile-time, for all the uses of dosomething.

The cycle repeats, no end in sight.

Nearly every program stops there – they choose error codes or C++ exceptions, to carefully handle bad logic WHEN it happens. Preventing them from happening, well that becomes an issue of “professionalism” and “discipline”.

 

Neither “professionalism” nor “discipline”, actually prevent the mistakes of logic, but blaming the mistake on a given individual’s moral or personal failings works nicely to…well I’m not actually sure; I have noticed it is a fairly pervasive and pathological mindset in case studies of engineering disasters (Therac-25, Toyota electronic throttle malfunctions, others). But, I digress.

 A few examples from Microsoft’s headers

One of Microsoft’s motivating factors in developing SAL, was the infamous blue screen. Drivers. particularly kernel-mode drivers, are extremely sensitive to misuse.  A failure in the kernel can mean system instability, serious security vulnerabilities, or massive data corruption. Furthermore, the kernel APIs that drivers rely on are often very complex, interacting with many components of hardware and software – and thus very hard to get right.

Bluescreens are extremely visible, and have a tremendous impact on overall user satisfaction and trust, thus Microsoft needed a way for driver developers to validate their own code, in a very scalable and reliable manner.

Functions like ExAllocatePoolWithTag, are heavily annotated (from km/wdm.h, 8.1 SDK):

__drv_allocatesMem(Mem)
_When_((PoolType & PagedPool) != 0, _IRQL_requires_max_(APC_LEVEL))
_When_((PoolType & PagedPool) == 0, _IRQL_requires_max_(DISPATCH_LEVEL))
_When_((PoolType & NonPagedPoolMustSucceed) != 0,
       __drv_reportError("Must succeed pool allocations are forbidden. "
                         "Allocation failures cause a system crash"))
_When_((PoolType & (NonPagedPoolMustSucceed |
                    POOL_RAISE_IF_ALLOCATION_FAILURE)) == 0,
       _Post_maybenull_ _Must_inspect_result_)
_When_((PoolType & (NonPagedPoolMustSucceed |
                    POOL_RAISE_IF_ALLOCATION_FAILURE)) != 0,
       _Post_notnull_)
_Post_writable_byte_size_(NumberOfBytes)
NTKERNELAPI
PVOID
NTAPI
ExAllocatePoolWithTag (
    _In_ __drv_strictTypeMatch(__drv_typeExpr) POOL_TYPE PoolType,
    _In_ SIZE_T NumberOfBytes,
    _In_ ULONG Tag
 );

The When annotations are specifying the precise hardware conditions where Developers can call ExAllocatePoolWithTag. Calling outside the correct context will result in an immediate bluescreen, saying something similar to the IRQL_NOT_LESS_OR_EQUAL that we’ve all seen at some point.

 

Another interesting function is IoBuildSynchronousFsdRequest, which is interesting because the In/Out annotation is conditional based on whether the function is reading or writing data. From km/wdm.h, 8.1 SDK:


_When_(MajorFunction == IRP_MJ_WRITE, _At_(Buffer, _In_))
_When_(MajorFunction == IRP_MJ_READ, _At_(Buffer, _Out_))
_When_(MajorFunction != IRP_MJ_READ && MajorFunction != IRP_MJ_WRITE, _At_(Buffer, _Pre_null_))
_Must_inspect_result_
__drv_aliasesMem
_IRQL_requires_max_(PASSIVE_LEVEL)
NTKERNELAPI
PIRP
IoBuildSynchronousFsdRequest(
    _In_ ULONG MajorFunction,
    _In_ PDEVICE_OBJECT DeviceObject,
    PVOID Buffer,
    _In_ ULONG Length,
    _In_opt_ PLARGE_INTEGER StartingOffset,
    _In_ PKEVENT Event,
    _Out_ PIO_STATUS_BLOCK IoStatusBlock
 );

Rolling your own preconditions

I think that’s enough ugly header code. Back to contrived examples!

The code we had before was as follows:

void dosomething( _In_ somestruct* in ) {
    //Not detected at compile time
    assert(in->some_precondition);
    if ( !in->some_precondition ) {
        throw std::invalid_argument( "Function preconditions not met!" );
        }
    }

I personally don’t like exceptions that much, and in the event of an actual invalid parameter, I’d rather log some debugging info and std::terminate( ).

Here, we’ll avoid catching exceptions (I typically avoid catching them anyways), and thus the code will be cleaner. You’ll be able to see the source on GitHub, at the end of the article.

The problem code from the beginning of this post is really easy check with SAL. The annotated function is:

_Pre_satisfies_( in->flag )
void dosomething( _In_ somestruct* in ) {
    //Not detected at compile time
    assert(in-> flag);
    if ( !in->flag ) {
        throw std::invalid_argument( "Function preconditions not met!" );
        }
    }

When I compile this with Visual Studio 2013 Code Analysis, I get a warning:

C28020 <strong>The expression is not true at this call</strong>
The expression '_Param_(1)->flag' is not true at this call.

codeanalysis_part2

Clicking the warning highlights the callsite, which is really handy on a large codebase.

If I want to, I can even configure Visual Studio to consider the build a failure if it detects C28020 – although it’s a bit of a counter-intuitive trick to enable.

You’ll have to open the ruleset in the project properties window:

codeanalysis_part2_open_ruleset

 

Then, find the rule that you’d like to change:

codeanalysis_part2_modify_ruleset

Lastly, hit save (I’m a keyboard person, so I hit CTRL + S).

 


 

Now let’s step it up just a notch.

 

Say the function has been in use for several years, and has grown a whole bunch of ugly hairs. It now conditionally assigns to/from a pass-by-pointer parameter. Nobody remembers why this ability was added, but the function is used a few thousand times across the whole codebase. You’re not in a good position to fix it, so you decide to make it more robust, and aggressively detect incorrect uses.

This does happen in real world scenarios (mirror: Bruce Dawson hits thousands of warnings in Chrome, Twitter). And SAL can help.

Here’s the new version of the function:

_Pre_satisfies_( in->flag )
void dosomethingelse( _In_ somestruct* const in, int* const some_number ) {
    //Not detected at compile time
    assert(in-> flag);
    if ( !in->flag ) {
        throw std::invalid_argument( "Function preconditions not met!" );
        }
    if ( in->member <= 0 ) {
        ( *some_number ) = in->member;
        }
    else {
        in->member = ( *some_number );
        }
    }

We still have the Pre_satisfies( in->flag ) precondition, but we have this other damned variable (at least it’s a const pointer!).

So, here’s what we know, in addition to the in->flag precondition:

  1. when in->member <= 0, the parameter `some_number` is written to.
  2. when in->member   > 0, the parameter `some_number` is read from.

Thankfully, we can express this almost literally in SAL. The annotations that we need to add:

_When_( in->member > 0, _At_( some_number, _In_ ) )
_When_( in->member <= 0, _At_( some_number, _Out_ ) )

The final function looks like this:

_Pre_satisfies_( in->flag )
_When_( in->member > 0, _At_( some_number, _In_ ) )
_When_( in->member <= 0, _At_( some_number, _Out_ ) )
void dosomethingelse( _In_ somestruct* const in, int* const some_number ) {
    //Not detected at compile time
    assert(in-> flag);
    if ( !in->flag ) {
        throw std::invalid_argument( "Function preconditions not met!" );
        }
    if ( in->member <= 0 ) {
        ( *some_number ) = in->member;
        }
    else {
        in->member = ( *some_number );
        }
    }

Sure enough, when I compile this with some test code, I get C6001: Using uninitialized memory.

codeanalysis_part2_final_test

Final thoughts

  • I  want to remind you that while SAL helps verify the correctness of your code, it is not a substitute for proper error/exception handling.  Although a properly annotated API will make VS yell at you for ignoring return values, and passing invalid parameters, people can (and will) still (void) those return values away.
  • Thoroughly annotating a codebase with SAL seems to have a tremendous impact on the run time of code analysis. (It’s MUCH faster)

Resources

OSR, a good resource of information on SAL since the stone age, has a great article: SAL Annotations Don’t Hate Me Because I’m Beautiful – OSR

I’ve, of course, mirrored it: SAL Annotations_ Don’t Hate Me Because I’m Beautiful – OSR

Also: I’ve been poorly curating a repository of SAL example usage – at the moment it’s mostly from the Windows headers, but I’m looking for other examples.

You can find it here: https://github.com/ariccio/SALExamples

You can find the source code for this article in that repository, but here’s a direct link: https://github.com/ariccio/SALExamples/blob/master/StructMember/StructMember/structmember.cpp

~ by Alexander Riccio on April 2, 2015.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

 
Lucky's Notes

Notes on math, coding, and other stuff

AbandonedNYC

Abandoned places and history from the five boroughs and beyond.

Open Mind

Science, Politics, Life, the Universe, and Everything

I learned it. I share it.

A software engineering blog by György Balássy

Untapped Cities

Rediscover your city: Urban discovery and exploration in NYC and around the world

Threatpost

The First Stop For Security News

Bit9 + Carbon Black Blog

#ArmYourEndpoints

The Electric Chronicles: Power in Flux

If someone ever tells you that you don't need more power, walk away. You don't need that kind of negativity in your life.

Ted's Energy Tips

Practical tips for making your home more comfortable, efficient and safe

love n grace

feel happy, be happy

Recognition, Evaluation, Control

News and views from Diamond Environmental Ltd.

greg tinkers

Sharing the successes and disasters.

Sam Thursfield's Blog

I want music in my life not questions!

Always In Motion | SAE International

A Safe, Green, Connected Blog from SAE International

Cranraspberry Blog

Sharing the things I love

Biosingularity

Advances in biological systems.

The Embedded Code

Designing From Scratch

Sean Heelan's Blog

Program analysis, verification and security

EduResearcher

Connecting Research, Practice, and Advocacy in Education

Popehat

A Group Complaint about Law, Liberty, and Leisure

Warner Stellian Appliance

Home & Kitchen Appliance Blog

Bad Science Debunked

Debunking dangerous junk science found on the Internet. Non-scientist friendly!

4 gravitons

The trials and tribulations of four gravitons and a postdoc

Strange Quark In London

A blog about physics, citylive and much procastination

The Lumber Room

"Consign them to dust and damp by way of preserving them"

In the Dark

A blog about the Universe, and all that surrounds it

andrea elizabeth

passionate - vibrant - ambitious

Probably Dance

I can program and like games

a totally unnecessary blog

paolo severini's waste of bandwidth

Musing Mortoray

Programming and Life

PJ Naughter's space

Musings on Native mode development on Windows using C++

  Bartosz Milewski's Programming Cafe

Concurrency, C++, Haskell, Category Theory

Brandon's Thoughts

Thoughts on programming

David Crocker's Verification Blog

Formal verification of C/C++ code for critical systems

Fusion

Championing a young, diverse, and inclusive America with a unique mix of smart and irreverent original reporting, lifestyle, and comedic content.

10 Minute Astronomy

Stargazing for people who think they don't have time for stargazing.

One Dev Job

notes of an interactive developer

Enterprise Architect, IoT, Cloud, Mobile Apps, Technology Evangelist, Technical Pre-Sales, Business Evangelist, Speaker

Coder/Architect for IoT, Cloud Technologies and Mobile Apps, Azure Cloud, Amazon Cloud, Windows Phone 10 Apps, iPhone Apps, Scrum Master, Business Evangelist, Mobile apps developer in iOS and Windows 10 UWP, Azure IoT Hub, Machine Learning, Stream Analytics, Azure Mobile Service, APM Tools

The Angry Technician

No, the Internet is not broken.

Kenny Kerr

Author • Systems programmer • Creator of C++/WinRT • Engineer on the Windows team • Romans 1:16

IT affinity!

The Ultimate Question of Life, the Universe, and Everything is answered somwhere else. This is just about IT.

%d bloggers like this: