Are there problems with Code Analysis with VS2015?

NTDEV
1

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 in function .

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

2

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

3

(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

4

(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

5

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: xxxxx@lists.osr.com
[mailto:xxxxx@lists.osr.com] On Behalf Of xxxxx@osr.com
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

6

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