Windows System Software -- Consulting, Training, Development -- Unique Expertise, Guaranteed Results

Home NTDEV

Before Posting...

Please check out the Community Guidelines in the Announcements and Administration Category.

More Info on Driver Writing and Debugging


The free OSR Learning Library has more than 50 articles on a wide variety of topics about writing and debugging device drivers and Minifilters. From introductory level to advanced. All the articles have been recently reviewed and updated, and are written using the clear and definitive style you've come to expect from OSR over the years.


Check out The OSR Learning Library at: https://www.osr.com/osr-learning-library/


Are there problems with Code Analysis with VS2015?

Don_BurnDon_Burn Member - All Emails Posts: 1,764
Until recently I only was doing experiments with VS2015 for my on use. Now I have a client who wants me to use it, and I am finding a bunch complaints from VS2015 that do not appear with VS2013 on the same driver. The most common one is:

status = WdfWaitLockAcquire( lock, NULL );
if ( NT_SUCCESS( status ) )
{
...
WdfWaitLockRelease( lock );
}

In every case this is flagging with:

warning C26165: Possibly failing to release lock <lock> in function <func>.

There are others, but this is the biggie, I hate the idea of turning off the warning globally on the driver, but suppressing all of these would be quite a chore.

Am I missing something, or did Microsoft?


Don Burn
Windows Driver Consulting
Website: http://www.windrvr.com

Comments

  • Peter_Viscarola_(OSR)Peter_Viscarola_(OSR) Administrator Posts: 8,996
    Well... this is a great question.

    You don't say so, but the analysis is complaining about the case where you don't take the if (that is, !NT_SUCCESS(status)?? And that within the "..." there's no way to exit the block without releasing the lock?

    The answer to your question can be found in the function declaration. This is the first place to look when you're curious about "what's up" with Code Analysis.

    The SAL Annotations to this function have significantly improved over the years. As of KMDF V1.15, the function's declaration is:

    _When_(Timeout == NULL, _IRQL_requires_max_(PASSIVE_LEVEL))
    _When_(Timeout != NULL && *Timeout == 0, _IRQL_requires_max_(DISPATCH_LEVEL))
    _When_(Timeout != NULL && *Timeout != 0, _IRQL_requires_max_(PASSIVE_LEVEL))
    _Always_(_When_(Timeout == NULL, _Acquires_lock_(Lock)))
    _When_(Timeout != NULL && return == STATUS_SUCCESS, _Acquires_lock_(Lock))
    _When_(Timeout != NULL, _Must_inspect_result_)
    NTSTATUS
    FORCEINLINE
    WdfWaitLockAcquire(
    _In_
    _Requires_lock_not_held_(_Curr_)
    WDFWAITLOCK Lock,
    _In_opt_
    PLONGLONG Timeout
    )
    {
    return ((PFN_WDFWAITLOCKACQUIRE) WdfFunctions[WdfWaitLockAcquireTableIndex])(WdfDriverGlobals, Lock, Timeout);
    }

    So... note that:

    _Always_(_When_(Timeout == NULL, _Acquires_lock_(Lock)))

    This indicates that the lock is ALWAYS acquired... unconditionally... when there's no timeout specified. Always. This pretty much matches the expectation set by this:

    _When_(Timeout != NULL, _Must_inspect_result_)

    Which basically says "you don't need to check the return value, unless Timeout is not NULL).

    So... You're pretty much getting the error that the person who wrote this annotation intends.

    (The IRQL requirement here is surprising, by the way... Shouldn't it be APC_LEVEL and not PASSIVE_LEVEL?? For cross-reference, see KeWaitForSingleObject).

    Now, we can argue about whether their intention is correct or not. But, in any case, the right way to disable this is NOT globally but rather in-line in the source on a case-by-case basis, with a comment (a) WHY you're disabling the check, and (b) The WDK version that caused you to do the disable.

    Peter
    OSR
    @OSRDrivers

    Peter Viscarola
    OSR
    @OSRDrivers

  • Peter_Viscarola_(OSR)Peter_Viscarola_(OSR) Administrator Posts: 8,996
    (don't you HATE people who follow-up their own posts? I know *I* do...)

    To further emphasize what the SAL says, note that WDK docs for this function:

    "The caller does not have to check the return value if the Timeout pointer is NULL, because in that case WdfWaitLockAcquire returns only after it acquires the lock."

    You might question this as a strange semantic "gloss" for the WDF folks to apply, but if you read the documentation for KeWaitForSingleObject carefully, you'll see the passing comment:

    "If a NULL pointer is supplied for Timeout, the calling thread remains in a wait state until the Object is signaled."

    ...with no further qualification. So, the WDF semantics indeed do match the Ke semantics in this regard. They're just made more clear and more specific.

    Peter
    OSR
    @OSRDrivers

    Peter Viscarola
    OSR
    @OSRDrivers

  • Peter_Viscarola_(OSR)Peter_Viscarola_(OSR) Administrator Posts: 8,996
    (yet ANOTHER follow-up)

    I just ran into this same annotation myself in some code I'm writing, and this lead to some in-depth discussions in the halls of OSR.

    While the annotation matches the WDK WDF documentation, *I* don't think either the annotation OR the documentation is wise.

    I just doesn't make sense to me that a function returns a status and that I MUST effectively cast the return to VOID -- and must NOT check the status, and must ASSUME the call always works. I don't like this for a lot of reasons... it looks sloppy during code review, for one. For another, it locks the WDF Dev Team into a specific implementation, which is just plain silly. Suppose in KMDF V1.x, somebody decides when KMDF Verifier is enabled to check to see if the passed-in handle is valid, and return an error if it's not. That's not inconceivable. It would even be a good thing, IMHO. But, most of all, not checking the return status Just Feels Wrong.

    Because the docs indicate that whenever Timeout is NULL the function can NOT fail, it seems to me to be entirely safe to do:

    status = WdfWaitLockAcquire(Lock, NULL);

    if(!NT_SUCCESS(status)) {

    // trace and exit

    goto done;
    }

    But of course, this fails Code Analysis.

    To fix the Code Analysis warning (Mr. Burn's initial point), the best alternative is to add:

    _Analysis_assume_lock_not_held_(Lock);

    to the error code. So the code would read:

    status = WdfWaitLockAcquire(Lock, NULL);

    if(!NT_SUCCESS(status)) {

    // trace and exit

    // KMDF V1.15, VS 2015: Long comment here describing why
    // I'm forcing this assumption. Yuck!
    //
    _Analysis_assume_lock_not_held_(Lock);

    goto done;
    }


    I think that's the best course to take.

    Somebody here at OSR is going to write-up a blog post about this. The biggest problem is that changing the annotation for WdfWaitLockAcquire at this point would be difficult. Which is another reason why one shouldn't have annotations that require the user to NOT check a return status in the first place.

    Peter
    OSR
    @OSRDrivers

    Peter Viscarola
    OSR
    @OSRDrivers

  • Don_BurnDon_Burn Member - All Emails Posts: 1,764
    Peter,

    I wanted to thank you for the posts on this. I have been delaying in
    responding trying to find a study (I know I have but with too many years in
    the industry who knows where) that showed one of the most common problems in
    C at one time was this type of design. I remember that the study concluded
    that having a return that was "always safe" depending on the parameters was
    a bug waiting to happen, since

    (void) WdfWaitLockAcquire(Lock, NULL);

    Can easily become by a code maintainer:

    (void) WdfWaitLockAcquire(Lock, timeout);

    Now we would hope everyone would always use code analysis, but it is not
    likely. Perhaps Microsoft could provide a wrapper
    WdfWaitLockAcquireNoTimeout(Lock) or otherwise appropriate name that wraps
    the (void) WdfWaitLockAcquire(Lock, NULL) so we don't have to fight the
    semantics.


    Don Burn
    Windows Driver Consulting
    Website: http://www.windrvr.com




    -----Original Message-----
    From: [email protected]
    [mailto:[email protected]] On Behalf Of [email protected]
    Sent: Wednesday, November 18, 2015 2:22 PM
    To: Windows System Software Devs Interest List
    Subject: RE:[ntdev] Are there problems with Code Analysis with VS2015?

    (yet ANOTHER follow-up)

    I just ran into this same annotation myself in some code I'm writing, and
    this lead to some in-depth discussions in the halls of OSR.

    While the annotation matches the WDK WDF documentation, *I* don't think
    either the annotation OR the documentation is wise.

    I just doesn't make sense to me that a function returns a status and that I
    MUST effectively cast the return to VOID -- and must NOT check the status,
    and must ASSUME the call always works. I don't like this for a lot of
    reasons... it looks sloppy during code review, for one. For another, it
    locks the WDF Dev Team into a specific implementation, which is just plain
    silly. Suppose in KMDF V1.x, somebody decides when KMDF Verifier is enabled
    to check to see if the passed-in handle is valid, and return an error if
    it's not. That's not inconceivable. It would even be a good thing, IMHO.
    But, most of all, not checking the return status Just Feels Wrong.

    Because the docs indicate that whenever Timeout is NULL the function can NOT
    fail, it seems to me to be entirely safe to do:

    status = WdfWaitLockAcquire(Lock, NULL);

    if(!NT_SUCCESS(status)) {

    // trace and exit

    goto done;
    }

    But of course, this fails Code Analysis.

    To fix the Code Analysis warning (Mr. Burn's initial point), the best
    alternative is to add:

    _Analysis_assume_lock_not_held_(Lock);

    to the error code. So the code would read:

    status = WdfWaitLockAcquire(Lock, NULL);

    if(!NT_SUCCESS(status)) {

    // trace and exit

    // KMDF V1.15, VS 2015: Long comment here describing why
    // I'm forcing this assumption. Yuck!
    //
    _Analysis_assume_lock_not_held_(Lock);

    goto done;
    }


    I think that's the best course to take.

    Somebody here at OSR is going to write-up a blog post about this. The
    biggest problem is that changing the annotation for WdfWaitLockAcquire at
    this point would be difficult. Which is another reason why one shouldn't
    have annotations that require the user to NOT check a return status in the
    first place.

    Peter
    OSR
    @OSRDrivers


    ---
    NTDEV is sponsored by OSR

    Visit the list at: http://www.osronline.com/showlists.cfm?list=ntdev

    OSR is HIRING!! See http://www.osr.com/careers

    For our schedule of WDF, WDM, debugging and other seminars visit:
    http://www.osr.com/seminars

    To unsubscribe, visit the List Server section of OSR Online at
    http://www.osronline.com/page.cfm?name=ListServer
  • Peter_Viscarola_(OSR)Peter_Viscarola_(OSR) Administrator Posts: 8,996
    No probs, Don. This one kind of "got under my skin"... and then to run into it virtually the next day in my own code. Funny coincidence.

    A basic tenet of WDF is that, except for properties functions (functions with Get or Set in the name), WDF function calls return status. I know this is a basic tenet of WDF because I was part of the conversation when this decision was made in consultation with the community. I think you, Mr. Burn, were on that same list.

    There are very few functions that do not follow this basic principle, and for the MOST part, the ones that do not follow it (the most notable being WdfRequestSend) do not follow it for a very good reason. Hurrah for WDF! Hurrah for consistency!!

    I don't mind in the WdfWaitLockAcquire case if the SAL doesn't REQUIRE you to check the return status when Timeout is NULL. That's fine with me. But I *really* don't like being FORCED not to check the result.

    What's *really* interesting about this to me is that it's a great example of how one expresses one's engineering philosophy when they write SAL annotations. Of course, if you're the guy who's stuck updating/changing the annotations on 100 functions so you can close a bug and do some real work, it's very likely that you won't sit down and discuss every choice you make over a latte in the cafeteria with friends. But this shows how a correct yet "less the optimal" (shall we say) decision really throw a screwdriver into the works.

    OK... No more pontificating for now.

    Peter
    OSR
    @OSRDrivers

    Peter Viscarola
    OSR
    @OSRDrivers

Sign In or Register to comment.

Howdy, Stranger!

It looks like you're new here. Sign in or register to get started.

Upcoming OSR Seminars
OSR has suspended in-person seminars due to the Covid-19 outbreak. But, don't miss your training! Attend via the internet instead!
Writing WDF Drivers 12 September 2022 Live, Online
Internals & Software Drivers 23 October 2022 Live, Online
Kernel Debugging 14 November 2022 Live, Online
Developing Minifilters 5 December 2022 Live, Online