Experts,
In the Filespy code I see such constructs every where in the FastIO
handlers…
PDEVICE_OBJECT DevObject;
PFAST_IO_DISPATCH FIODispatch;
BOOLEAN bResult ;
PAGED_CODE();
ASSERT( IS_CASPY_DEVICE_OBJECT( DeviceObject ) );
//
// Pass through logic for this type of Fast I/O
//
DevObject = ((PCASPY_DEVICE_EXTENSION)
(DeviceObject->DeviceExtension))->pAttachedToDeviceObject;
-
MS has not initialised the locals with NULL or initial values. Isn’t this
bad programming practice? OR sis they deliberately leave out the error
corrections to avoid code cluttering.
-
The ASSERT part doesnt get included in free build, thus deferencing the
DeviceObject pointer can cause a crash if it is NULL/stray. There is no
check anywhere for this too, isn’t it encessary. Well, the code never
crashes, except when I do a dc2 /hct /drv <mydriver_name> and the rigorous
testing with errors injected does make it crash.
So Was it a deliberate attempt to simplify things, or was it forgotten?
amitr0
–
- amitr0</mydriver_name>
Why would anyone uinitialize a local with NULL is it aint necesarry ?
----- Original Message -----
From: amitr0
To: Windows File Systems Devs Interest List
Sent: Friday, June 23, 2006 4:20 PM
Subject: [ntfsd] Is a NULL Check necessary here?
Experts,
In the Filespy code I see such constructs every where in the FastIO handlers…
PDEVICE_OBJECT DevObject;
PFAST_IO_DISPATCH FIODispatch;
BOOLEAN bResult ;
PAGED_CODE();
ASSERT( IS_CASPY_DEVICE_OBJECT( DeviceObject ) );
//
// Pass through logic for this type of Fast I/O
//
DevObject = ((PCASPY_DEVICE_EXTENSION) (DeviceObject->DeviceExtension))->pAttachedToDeviceObject;
-
MS has not initialised the locals with NULL or initial values. Isn’t this bad programming practice? OR sis they deliberately leave out the error corrections to avoid code cluttering.
-
The ASSERT part doesnt get included in free build, thus deferencing the DeviceObject pointer can cause a crash if it is NULL/stray. There is no check anywhere for this too, isn’t it encessary. Well, the code never crashes, except when I do a dc2 /hct /drv <mydriver_name> and the rigorous testing with errors injected does make it crash.
So Was it a deliberate attempt to simplify things, or was it forgotten?
amitr0
–
- amitr0
— Questions? First check the IFS FAQ at https://www.osronline.com/article.cfm?id=17 You are currently subscribed to ntfsd as: xxxxx@rdsor.ro To unsubscribe send a blank email to xxxxx@lists.osr.com</mydriver_name>
“amitr0” wrote in message news:xxxxx@ntfsd…
> 1. MS has not initialised the locals with NULL or initial values. Isn’t
> this
> bad programming practice? OR sis they deliberately leave out the error
> corrections to avoid code cluttering.
The question is, are there any paths that can reference an unitialized local
variable? Blindly, setting locals that will always be overwritten, can
confuse the next developer with a what is missing? A good set of checks for
this is to compile with /W4 and with PreFast both will complain for possible
unitialized paths. Unfortunately, the IFS samples have lagged the DDK
samples on becoming /W4 and Prefast clean.
> 2. The ASSERT part doesnt get included in free build, thus deferencing the
> DeviceObject pointer can cause a crash if it is NULL/stray. There is no
> check anywhere for this too, isn’t it encessary. Well, the code never
> crashes, except when I do a dc2 /hct /drv <mydriver_name> and the rigorous
> testing with errors injected does make it crash.
ASSERT’s never get built in the free build. Now the question for a
developer is this condition possible in normal operation, and does it make
sense to make this an error condition? If the condition should never happen
an ASSERT is the right idea, if it is a possible make it an error.
–
Don Burn (MVP, Windows DDK)
Windows 2k/XP/2k3 Filesystem and Driver Consulting
http://www.windrvr.com
Remove StopSpam from the email to reply</mydriver_name>
Don,
“Unfortunately, the IFS samples have lagged the DDK
samples on becoming /W4 and Prefast clean”
Yes, that is precisely the case, Prefast complains a lot, as you said.
'ASSERT’s never get built in the free build. Now the question for a
developer is this condition possible in normal operation, and does it make
sense to make this an error condition? If the condition should never happen
an ASSERT is the right idea, if it is a possible make it an error."
Is there a guarenteed way of saying that it will never happen. Can’t there
be cases unthought of? E.g a stray driver mangling my memory space etc.?
Should I crash just because of that, or shouldn’t I try to recover from it
gracefully?
If this condition should never occur, then why does DC2 test suite simulate
it?
Please excuse my ignorance, I am trying to learn…
amitr0
“amitr0” wrote in message news:xxxxx@ntfsd…
> 'ASSERT’s never get built in the free build. Now the question for a
> developer is this condition possible in normal operation, and does it make
> sense to make this an error condition? If the condition should never
> happen
> an ASSERT is the right idea, if it is a possible make it an error."
>
> Is there a guarenteed way of saying that it will never happen. Can’t there
> be cases unthought of? E.g a stray driver mangling my memory space etc.?
> Should I crash just because of that, or shouldn’t I try to recover from it
> gracefully?
There are no guarantees, but the ASSERT’s in the Microsoft examples are
reasonable. I.E. the condition should not be happening, but it is good to
test for in case it does. Of course there are cases you cannot handle,
somebody can blow away your stack or data space, there is little you can do.
The trick is to strike a balance between likely threats (for instance any
buffer from user space is truly suspect), and worrying unessecarily (how do
I recover when my stack is trashed by a 3rd party driver) on things you
cannot recover from.
Take a look at the security stuff on the WHDC, for instance
http://www.microsoft.com/whdc/driver/security/threatmodel.mspx gives a good
model of how to view threats. The prinicipals are the same whether the
attacker is malicious, or some unintended failure.
–
Don Burn (MVP, Windows DDK)
Windows 2k/XP/2k3 Filesystem and Driver Consulting
http://www.windrvr.com
Remove StopSpam from the email to reply
Thanks Don.
On 6/23/06, Don Burn wrote:
>
>
> “amitr0” wrote in message news:xxxxx@ntfsd…
> > 'ASSERT’s never get built in the free build. Now the question for a
> > developer is this condition possible in normal operation, and does it
> make
> > sense to make this an error condition? If the condition should never
> > happen
> > an ASSERT is the right idea, if it is a possible make it an error."
> >
> > Is there a guarenteed way of saying that it will never happen. Can’t
> there
> > be cases unthought of? E.g a stray driver mangling my memory space etc.?
> > Should I crash just because of that, or shouldn’t I try to recover from
> it
> > gracefully?
>
> There are no guarantees, but the ASSERT’s in the Microsoft examples are
> reasonable. I.E. the condition should not be happening, but it is good to
> test for in case it does. Of course there are cases you cannot handle,
> somebody can blow away your stack or data space, there is little you can
> do.
> The trick is to strike a balance between likely threats (for instance any
> buffer from user space is truly suspect), and worrying unessecarily (how
> do
> I recover when my stack is trashed by a 3rd party driver) on things you
> cannot recover from.
>
> Take a look at the security stuff on the WHDC, for instance
> http://www.microsoft.com/whdc/driver/security/threatmodel.mspx gives a
> good
> model of how to view threats. The prinicipals are the same whether the
> attacker is malicious, or some unintended failure.
>
>
> –
> Don Burn (MVP, Windows DDK)
> Windows 2k/XP/2k3 Filesystem and Driver Consulting
> http://www.windrvr.com
> Remove StopSpam from the email to reply
>
>
>
>
> —
> Questions? First check the IFS FAQ at
> https://www.osronline.com/article.cfm?id=17
>
> You are currently subscribed to ntfsd as: xxxxx@gmail.com
> To unsubscribe send a blank email to xxxxx@lists.osr.com
>
–
- amitr0