I have a number of routines in my driver with code like the following
if (user == NULL) return NOT_REGISTERED;
if (user->flags & NOT_REGISTERED) return NOT_REGISTERED;
return user->locks;
PREfast complains at the “return user->locks;” line
Locks.c(673) : warning 11: Dereferencing NULL pointer ‘user’.
It’s obviously confused. Any ideas as to how to remove the confusion (preferably without using a suppress pragma)? Note that it doesn’t complain about the immediately preceding user->flags reference.
I tried this
if (user == NULL)
return NOT_REGISTERED;
else
{
if (user->flags & NOT_REGISTERED)
return NOT_REGISTERED;
else
return user->locks;
}
But it still complained about “return user->locks”.
Don
The code that PREfast is complaining about is in a __try … __finally block. I don’t know if that’s relevant; just thought I’d mention it.
Don
It’s probably a Prefast bug that you should report, but…
What’s the code path that Prefast highlights for this warning?
Is it C or C++? I ask because if you overrode operator “==” in a smart
pointer class, for example, it’s entirely likely Prefast doesn’t
understand that.
Is the code in the try or finally block?
Actually, if you can post an entire example function that exhibits the
problem that might help…
xxxxx@careful.co.uk wrote:
I have a number of routines in my driver with code like the following
if (user == NULL) return NOT_REGISTERED;
if (user->flags & NOT_REGISTERED) return NOT_REGISTERED;
return user->locks;
PREfast complains at the “return user->locks;” line
Locks.c(673) : warning 11: Dereferencing NULL pointer ‘user’.
It’s obviously confused. Any ideas as to how to remove the confusion (preferably without using a suppress pragma)? Note that it doesn’t complain about the immediately preceding user->flags reference.
I tried this
if (user == NULL)
return NOT_REGISTERED;
else
{
if (user->flags & NOT_REGISTERED)
return NOT_REGISTERED;
else
return user->locks;
}
But it still complained about “return user->locks”.
Don
–
Ray
(If you want to reply to me off list, please remove “spamblock.” from my
email address)
The entire function (written in C, not c++) is
Locks_t kssLocksHeld(PFDO_DATA Ext, Pid_t pid)
{
if (pid == 0) return NOT_REGISTERED;
kssSpinLockAcquire(Ext->userInfo.lock);
__try
{
Puser_t user = findUser(pid, &Ext->userInfo.userList);
if (user == NULL) return NOT_REGISTERED;
if (user->flags & NOT_REGISTERED) return NOT_REGISTERED;
return user->locks;
}
__finally {kssSpinLockRelease(Ext->userInfo.lock);}
}
[resend due to bounce]
The owner of prefast for drivers has this to say:
This is from an old version of PFD (the warning number is 11, not 6011).? I doubt the try/finally is >relevant, but with static tools you never know for sure.
First thing to do is try a newer PFD (in the WDK).? If it repros then I’d need a repro file – it >doesn’t matter much what it’s like as long as it gets the false positive.
I don’t see anything I recognize as a known fixed bug, but there were a lot of bugfixes in this >general vicinity.
Thx
d
I think that you can get rid of the warning with an ASSERT(user != NULL)
before the return user->locks.
Bill Wandel
-----Original Message-----
From: xxxxx@lists.osr.com [mailto:xxxxx@lists.osr.com]
On Behalf Of xxxxx@careful.co.uk
Sent: Tuesday, August 01, 2006 1:03 PM
To: Windows System Software Devs Interest List
Subject: [ntdev] Incorrect PREfast diagnostic
I have a number of routines in my driver with code like the following
if (user == NULL) return NOT_REGISTERED;
if (user->flags & NOT_REGISTERED) return NOT_REGISTERED;
return user->locks;
PREfast complains at the “return user->locks;” line
Locks.c(673) : warning 11: Dereferencing NULL pointer ‘user’.
It’s obviously confused. Any ideas as to how to remove the confusion
(preferably without using a suppress pragma)? Note that it doesn’t complain
about the immediately preceding user->flags reference.
I tried this
if (user == NULL)
return NOT_REGISTERED;
else
{
if (user->flags & NOT_REGISTERED)
return NOT_REGISTERED;
else
return user->locks;
}
But it still complained about “return user->locks”.
Don
Questions? First check the Kernel Driver FAQ at
http://www.osronline.com/article.cfm?id=256
To unsubscribe, visit the List Server section of OSR Online at
http://www.osronline.com/page.cfm?name=ListServer
Thanks for the quick response: I mentioned the try/finally because I remembered a number of spurious “a code path does not return a value” compiler diagnostics that code like this used to provoke (from a much older version of the C compiler).
I’m using the PREfast I got with WDF version 1.1 (i.e. from 3790.1830) It reports
Microsoft (R) PREfast Version 1.5.2402 for 80x86.
in the console log.
Looking at http://www.microsoft.com/whdc/DevTools/tools/PREfast_steps.mspx,
I would have concluded that 1.5 is the version I should be running. Is it possible to run PREfast v8.0 (which is what I think Doron implies I should try) within a 3790.1830 build environment?
Or do I have to download the entire WDK?
Don
> I think that you can get rid of the warning with an ASSERT(user != NULL)
before the return user->locks.
Didn’t work for me I’m afraid: It still complained.
> kssSpinLockAcquire(Ext->userInfo.lock);
__try
{
Puser_t user = findUser(pid, &Ext->userInfo.userList);
if (user == NULL) return NOT_REGISTERED;
if (user->flags & NOT_REGISTERED) return NOT_REGISTERED;
return user->locks;
}
__finally {kssSpinLockRelease(Ext->userInfo.lock);}
Ya know, it just might be right. You have the return coded inside the
protecting spinlock. But you have a finally releasing the spinlock. The
lock is presumably released before the function actually returns, right?
While user will still be valid at the time of the return, who says that
user->locks will have the same value that it had inside the spinlock?
Presumably the structure could have been deallocated the instant the
spinlock was released?
Maybe grab the locks member into a local and return that.
Loren
That works for me.
-----Original Message-----
From: xxxxx@lists.osr.com
[mailto:xxxxx@lists.osr.com] On Behalf Of Loren Wilton
Sent: Tuesday, August 01, 2006 2:52 PM
To: Windows System Software Devs Interest List
Subject: Re: RE:[ntdev] Incorrect PREfast diagnostic
kssSpinLockAcquire(Ext->userInfo.lock);
__try
{
Puser_t user = findUser(pid, &Ext->userInfo.userList);
if (user == NULL) return NOT_REGISTERED;
if (user->flags & NOT_REGISTERED) return NOT_REGISTERED;
return user->locks;
}
__finally {kssSpinLockRelease(Ext->userInfo.lock);}
Ya know, it just might be right. You have the return coded inside the
protecting spinlock. But you have a finally releasing the spinlock.
The
lock is presumably released before the function actually returns, right?
While user will still be valid at the time of the return, who says that
user->locks will have the same value that it had inside the spinlock?
Presumably the structure could have been deallocated the instant the
spinlock was released?
Maybe grab the locks member into a local and return that.
Loren
Questions? First check the Kernel Driver FAQ at
http://www.osronline.com/article.cfm?id=256
To unsubscribe, visit the List Server section of OSR Online at
http://www.osronline.com/page.cfm?name=ListServer
Loren Wilton writes:
> > kssSpinLockAcquire(Ext->userInfo.lock);
> > __try
> > {
> > Puser_t user = findUser(pid, &Ext->userInfo.userList);
> >
> > if (user == NULL) return NOT_REGISTERED;
> > if (user->flags & NOT_REGISTERED) return NOT_REGISTERED;
> > return user->locks;
> > }
> > __finally {kssSpinLockRelease(Ext->userInfo.lock);}
>
> Ya know, it just might be right. You have the return coded inside the
> protecting spinlock. But you have a finally releasing the spinlock. The
> lock is presumably released before the function actually returns, right?
Huh? Dereferencing of “user” pointer happens inside of
spinlock-protected region, so no race here (except for compiler bug,
including illegal reordering across implied barrier).
> While user will still be valid at the time of the return, who says that
> user->locks will have the same value that it had inside the spinlock?
> Presumably the structure could have been deallocated the instant the
> spinlock was released?
But user->locks is not dereferenced, what’s wrong with returning a
pointer to recycled object? Well, the function returning pointer to
object that can disappear anytime is not awfully useful, but nor it is
wrong.
> Maybe grab the locks member into a local and return that.
>
> Loren
>
Nikita.
> ----------
From: xxxxx@lists.osr.com[SMTP:xxxxx@lists.osr.com] on behalf of Loren Wilton[SMTP:xxxxx@earthlink.net]
Reply To: Windows System Software Devs Interest List
Sent: Tuesday, August 01, 2006 8:51 PM
To: Windows System Software Devs Interest List
Subject: Re: RE:[ntdev] Incorrect PREfast diagnostic
> kssSpinLockAcquire(Ext->userInfo.lock);
> __try
> {
> Puser_t user = findUser(pid, &Ext->userInfo.userList);
>
> if (user == NULL) return NOT_REGISTERED;
> if (user->flags & NOT_REGISTERED) return NOT_REGISTERED;
> return user->locks;
> }
> __finally {kssSpinLockRelease(Ext->userInfo.lock);}
Ya know, it just might be right. You have the return coded inside the
protecting spinlock. But you have a finally releasing the spinlock. The
lock is presumably released before the function actually returns, right?
While user will still be valid at the time of the return, who says that
user->locks will have the same value that it had inside the spinlock?
Presumably the structure could have been deallocated the instant the
spinlock was released?
PREfast complains about dereferencing NULL pointer ‘user’. I’d presume value to be returned is stored somewhere (register, stack variable) and then __finally block is executed.
Anyway, above code is ugly and inefficient. Direct return from __try block causes unnecessary stack unwinding with performance penalty. __leave should be used instead.
Best regards,
Michal Vodicka
UPEK, Inc.
[xxxxx@upek.com, http://www.upek.com]
> Ya know, it just might be right. You have the return coded inside the
protecting spinlock. But you have a finally releasing the spinlock. The
lock is presumably released before the function actually returns, right?
Quite right. but the value of user->locks is what the function should return, even if the
user structure evaporates between the release of the spinlock and the exit of the function.
There is no concept that C somehow takes the expression in a return statement and evaluates
it again just before the function exits. The value of the function is whatever user->locks turns
out to be when it is evaluated, then the spinlock is released, then the function exits with the
precomputed value.
(From a different post)
Anyway, above code is ugly and inefficient
Maybe: ugliness is always in the eye of the beholder. The reason I used try/finally was that if any
code called by this function generated an exception, and a calling function had an exception handler, it is guaranteed that the spinlock would be released as part of the stack unwinding. You call it ugly; I call it robust. Our opinions differ.
Don
If your code is generating a random exception, then you are in an
unknown state and catching the exception above you w/out letting the app
die is not very robust. Now you are running in a state where you can’t
trust anything anymore. But this is an argument for another day…
d
> ----------
From: xxxxx@lists.osr.com[SMTP:xxxxx@lists.osr.com] on behalf of xxxxx@careful.co.uk[SMTP:xxxxx@careful.co.uk]
Reply To: Windows System Software Devs Interest List
Sent: Tuesday, August 01, 2006 11:18 PM
To: Windows System Software Devs Interest List
Subject: RE:[ntdev] Incorrect PREfast diagnostic
(From a different post)
> Anyway, above code is ugly and inefficient
Maybe: ugliness is always in the eye of the beholder. The reason I used try/finally was that if any
code called by this function generated an exception, and a calling function had an exception handler, it is guaranteed that the spinlock would be released as part of the stack unwinding. You call it ugly; I call it robust. Our opinions differ.
__try/__finally is OK. return from the middle of __try block isn’t. return or goto causes stack unwinding which is unnecessary. __leave should be used instead. Read the relevant part of MSDN docs.
Best regards,
Michal Vodicka
UPEK, Inc.
[xxxxx@upek.com, http://www.upek.com]
With a spinlock held, this code runs at dispatch.
Will __finally and stack unwinding work correctly at dispatch?
–PA
“Roddy, Mark” wrote in message news:xxxxx@ntdev…
That works for me.
-----Original Message-----
From: xxxxx@lists.osr.com
[mailto:xxxxx@lists.osr.com] On Behalf Of Loren Wilton
Sent: Tuesday, August 01, 2006 2:52 PM
To: Windows System Software Devs Interest List
Subject: Re: RE:[ntdev] Incorrect PREfast diagnostic
> kssSpinLockAcquire(Ext->userInfo.lock);
> __try
> {
> Puser_t user = findUser(pid, &Ext->userInfo.userList);
>
> if (user == NULL) return NOT_REGISTERED;
> if (user->flags & NOT_REGISTERED) return NOT_REGISTERED;
> return user->locks;
> }
>__finally {kssSpinLockRelease(Ext->userInfo.lock);}
Ya know, it just might be right. You have the return coded inside the
protecting spinlock. But you have a finally releasing the spinlock.
The
lock is presumably released before the function actually returns, right?
While user will still be valid at the time of the return, who says that
user->locks will have the same value that it had inside the spinlock?
Presumably the structure could have been deallocated the instant the
spinlock was released?
Maybe grab the locks member into a local and return that.
Loren
—
Questions? First check the Kernel Driver FAQ at
http://www.osronline.com/article.cfm?id=256
To unsubscribe, visit the List Server section of OSR Online at
http://www.osronline.com/page.cfm?name=ListServer
It works, the issue is the hideous stack usage. I prefer other less
expensive constructs.
The issue is it seems exactly what code is generated by the return/finally
sequence. If in fact the sequence is
Release lock;
Return user->locks;
Then perhaps prefast, and perhaps unintentionally, has a point. The
disassembly for this code segment would be interesting.
=====================
Mark Roddy DDK MVP
Windows 2003/XP/2000 Consulting
Hollis Technology Solutions 603-321-1032
www.hollistech.com
-----Original Message-----
From: xxxxx@lists.osr.com
[mailto:xxxxx@lists.osr.com] On Behalf Of Pavel A.
Sent: Tuesday, August 01, 2006 8:03 PM
To: Windows System Software Devs Interest List
Subject: Re:[ntdev] RE:Incorrect PREfast diagnostic
With a spinlock held, this code runs at dispatch.
Will __finally and stack unwinding work correctly at dispatch?
–PA
“Roddy, Mark” wrote in message
> news:xxxxx@ntdev…
> That works for me.
>
>
> -----Original Message-----
> From: xxxxx@lists.osr.com
> [mailto:xxxxx@lists.osr.com] On Behalf Of Loren Wilton
> Sent: Tuesday, August 01, 2006 2:52 PM
> To: Windows System Software Devs Interest List
> Subject: Re: RE:[ntdev] Incorrect PREfast diagnostic
>
> > kssSpinLockAcquire(Ext->userInfo.lock);
> > __try
> > {
> > Puser_t user = findUser(pid, &Ext->userInfo.userList);
> >
> > if (user == NULL) return NOT_REGISTERED;
> > if (user->flags & NOT_REGISTERED) return NOT_REGISTERED;
> > return user->locks;
> > }
> >__finally {kssSpinLockRelease(Ext->userInfo.lock);}
>
> Ya know, it just might be right. You have the return coded
> inside the protecting spinlock. But you have a finally
> releasing the spinlock.
> The
> lock is presumably released before the function actually
> returns, right?
>
> While user will still be valid at the time of the return, who
> says that
> user->locks will have the same value that it had inside the spinlock?
> Presumably the structure could have been deallocated the
> instant the spinlock was released?
>
> Maybe grab the locks member into a local and return that.
>
> Loren
>
>
> —
> Questions? First check the Kernel Driver FAQ at
> http://www.osronline.com/article.cfm?id=256
>
> To unsubscribe, visit the List Server section of OSR Online
> at http://www.osronline.com/page.cfm?name=ListServer
>
>
>
> —
> Questions? First check the Kernel Driver FAQ at
> http://www.osronline.com/article.cfm?id=256
>
> To unsubscribe, visit the List Server section of OSR Online
> at http://www.osronline.com/page.cfm?name=ListServer
>
> There is no concept that C somehow takes the expression in a return
statement and evaluates
it again just before the function exits. The value of the function is
whatever user->locks turns
out to be when it is evaluated, then the spinlock is released, then the
function exits with the
precomputed value.
There is no concept in C of __try and __finally. There is of ‘try’, but not
‘__try’. Thus, the order of the parameter evaluation for the return
statement vs the execution of the finally code is not completely specified,
at least in my mind. You believe it will evaluate user->locks into a temp
and then return that. *I* don’t believe I’d blindly trust the compiler to
do something like that.
Before I’d use that code I’d look at what the compiler generated at both
debug level and the desired release optimization level. Better, I’d pull
the result in to a local temp myself and save the worry of what the compiler
generates changing for the worse on the next release.
(From a different post)
> Anyway, above code is ugly and inefficient
Maybe: ugliness is always in the eye of the beholder. The reason I used
try/finally was that if any
code called by this function generated an exception, and a calling
function had an exception handler, it is guaranteed that the spinlock
would be released as part of the stack unwinding. You call it ugly; I call
it robust. Our opinions differ.
His opinion happens to be right: function return from inside a try block or
an exception handler(including a __finally) is slow and ugly. If you dig
around on MSDN it even tells you this. And there is absolutely no need for
it in this case. Simply set the value of to be returned into a temp and
then return that temp value after the finally block.
Loren
> If your code is generating a random exception, then you are in an
unknown state and catching the exception above you w/out letting the app
die is not very robust. Now you are running in a state where you can’t
trust anything anymore. But this is an argument for another day…
True. But if the alternative to catching the exception (and perhaps emitting some kind of “it failed” message) is a blue screen, because there is an unreleased spinlock, which would you prefer?
BTW. I asked if I could get PREfast 8.0 to work with 3790.1830. Do you happen to know if it works?
Don
> His opinion happens to be right: function return from inside a try block or
an exception handler(including a __finally) is slow and ugly.
So our opinions differ too :-). If the code in question was a performance bottleneck then I would care about slow. As it is I prefer clarity. Your mileage obviously does vary, but from my point of view it’s more a matter of taste. However, we could discuss this until the cows come home without coming to a resoulution: so I suggest we leave it there before the thread goes even more OT than it is already.
And there is absolutely no need for
it in this case. Simply set the value of to be returned into a temp and
then return that temp value after the finally block.
You’re right. I’m going to try this because, if it makes the PREfast error go away it is probably worth doing for that reason alone
Don