PreFast false positives....

NTDEV
1

I’m getting a false positive from prefast in a routine suggesting that
execution can NULL reference a pointer. (In this case there is a loop
and a dependency on a second pointer that prefast cannot ‘see’.)

The question I have is whether all prefast warnings must be resolved
prior to WHQL submittial?

Thanks,
-PWM

2

As far as I know it is not a requirement to resolve all PREfast warnings
before making a WHQL submission.

However, for at least some PREfast “false positives” simple and reasonable
code restructuring can eliminate the warning and, in the long run, make the
code more maintainable.

If there are residual “false positives” that you have carefully evaluated,
then there are pragmas that can be applied to suppress the warning. Annotate
the pragma with a short explanation of the reasoning.

Thomas F. Divine
http://www.pcausa.com

From: “Peter W. Morreale”
Sent: Tuesday, February 16, 2010 11:36 AM
To: “Windows System Software Devs Interest List”
Subject: [ntdev] PreFast false positives…

>
> I’m getting a false positive from prefast in a routine suggesting that
> execution can NULL reference a pointer. (In this case there is a loop
> and a dependency on a second pointer that prefast cannot ‘see’.)
>
> The question I have is whether all prefast warnings must be resolved
> prior to WHQL submittial?
>
> Thanks,
> -PWM
>
>
>
> —
> NTDEV is sponsored by OSR
>
> 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

3

In one early version of DTM it was required to send the PreFast log
of the driver along with the submission. There was also an inactive
link to include an SDV log as well, with the implication that this
would become a requirement with the next DTM version.

These ended up being withdrawn, my understanding being that some
large outfits claimed this could require them to send proprietary
information to Microsoft and if that was the case, then they would
stop getting drivers WHQL’ed.

So, the direct answer to your question is that no you don’t have to
be clean in PreFast before submission. On the other hand, I would
suggest that it is treated just like any other useful tool such as
compiler warnings and Lint. It’s always good to have your code
considered clean by as many of these things as possible.

Of course, nothing substitutes for experience, so in cases like yours
where I understand what is being complained about and know it can’t
happen I’ll insert the pragma with a commented explanation of why it’s there.

Mark.

At 16:36 16/02/2010, Peter W. Morreale wrote:

I’m getting a false positive from prefast in a routine suggesting that
execution can NULL reference a pointer. (In this case there is a loop
and a dependency on a second pointer that prefast cannot ‘see’.)

The question I have is whether all prefast warnings must be resolved
prior to WHQL submittial?

Thanks,
-PWM

NTDEV is sponsored by OSR

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

4

no - prefast is urged but not required. That could change of course.
There is probably some obscure and less than actually documented
notation that would get you out of the prefast error.
Mark Roddy

On Tue, Feb 16, 2010 at 8:36 AM, Peter W. Morreale wrote:
>
> I’m getting a false positive from prefast in a routine suggesting that
> execution can NULL reference a pointer. ? (In this case there is a loop
> and a dependency on a second pointer that prefast cannot ‘see’.)
>
> The question I have is whether all prefast warnings must be resolved
> prior to WHQL submittial?
>
> Thanks,
> -PWM
>
>
>
> —
> NTDEV is sponsored by OSR
>
> 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
>

5

On Tue, 2010-02-16 at 12:10 -0500, Thomas F. Divine wrote:

As far as I know it is not a requirement to resolve all PREfast warnings
before making a WHQL submission.

However, for at least some PREfast “false positives” simple and reasonable
code restructuring can eliminate the warning and, in the long run, make the
code more maintainable.

I don’t believe so in this case, however the routine is included below
for commenting enjoyment. The pointer in question is “prev”.

If there are residual “false positives” that you have carefully evaluated,
then there are pragmas that can be applied to suppress the warning. Annotate
the pragma with a short explanation of the reasoning.

I prefer to not add pragmas for such, but thank you for the suggestion.

Best,
-PWM

/*
* SrRxProcess - Process the busy list of incoming frames.
* For keep packets, we chain the rcb NBLs together
* to send to the upper layers as a single entity.
*/
static VOID
SrRxProcess(PADAPTER a, PLIST_ENTRY incoming)
{
PNET_BUFFER_LIST nbl = NULL;
PNET_BUFFER_LIST prev = NULL;
PRCB rcb;
ULONG count = 0;

while(!IsListEmpty(incoming)) {
rcb = (PRCB)RemoveHeadList(incoming);

/* Decide whether to discard or keep. */
if (SrRxKeep(a, rcb)) {

SrRxProcessFrame(rcb);

/* Chain received packets together. */
if (!nbl)
nbl = rcb->nbl;
else {
NET_BUFFER_LIST_NEXT_NBL(prev) =
rcb->nbl;
}
count++;
VenetBusyInc(a); /* Decremented in ReturnNetBuf*
*/
prev = rcb->nbl;
}
else
SrRcbPut(a, rcb, TRUE);
}

if (count) {
NET_BUFFER_LIST_NEXT_NBL(prev) = NULL;
NdisMIndicateReceiveNetBufferLists(a->adapterHandle,
nbl,
NDIS_DEFAULT_PORT_NUMBER, count,
NDIS_RECEIVE_FLAGS_DISPATCH_LEVEL);
}
}

6

Peter W. Morreale wrote:

static VOID
SrRxProcess(PADAPTER a, PLIST_ENTRY incoming)
{
PNET_BUFFER_LIST nbl = NULL;
PNET_BUFFER_LIST prev = NULL;
PRCB rcb;
ULONG count = 0;

while(!IsListEmpty(incoming)) {
rcb = (PRCB)RemoveHeadList(incoming);

/* Decide whether to discard or keep. */
if (SrRxKeep(a, rcb)) {

SrRxProcessFrame(rcb);

/* Chain received packets together. */
if (!nbl)
nbl = rcb->nbl;
else {
NET_BUFFER_LIST_NEXT_NBL(prev) = rcb->nbl;
}
count++;
VenetBusyInc(a); /* Decremented in
ReturnNetBuf* */
prev = rcb->nbl;
}
else
SrRcbPut(a, rcb, TRUE);
}

Are you sure it’s complaining about “nbl” and “prev”, and not the fact
that you never check “rcb” for NULL before dereferencing it?

–
Tim Roberts, xxxxx@probo.com
Providenza & Boekelheide, Inc.

7

On Tue, 2010-02-16 at 10:09 -0800, Tim Roberts wrote:

Peter W. Morreale wrote:
>
>
> static VOID
> SrRxProcess(PADAPTER a, PLIST_ENTRY incoming)
> {
> PNET_BUFFER_LIST nbl = NULL;
> PNET_BUFFER_LIST prev = NULL;
> PRCB rcb;
> ULONG count = 0;
>
> while(!IsListEmpty(incoming)) {
> rcb = (PRCB)RemoveHeadList(incoming);
>
> /* Decide whether to discard or keep. */
> if (SrRxKeep(a, rcb)) {
>
> SrRxProcessFrame(rcb);
>
> /* Chain received packets together. */
> if (!nbl)
> nbl = rcb->nbl;
> else {
> NET_BUFFER_LIST_NEXT_NBL(prev) = rcb->nbl;
> }
> count++;
> VenetBusyInc(a); /* Decremented in
> ReturnNetBuf* */
> prev = rcb->nbl;
> }
> else
> SrRcbPut(a, rcb, TRUE);
> }

Are you sure it’s complaining about “nbl” and “prev”, and not the fact
that you never check “rcb” for NULL before dereferencing it?

Yes. Its only complaining about prev.

rcb cannot be NULL. The struct contains the LIST_ENTRY as the first
member.

Thanks,
-PWM

Thanks,
-PWM

8

How about:

static VOID
SrRxProcess(PADAPTER a, PLIST_ENTRY incoming)
{
PNET_BUFFER_LIST nbl = NULL;
PNET_BUFFER_LIST prev = NULL;
PLIST_ENTRY entry;
PRCB rcb;
ULONG count = 0;

while ((entry = RemoveHeadList (incoming)) != incoming
&& entry != NULL) /* Just an extra test to shut up
PreFast */
{
rcb = (PRCB) entry;

/* Decide whether to discard or keep. */
if (SrRxKeep(a, rcb)) {

SrRxProcessFrame(rcb);

/* Chain received packets together. */
if (!prev) /* Check prev instead of nbl
*/
nbl = rcb->nbl;
else {
NET_BUFFER_LIST_NEXT_NBL(prev) = rcb->nbl;
}
count++;
VenetBusyInc(a); /* Decremented in ReturnNetBuf*
*/
prev = rcb->nbl;
}
else
SrRcbPut(a, rcb, TRUE);
}

if (prev) { /* If count != 0, then prev != NULL */
NET_BUFFER_LIST_NEXT_NBL(prev) = NULL;
NdisMIndicateReceiveNetBufferLists(a->adapterHandle, nbl,
NDIS_DEFAULT_PORT_NUMBER, count,
NDIS_RECEIVE_FLAGS_DISPATCH_LEVEL);
}
}

“Peter W. Morreale”
Sent by: xxxxx@lists.osr.com
02/16/2010 12:42 PM
Please respond to
“Windows System Software Devs Interest List”

To
“Windows System Software Devs Interest List”
cc

Subject
Re: [ntdev] PreFast false positives…

On Tue, 2010-02-16 at 12:10 -0500, Thomas F. Divine wrote:

As far as I know it is not a requirement to resolve all PREfast warnings
before making a WHQL submission.

However, for at least some PREfast “false positives” simple and reasonable

code restructuring can eliminate the warning and, in the long run, make
the
code more maintainable.

I don’t believe so in this case, however the routine is included below for
commenting enjoyment. The pointer in question is “prev”.

If there are residual “false positives” that you have carefully evaluated,

then there are pragmas that can be applied to suppress the warning.
Annotate
the pragma with a short explanation of the reasoning.

I prefer to not add pragmas for such, but thank you for the suggestion.

Best,
-PWM

/*
* SrRxProcess - Process the busy list of incoming frames.
* For keep packets, we chain the rcb NBLs together
* to send to the upper layers as a single entity.
/
static VOID
SrRxProcess(PADAPTER a, PLIST_ENTRY incoming)
{
PNET_BUFFER_LIST nbl = NULL;
PNET_BUFFER_LIST prev = NULL;
PRCB rcb;
ULONG count = 0;

while(!IsListEmpty(incoming)) {
rcb = (PRCB)RemoveHeadList(incoming);

/ Decide whether to discard or keep. /
if (SrRxKeep(a, rcb)) {

SrRxProcessFrame(rcb);

/ Chain received packets together. /
if (!nbl)
nbl = rcb->nbl;
else {
NET_BUFFER_LIST_NEXT_NBL(prev) = rcb->nbl;
}
count++;
VenetBusyInc(a); / Decremented in ReturnNetBuf*
*/
prev = rcb->nbl;
}
else
SrRcbPut(a, rcb, TRUE);
}

if (count) {
NET_BUFFER_LIST_NEXT_NBL(prev) = NULL;
NdisMIndicateReceiveNetBufferLists(a->adapterHandle, nbl,
NDIS_DEFAULT_PORT_NUMBER, count,
NDIS_RECEIVE_FLAGS_DISPATCH_LEVEL);
}
}

—
NTDEV is sponsored by OSR

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

9

On Tue, 2010-02-16 at 13:43 -0500, xxxxx@attotech.com wrote:

How about:

Ooooohhhhh!!! good point! Checking on prev is all that is needed.

Thank you!

-PWM

10

>no - prefast is urged but not required. That could change of course.

Requiring PREFast for WHQL will require the submitter to submit the source to MS, not only the binaries + test log.

–
Maxim S. Shatskih
Windows DDK MVP
xxxxx@storagecraft.com
http://www.storagecraft.com

11

Oooohhh I hate prefast when it does this. Extra code added to shut up
code integrity tool that doesn’t understand semantics of fundamental
data structures such as LIST_ENTRY. Add noise to code to eliminate
noise! Anyone else have a problem with that?

Mark Roddy

On Tue, Feb 16, 2010 at 10:51 AM, Peter W. Morreale
wrote:
> On Tue, 2010-02-16 at 13:43 -0500, xxxxx@attotech.com wrote:
>>
>> How about:
>>
>
> Ooooohhhhh!!! ?good point! ?Checking on prev is all that is needed.
>
> Thank you!
>
> -PWM
>
>
>
>
> —
> NTDEV is sponsored by OSR
>
> 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
>

12

I’m sure somebody will manage an enforcement mechanism that doesn’t
require source code.

Mark Roddy

On Tue, Feb 16, 2010 at 12:55 PM, Maxim S. Shatskih
wrote:
>>no - prefast is urged but not required. That could change of course.
>
> Requiring PREFast for WHQL will require the submitter to submit the source to MS, not only the binaries + test log.
>
> –
> Maxim S. Shatskih
> Windows DDK MVP
> xxxxx@storagecraft.com
> http://www.storagecraft.com
>
>
> —
> NTDEV is sponsored by OSR
>
> 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
>

13

I don’t think PreFast got confused by list itself, but rather failed to
recognize that “if (!nbl)” always evaluates to true on the first iteration:

if (!nbl)
nbl = rcb->nbl;
else {
NET_BUFFER_LIST_NEXT_NBL(prev) = rcb->nbl;
}
[…]

prev = rcb->nbl;

On Tue, 16 Feb 2010 21:58:26 +0100, Mark Roddy
wrote:
> Oooohhh I hate prefast when it does this. Extra code added to shut up
> code integrity tool that doesn’t understand semantics of fundamental
> data structures such as LIST_ENTRY. Add noise to code to eliminate
> noise! Anyone else have a problem with that?
>
> Mark Roddy
>
>
>
> On Tue, Feb 16, 2010 at 10:51 AM, Peter W. Morreale
> wrote:
>> On Tue, 2010-02-16 at 13:43 -0500, xxxxx@attotech.com wrote:
>>>
>>> How about:
>>>
>>
>> Ooooohhhhh!!! good point! Checking on prev is all that is needed.
>>
>> Thank you!
>>
>> -PWM

14

On Tue, 2010-02-16 at 12:58 -0800, Mark Roddy wrote:

Oooohhh I hate prefast when it does this. Extra code added to shut up
code integrity tool that doesn’t understand semantics of fundamental
data structures such as LIST_ENTRY. Add noise to code to eliminate
noise! Anyone else have a problem with that?

Mark Roddy

Yes.

-PWM

15

Maybe it’s just me, but how is PreFast supposed to know that rcb->nbl can’t be NULL? It’s just a
value passed in inside a list, and PreFast is not a global optimizer. If any rcb->nbl in the
(non-empty) list ever *were* NULL, NET_BUFFER_LIST_NEXT_NBL would, indeed, dereference a NULL
pointer, as long as it wasn’t the first or last one.

(even in the case where it was the first/last one, the call inside the “if(count)” would dereference
NULL)

Now, perhaps you’re validating elsewhere (in SrRxKeep, perhaps?) that rcb->nbl != NULL… but it
looks to me like a genuine hole in this function, when considered standalone.

On 2/16/2010 9:41 AM, Peter W. Morreale wrote:

On Tue, 2010-02-16 at 12:10 -0500, Thomas F. Divine wrote:
> As far as I know it is not a requirement to resolve all PREfast warnings
> before making a WHQL submission.
>
> However, for at least some PREfast"false positives" simple and reasonable
> code restructuring can eliminate the warning and, in the long run, make the
> code more maintainable.

I don’t believe so in this case, however the routine is included below
for commenting enjoyment. The pointer in question is “prev”.

>
> If there are residual"false positives" that you have carefully evaluated,
> then there are pragmas that can be applied to suppress the warning. Annotate
> the pragma with a short explanation of the reasoning.
>

I prefer to not add pragmas for such, but thank you for the suggestion.

Best,
-PWM

/*
* SrRxProcess - Process the busy list of incoming frames.
* For keep packets, we chain the rcb NBLs together
* to send to the upper layers as a single entity.
*/
static VOID
SrRxProcess(PADAPTER a, PLIST_ENTRY incoming)
{
PNET_BUFFER_LIST nbl = NULL;
PNET_BUFFER_LIST prev = NULL;
PRCB rcb;
ULONG count = 0;

while(!IsListEmpty(incoming)) {
rcb = (PRCB)RemoveHeadList(incoming);

/* Decide whether to discard or keep. */
if (SrRxKeep(a, rcb)) {

SrRxProcessFrame(rcb);

/* Chain received packets together. */
if (!nbl)
nbl = rcb->nbl;
else {
NET_BUFFER_LIST_NEXT_NBL(prev) = rcb->nbl;
}
count++;
VenetBusyInc(a); /* Decremented in ReturnNetBuf* */
prev = rcb->nbl;
}
else
SrRcbPut(a, rcb, TRUE);
}

if (count) {
NET_BUFFER_LIST_NEXT_NBL(prev) = NULL;
NdisMIndicateReceiveNetBufferLists(a->adapterHandle, nbl,
NDIS_DEFAULT_PORT_NUMBER, count,
NDIS_RECEIVE_FLAGS_DISPATCH_LEVEL);
}
}

16

On Tue, 2010-02-16 at 16:56 -0800, Ray Trent wrote:

Maybe it’s just me, but how is PreFast supposed to know that rcb->nbl can’t be NULL? It’s just a
value passed in inside a list, and PreFast is not a global optimizer. If any rcb->nbl in the
(non-empty) list ever *were* NULL, NET_BUFFER_LIST_NEXT_NBL would, indeed, dereference a NULL
pointer, as long as it wasn’t the first or last one.

That’s right, it can’t. *I* know its not NULL because rcb exists.
(Initialized at driver init, before we go ‘live’)

But PreFast was not complaining about rcb->nbl, it was complaining about
‘prev’, which it rightly (as a static code analyser) could not detect
that prev would be initialized prior to reference.

(even in the case where it was the first/last one, the call inside the “if(count)” would dereference
NULL)

Nope. Count can only be non-zero if an rcb was pulled from the list
head (incoming), in which case, prev = rcb->nbl = nbl. Read the code
again?

The idea of the code is to create a chained list of net buffer lists,
and pass in a single list to the upper stack for processing. This (I
was told) would be faster than passing up each NBL individually.

The correct mod was to do the if test on prev, since prev is set outside
of the if block. This allows static analysis to pass on the warning
since it will see that prev is set after the if block.

Static analyzers can only make a single pass through a looping
construct, so they test everything within the block against the
initializations prior to the block.

PreFast complained about the line that contains the:

NET_BUFFER_LIST_NEXT_NBL(prev) = rcb->nbl;
^^^^
as it rightly would. Its just a false positive in this case since only
in the case of having another rcb in the list would that line be
executed (nbl would not be NULL).

Best,
-PWM

Now, perhaps you’re validating elsewhere (in SrRxKeep, perhaps?) that rcb->nbl != NULL… but it
looks to me like a genuine hole in this function, when considered standalone.
On 2/16/2010 9:41 AM, Peter W. Morreale wrote:
> On Tue, 2010-02-16 at 12:10 -0500, Thomas F. Divine wrote:
>> As far as I know it is not a requirement to resolve all PREfast warnings
>> before making a WHQL submission.
>>
>> However, for at least some PREfast"false positives" simple and reasonable
>> code restructuring can eliminate the warning and, in the long run, make the
>> code more maintainable.
>
> I don’t believe so in this case, however the routine is included below
> for commenting enjoyment. The pointer in question is “prev”.
>
>>
>> If there are residual"false positives" that you have carefully evaluated,
>> then there are pragmas that can be applied to suppress the warning. Annotate
>> the pragma with a short explanation of the reasoning.
>>
>
> I prefer to not add pragmas for such, but thank you for the suggestion.
>
> Best,
> -PWM
>
>
> /*
> * SrRxProcess - Process the busy list of incoming frames.
> * For keep packets, we chain the rcb NBLs together
> * to send to the upper layers as a single entity.
> */
> static VOID
> SrRxProcess(PADAPTER a, PLIST_ENTRY incoming)
> {
> PNET_BUFFER_LIST nbl = NULL;
> PNET_BUFFER_LIST prev = NULL;
> PRCB rcb;
> ULONG count = 0;
>
> while(!IsListEmpty(incoming)) {
> rcb = (PRCB)RemoveHeadList(incoming);
>
> /* Decide whether to discard or keep. */
> if (SrRxKeep(a, rcb)) {
>
> SrRxProcessFrame(rcb);
>
> /* Chain received packets together. */
> if (!nbl)
> nbl = rcb->nbl;
> else {
> NET_BUFFER_LIST_NEXT_NBL(prev) = rcb->nbl;
> }
> count++;
> VenetBusyInc(a); /* Decremented in ReturnNetBuf* */
> prev = rcb->nbl;
> }
> else
> SrRcbPut(a, rcb, TRUE);
> }
>
> if (count) {
> NET_BUFFER_LIST_NEXT_NBL(prev) = NULL;
> NdisMIndicateReceiveNetBufferLists(a->adapterHandle, nbl,
> NDIS_DEFAULT_PORT_NUMBER, count,
> NDIS_RECEIVE_FLAGS_DISPATCH_LEVEL);
> }
> }
>

NTDEV is sponsored by OSR

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

17

"

Static analyzers can only make a single pass through a looping
construct, so they test everything within the block against the
initializations prior to the block.

"

Gonna disagree on this one. Prefast (at least the one I use internally)
does show its progress multiple times through the loop. If prefast
doesn’t know that rcb->nbl isn’t null, it won’t know that “prev” isn’t
null the next time around.

You can determine if this is the case by seeing if this fixes the problem:

__analysis_assume(rcb->nbl != NULL);

It helps to think in terms of local analysis: What CAN Prefast know.
The value of its output is limited by the value of its input.

I tend to prefer __analysis_assume() in my code over #pragma prefast (
suppress: XXX ) because a programmer can easily sanity check the
expression. And I tend to prefer these solutions to doing nothing,
because… generally, Prefast is probably right and I’m probably wrong.

Thanks,
Daniel

On 2/16/2010 5:49 PM, Peter W. Morreale wrote:

On Tue, 2010-02-16 at 16:56 -0800, Ray Trent wrote:

> Maybe it’s just me, but how is PreFast supposed to know that rcb->nbl can’t be NULL? It’s just a
> value passed in inside a list, and PreFast is not a global optimizer. If any rcb->nbl in the
> (non-empty) list ever *were* NULL, NET_BUFFER_LIST_NEXT_NBL would, indeed, dereference a NULL
> pointer, as long as it wasn’t the first or last one.
>
That’s right, it can’t. *I* know its not NULL because rcb exists.
(Initialized at driver init, before we go ‘live’)

But PreFast was not complaining about rcb->nbl, it was complaining about
‘prev’, which it rightly (as a static code analyser) could not detect
that prev would be initialized prior to reference.

> (even in the case where it was the first/last one, the call inside the “if(count)” would dereference
> NULL)
>
Nope. Count can only be non-zero if an rcb was pulled from the list
head (incoming), in which case, prev = rcb->nbl = nbl. Read the code
again?

The idea of the code is to create a chained list of net buffer lists,
and pass in a single list to the upper stack for processing. This (I
was told) would be faster than passing up each NBL individually.

The correct mod was to do the if test on prev, since prev is set outside
of the if block. This allows static analysis to pass on the warning
since it will see that prev is set after the if block.

Static analyzers can only make a single pass through a looping
construct, so they test everything within the block against the
initializations prior to the block.

PreFast complained about the line that contains the:

NET_BUFFER_LIST_NEXT_NBL(prev) = rcb->nbl;
^^^^
as it rightly would. Its just a false positive in this case since only
in the case of having another rcb in the list would that line be
executed (nbl would not be NULL).

Best,
-PWM

> Now, perhaps you’re validating elsewhere (in SrRxKeep, perhaps?) that rcb->nbl != NULL… but it
> looks to me like a genuine hole in this function, when considered standalone.
> On 2/16/2010 9:41 AM, Peter W. Morreale wrote:
>
>> On Tue, 2010-02-16 at 12:10 -0500, Thomas F. Divine wrote:
>>
>>> As far as I know it is not a requirement to resolve all PREfast warnings
>>> before making a WHQL submission.
>>>
>>> However, for at least some PREfast"false positives" simple and reasonable
>>> code restructuring can eliminate the warning and, in the long run, make the
>>> code more maintainable.
>>>
>> I don’t believe so in this case, however the routine is included below
>> for commenting enjoyment. The pointer in question is “prev”.
>>
>>
>>> If there are residual"false positives" that you have carefully evaluated,
>>> then there are pragmas that can be applied to suppress the warning. Annotate
>>> the pragma with a short explanation of the reasoning.
>>>
>>>
>> I prefer to not add pragmas for such, but thank you for the suggestion.
>>
>> Best,
>> -PWM
>>
>>
>> /*
>> * SrRxProcess - Process the busy list of incoming frames.
>> * For keep packets, we chain the rcb NBLs together
>> * to send to the upper layers as a single entity.
>> */
>> static VOID
>> SrRxProcess(PADAPTER a, PLIST_ENTRY incoming)
>> {
>> PNET_BUFFER_LIST nbl = NULL;
>> PNET_BUFFER_LIST prev = NULL;
>> PRCB rcb;
>> ULONG count = 0;
>>
>> while(!IsListEmpty(incoming)) {
>> rcb = (PRCB)RemoveHeadList(incoming);
>>
>> /* Decide whether to discard or keep. */
>> if (SrRxKeep(a, rcb)) {
>>
>> SrRxProcessFrame(rcb);
>>
>> /* Chain received packets together. */
>> if (!nbl)
>> nbl = rcb->nbl;
>> else {
>> NET_BUFFER_LIST_NEXT_NBL(prev) = rcb->nbl;
>> }
>> count++;
>> VenetBusyInc(a); /* Decremented in ReturnNetBuf* */
>> prev = rcb->nbl;
>> }
>> else
>> SrRcbPut(a, rcb, TRUE);
>> }
>>
>> if (count) {
>> NET_BUFFER_LIST_NEXT_NBL(prev) = NULL;
>> NdisMIndicateReceiveNetBufferLists(a->adapterHandle, nbl,
>> NDIS_DEFAULT_PORT_NUMBER, count,
>> NDIS_RECEIVE_FLAGS_DISPATCH_LEVEL);
>> }
>> }
>>
>>
>
> —
> NTDEV is sponsored by OSR
>
> 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
>

NTDEV is sponsored by OSR

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

18

Mark Roddy wrote:

I’m sure somebody will manage an enforcement mechanism that
doesn’t require source code.

Wasn’t there a MSFT-provided tool that stripped “proprietary information” out of the defects.xml file? I seem to remember something like that…

19

The first idea from Microsoft was to use the summary file that only had
totals and some info about the sys file including a CRC. The WHQL software
wouldn’t handle it properly and you had to submit the xml file. They
provided a script that would eliminate all the source information and that
output could be submitted. Of course, you could submit any file from any
version and WHQL would be happy since the summary file was the only one with
the CRC, date, and time information. I think it was a good idea even if it
had many bugs such as acquiring critical sections in one function and
releasing them in another. I think it failed even if those functions were
actually inlined in the calling function.

wrote in message news:xxxxx@ntdev…
> Mark Roddy wrote:
>
>> I’m sure somebody will manage an enforcement mechanism that
>> doesn’t require source code.
>
> Wasn’t there a MSFT-provided tool that stripped “proprietary information”
> out of the defects.xml file? I seem to remember something like that…
>

20

The point I was trying to make was that prev = rcb->nbl. So if the list was non-empty, but any of
the rcb->nbl were ever NULL, prev would also be NULL at some point (because prev is set to rcb->nbl
on each loop iteration).

“nbl” is just an element of the rcb struct, so there’s no way for Prefast to know that nbl can’t be
NULL (that would be knowledge from outside the function, because these values are passed in in the
list). In this function, there’s no check to make sure rcb->nbl != NULL.

If rcb->nbl *were* ever NULL, then prev would be NULL, and NET_BUFFER_LIST_NEXT_NBL(prev) would
segfault on the next loop iteration. That macro is essentially “prev->blah.blah.nbl”.

It has *everything* to do with Prefast going through the loop multiple times. I don’t recall how
many iterations it analyzes, but it’s not just 1 loop iteration.

On 2/16/2010 5:49 PM, Peter W. Morreale wrote:

On Tue, 2010-02-16 at 16:56 -0800, Ray Trent wrote:
> Maybe it’s just me, but how is PreFast supposed to know that rcb->nbl can’t be NULL? It’s just a
> value passed in inside a list, and PreFast is not a global optimizer. If any rcb->nbl in the
> (non-empty) list ever *were* NULL, NET_BUFFER_LIST_NEXT_NBL would, indeed, dereference a NULL
> pointer, as long as it wasn’t the first or last one.

That’s right, it can’t. *I* know its not NULL because rcb exists.
(Initialized at driver init, before we go ‘live’)

But PreFast was not complaining about rcb->nbl, it was complaining about
‘prev’, which it rightly (as a static code analyser) could not detect
that prev would be initialized prior to reference.

> (even in the case where it was the first/last one, the call inside the “if(count)” would dereference
> NULL)

Nope. Count can only be non-zero if an rcb was pulled from the list
head (incoming), in which case, prev = rcb->nbl = nbl. Read the code
again?

The idea of the code is to create a chained list of net buffer lists,
and pass in a single list to the upper stack for processing. This (I
was told) would be faster than passing up each NBL individually.

The correct mod was to do the if test on prev, since prev is set outside
of the if block. This allows static analysis to pass on the warning
since it will see that prev is set after the if block.

Static analyzers can only make a single pass through a looping
construct, so they test everything within the block against the
initializations prior to the block.

PreFast complained about the line that contains the:

NET_BUFFER_LIST_NEXT_NBL(prev) = rcb->nbl;
^^^^
as it rightly would. Its just a false positive in this case since only
in the case of having another rcb in the list would that line be
executed (nbl would not be NULL).

Best,
-PWM

>
> Now, perhaps you’re validating elsewhere (in SrRxKeep, perhaps?) that rcb->nbl != NULL… but it
> looks to me like a genuine hole in this function, when considered standalone.
> On 2/16/2010 9:41 AM, Peter W. Morreale wrote:
>> On Tue, 2010-02-16 at 12:10 -0500, Thomas F. Divine wrote:
>>> As far as I know it is not a requirement to resolve all PREfast warnings
>>> before making a WHQL submission.
>>>
>>> However, for at least some PREfast"false positives" simple and reasonable
>>> code restructuring can eliminate the warning and, in the long run, make the
>>> code more maintainable.
>>
>> I don’t believe so in this case, however the routine is included below
>> for commenting enjoyment. The pointer in question is “prev”.
>>
>>>
>>> If there are residual"false positives" that you have carefully evaluated,
>>> then there are pragmas that can be applied to suppress the warning. Annotate
>>> the pragma with a short explanation of the reasoning.
>>>
>>
>> I prefer to not add pragmas for such, but thank you for the suggestion.
>>
>> Best,
>> -PWM
>>
>>
>> /*
>> * SrRxProcess - Process the busy list of incoming frames.
>> * For keep packets, we chain the rcb NBLs together
>> * to send to the upper layers as a single entity.
>> */
>> static VOID
>> SrRxProcess(PADAPTER a, PLIST_ENTRY incoming)
>> {
>> PNET_BUFFER_LIST nbl = NULL;
>> PNET_BUFFER_LIST prev = NULL;
>> PRCB rcb;
>> ULONG count = 0;
>>
>> while(!IsListEmpty(incoming)) {
>> rcb = (PRCB)RemoveHeadList(incoming);
>>
>> /* Decide whether to discard or keep. */
>> if (SrRxKeep(a, rcb)) {
>>
>> SrRxProcessFrame(rcb);
>>
>> /* Chain received packets together. */
>> if (!nbl)
>> nbl = rcb->nbl;
>> else {
>> NET_BUFFER_LIST_NEXT_NBL(prev) = rcb->nbl;
>> }
>> count++;
>> VenetBusyInc(a); /* Decremented in ReturnNetBuf* */
>> prev = rcb->nbl;
>> }
>> else
>> SrRcbPut(a, rcb, TRUE);
>> }
>>
>> if (count) {
>> NET_BUFFER_LIST_NEXT_NBL(prev) = NULL;
>> NdisMIndicateReceiveNetBufferLists(a->adapterHandle, nbl,
>> NDIS_DEFAULT_PORT_NUMBER, count,
>> NDIS_RECEIVE_FLAGS_DISPATCH_LEVEL);
>> }
>> }
>>
>
>
> —
> NTDEV is sponsored by OSR
>
> 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