Reaping the Benefits of Static Analysis Tools

Our team has a huge amount of legacy test driver code, much of it rather old, and originally created by somewhat less experienced developers.  I continually find myself going back and performing maintenance on that code, and as I've noted previously- Static Analysis Tools are a great help in identifying problems in this code.  I'll put this "down time" (among other things, I've got PFD analyzing 40+ files that are rather large as I write this) to good use by describing both some further benefits and showing how easy it is to annotate your own code so you can recognize this sort of benefit.

Irql Tracking Finds Yet Another Flaw

One of the benefits PFD adds above regular PreFast is tracking IRQL changes in your code.  Of course, there are additional annotations used to support this, and I found them to be most useful in a very real-world way.

In our tests of the KMDF DDI, we try to make a lot of invalid calls to assess how well the framework verifier works.  One of the things that we do is make calls at improper IRQL levels- since efficiency suggests you usually have one place that tests a particular call, this change has to be programmable.  Not the sort of thing you expect a static analysis tool to be able to do for you, eh?  Well one of our SDETs made an initial pass at this issue [after I told the team we were going to make all of our test drivers PFD clean over the next few months, instead of having me piecemeal it as I fixed bugs as I had done previously], and encountered some difficulties

The original declarations looked like this:

/*******************************************************************************
 KmdfTestRaiseAndPrintIRQL

 Synopsis - Raises IRQL to DISPATCH_LEVEL and prints IRQL

 Parameters -
   eIRQL    -   Enumerator for IRQL
   OldIRQL  -   IRQL prior to raising to DISPATCH_LEVEL

******************************************************************************/
VOID 
KmdfTestRaiseAndPrintIRQL(
    __in    ULONG   eIRQL,
    __out   KIRQL   *OldIRQL
    );

Initial code- nice enough, and PreFast annotated, but it changes IRQL and doesn't say a thing about that other than in comments

There was no declaration for a function to lower IRQL- typically eIRQL was compared to a constant and KeLowerIrql was called when needed.  Well, PFD quickly noticed this function elevates IRQL on some paths, so the attempt was made to annotate it, and a paired restore function was also defined- thus:

/*******************************************************************************
 KmdfTestRaiseAndPrintIRQL

 Synopsis - Raises IRQL to DISPATCH_LEVEL and prints IRQL

 Parameters -
   eIRQL    -   Enumerator for IRQL
   OldIRQL  -   IRQL prior to raising to DISPATCH_LEVEL

******************************************************************************/
__drv_raisesIRQL(DISPATCH_LEVEL)
VOID 
KmdfTestRaiseAndPrintIRQL(
    __in    ULONG   eIRQL,
    __out   __drv_out_deref(__drv_savesIRQL) PKIRQL   OldIRQL
    );

/*******************************************************************************
 KmdfTestRestoreIRQL

 Synopsis - Restore IRQL

 Parameters -
   eIRQL    -   Enumerator for IRQL
   OldIRQL  -   IRQL returned from KmdfTestRaiseAndPrintIRQL

******************************************************************************/
VOID
KmdfTestRestoreIRQL(
    __in    ULONG   eIRQL,
    __in __drv_restoresIRQL __drv_nonConstant KIRQL OldIRQL
    );

First attempt- this says IRQL is changed and a restore function is added, but is it enough?

However, this didn't work out so well, when the implementation of these routines was analyzed by PFD, and since I had mandated cleanliness, warnings from PFD were suppressed, thus:

__drv_raisesIRQL(DISPATCH_LEVEL)
VOID __stdcall
#pragma prefast(suppress:__WARNING_IRQL_NOT_SET, "Intentionally save irql at specified condition");
KmdfTestRaiseAndPrintIRQL(
    __in    ULONG   eIRQL,
    __out   __drv_out_deref(__drv_savesIRQL) PKIRQL   OldIRQL
    )

// and then this

VOID __stdcall
#pragma prefast(suppress:__WARNING_IRQL_NOT_USED, "Intentionally restore irql at specified condition");
KmdfTestRestoreIRQL(
    __in    ULONG   eIRQL,
    __in __drv_restoresIRQL __drv_nonConstant KIRQL OldIRQL
    )

These annotations must not be enough, because PFD is smart enough to find paths that put the lie to the annotated effects of these functions

Why the need to suppress?  Because PFD correctly analyzed that there were paths through the code that did not do what the annotations say these functions do (raise IRQL to dispatch and restore it, respectively). These actions were conditional, and the annotations didn't reflect this.

Conditional Annotations: One Powerful Tool Makes One Effective Solution

I encountered the problem in different form.  I found myself working with the earliest of our drivers, written before these library routines were used.  They defined similar functions inline,   At first, I made these same annotations there [but didn't suppress], as that let me borrow from the work already done.  But since I didn't suppress, I got the identical errors.

When I realized why I was getting errors, I recalled reading about conditional annotations, so I read up on them and really, for a case like this they just weren't that hard to figure out.  I fixed the code I was working with, and then when I was sure this was the right thing to do, I made the same fix to the library code and header file I snipped above.  The end result looked like this (and I removed the suppression pragma from the library code, of course).

/*******************************************************************************
 KmdfTestRaiseAndPrintIRQL

 Synopsis - Raises IRQL to DISPATCH_LEVEL and prints IRQL

 Parameters -
   eIRQL    -   Enumerator for IRQL
   OldIRQL  -   IRQL prior to raising to DISPATCH_LEVEL

******************************************************************************/
__drv_when( eIRQL == EXECUTION_LEVEL_DISPATCH, __drv_raisesIRQL(DISPATCH_LEVEL))
VOID __stdcall
KmdfTestRaiseAndPrintIRQL(
    __in    ULONG   eIRQL,
    __out __drv_when( eIRQL == EXECUTION_LEVEL_DISPATCH, __drv_out_deref(__drv_savesIRQL))
            PKIRQL  OldIRQL
    );

/*******************************************************************************
 KmdfTestRestoreIRQL

 Synopsis - Restore IRQL

 Parameters -
   eIRQL    -   Enumerator for IRQL
   OldIRQL  -   IRQL returned from KmdfTestRaiseAndPrintIRQL

******************************************************************************/
VOID __stdcall
KmdfTestRestoreIRQL(
    __in    ULONG   eIRQL,
    __in __drv_when( eIRQL == EXECUTION_LEVEL_DISPATCH, __drv_restoresIRQL)
            KIRQL   OldIRQL
    );

Adding conditional annotations lets PFD correctly analyze the functions and code that uses them, removing the need to suppress perfectly valid warnings.

It also more clearly states how the function works in a fairly readable way.

The annotations are now more precise.  EXECUTION_LEVEL_DISPATCH is defined in a header always known when this is included, so PFD can properly analyze all usages.  It now knows that we change and restore when the eIRQL parameter is set to this value, and leave it alone otherwise.

Reaping The Benefits, Part 1- Finding a Real Bug at Build Time

Well, it didn't take long for it to turn up a real flaw [in one of those 40 files I alluded to earlier].  This is pseudo-code illustrating what it found.

KmdfTestRaiseAndPrintIRQL(
    eIRQL, 
    &OriginalIRQL
    );

if (condition) {
    MakeTheCall();
    KmdfTestRestoreIRQL(
        eIRQL,
        OriginalIRQL
        );
}
Pseudocode for a bug the above annotations exposed- found by PFD in that ancient test code.

Out of the 10's of thousands of source lines I'm working with, I don't think I could have noticed that- but PFD zeroed right in on it.  If the condition isn't met, we can raise IRQL without lowering it.  It's rare for the condition to not occur, which is one reason why nobody had found this before [another is that intermittent failures in these tests used to be routinely overlooked- but I don't work that way- I only give up when I absolutely have to].  Easily fixed, of course, just move the raise inside the conditional block as well.

Reaping the Benefits, Part 2- Clearer Documentation of Test Practices

By requiring the code to be clean (no errors or warnings) we have to use suppression in those places where we deliberately break the rules.  Now I could call this an annoyance, but actually I think it makes it a lot easier to understand better how we are testing the DDI in these cases.  So I'll leave this final sample:

        TraceEvents(TRACE_LEVEL_INFORMATION, DBG_INFO, 
            "\nCalling DDI WdfDmaEnablerCreate\n\n");

#pragma prefast(suppress:__WARNING_INVALID_PARAM_VALUE_1, "Intentionally passing 0 to test the DDI")
#pragma prefast(suppress:__WARNING_IRQ_TOO_HIGH, "Deliberately done to test framework verifier")
        status = WdfDmaEnablerCreate(
                    device,          // Handle to device object
                    pDmaConfig,      // Pointer to WDF_DMA_ENABLER_CONFIG structure
                    pAttributes,     // Pointer to WDF_OBJECT_ATTRIBUTES
                    pDmaEnabler      // Pointer to WDFDMAENABLER to create
                    );

For a further benefit, places where rules are deliberately broken for test purposes are now clearly marked by the suppression pragmas

Now, we actually pass NULL values in some other parameters- one reason this isn't reflected is there are common routines that establish some of these pointer values, and they are not annotated to reflect that the values can be NULL on output.  Someday maybe I'll get to that task [or just try to make sure we do it upfront on new test code].

Well, that run finished and I've got 128 bits of further errata to investigate [I sort by warning number, and I'd just finished the warnings for Irql too high when I saved it all off, recompiled to make sure I hadn't broken anything, and went on- I started with over 180 failures, so I'm making headway].  In case you were unaware of it, you can get the mnemonic constants for the errors [typically reported by number] from the file suppress.h in the sdk\inc directory.

So, I'm off to do more cleanup [I started with one test driver, and this just sort of ballooned on me when I decided to rip out some less desirable things I've been meaning to rip out], and I'll let you get back to your regularly scheduled workday!

Tunes

Grateful Dead

[Aoxomoxoa] China Cat Sunflower

[Workingman's Dead] High Time, Black Peter

[Europe 72] Sugar Magnolia

[Wake Of The Flood] Eyes of the World

[Shakedown Street] If I had the World to Give [Bonus Track]

[Go To Heaven] Alabama Getaway

[Dead Set] Brokedown Palace, Rhythm Devils

[Reckoning] It Must Have Been The Roses

Various Artists

[Record of Lodoss War] Instrumental [name untranslated]

[InuYasha TV OST] Kagome and InuYasha 1

Tsuneo Imahori

[Trigun the First Donuts] Knives

Yoko Kanno /Seatbelts

[Cowboy Bebop] Car 24, The Egg and I

[Cowboy Bebop Blue] Farewell Blues

[Cowboy Bebop Knockin On Heaven's Door] Diggin'

Yuki Kajiura / See-Saw

[.hack//liminality] [name untranslated- also on "Dream Field"], [name untranslated- also on "Dream Field"], [name untranslated]

[.hack//sign OST1] silent life, kiss

[Fiction] Fiction