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