Strategy for invalid parameters

[M Bealby]

I was just taking a look at some ReactOS code this evening and it got
me thinking about what our general strategy for handling invalid
parameters is.
For example, in kernel32.dll there is a function DebugBreakProcess.
This function takes one parameter, a process handle.
This parameter is passed through the following list of functions
unchecked by any of them, until the final one will return a failure...
DebugBreakProcess
DbgUiIssueRemoteBreakIn
RtlCreateUserThread
RtlpCreateUserStack
ZwAllocateVirtualMemory
ObReferenceObjectByHandle
Now I know this isn't a security vulnerability, which is what I was
originally looking for, but it did make me think of the question of
where should bounds checking be added?
In this example, the process handle must be a value greater than zero.
Should this simple check be added to DebugBreakProcess, or all of the
above?  Is there some sort of standard that everyone should work to?
e.g. should all functions check their own parameters.  Sure it might
make it a little slower due to multiple checks but it would make
ReactOS very robust.
Any thoughts on this?
Martin
PS: Lack of activity recently had been due to uni. exams.

[Alex Ionescu]

M Bealby wrote:
>  I was just taking a look at some ReactOS code this
> evening and it got
> me thinking about what our general strategy for handling invalid
> parameters is.
> For example, in kernel32.dll there is a function DebugBreakProcess.
> This function takes one parameter, a process handle.
> This parameter is passed through the following list of functions
> unchecked by any of them, until the final one will return a failure...
> DebugBreakProcess
>  
Adding the check here means that if someone calls the Native API
directly the check is skipped, and this the one in the win32 api is useless.
>  DbgUiIssueRemoteBreakIn
>  
Adding the check here cuts the problem short, but now it means every API
that uses a process handle now needs to check the parameter, thus adding
thousands of lines of duplicated code. The same argument goes for the
ones below
>  RtlCreateUserThread
> RtlpCreateUserStack
> ZwAllocateVirtualMemory
>  
Since process handles are the responsability of the object manager, it
is the lowest place and the only place that should report this failure.
Rtl code shouldn't make assumptions about what is a valid handle and
what is not. If one day a new object manager is created which uses
negative handles as correct handles, and the 0 handle as "Current
process" handle, then thousands of lines of code woul dhave to be
changed. By leaving the responsability of determing what is and what
isn't a valid handle to the object manager itself, this keeps the kernel
componentized and mostly independent.
>  ObReferenceObjectByHandle
>  
Best regards,
Alex Ionescu

[M Bealby]

>  Since process handles are the responsability of the
> object manager,
> it is the lowest place and the only place that should report this
> failure.  Rtl code shouldn't make assumptions about what is a valid
> handle and what is not. If one day a new object manager is created
> which uses negative handles as correct handles, and the 0 handle as
> "Current process" handle, then thousands of lines of code woul dhave
> to be changed. By leaving the responsability of determing what is and
> what isn't a valid handle to the object manager itself, this keeps
> the kernel componentized and mostly independent. 
So what you're saying is that the functions in the modules of code
which actually do the work should perform the checking.  What about
this: If a parameter is passed straight through a function to another
no checking should take place, but if any manipulation of the
parameters occurs then they should be checked?  I think this might be
slightly different to your idea Alex.  What does everyone think?  I
personally think this is the best approach as it requires little, if
any duplicated checking code.
Comments welcome,
Martin

[Thomas Weidenmueller]

M Bealby wrote:
>  So what you're saying is that the functions in the
> modules of code
> which actually do the work should perform the checking.  What about
> this: If a parameter is passed straight through a function to another
> no checking should take place, but if any manipulation of the
> parameters occurs then they should be checked?  I think this might be
> slightly different to your idea Alex.  What does everyone think?  I
> personally think this is the best approach as it requires little, if
> any duplicated checking code.
>  
I agree with Alex because the only place the handle can be checked is in
ob. It doesn't make any sense to add pseudo checks.
- Thomas