Should I check the parameters to my function?
I just had an interesting discussion with one of the testers in my group.
He had just finished filing a series of bugs against our components because they weren’t failing when he passed bogus pointers to the API. Instead, they raised a 0xC0000005 exception and crashed his application.
The APIs did fail if he passed a null pointer in, with E_POINTER.
But he felt that the API should check all the bogus pointers passed in and fail with E_POINTER if the pointer passed in didn’t point to valid memory.
This has been a subject of a lot of ongoing discussion over the years internally here at Microsoft. There are two schools of thought:
School one says “We shouldn’t crash the application on bogus data. Crashing is bad. So we should check our parameters and return error codes if they’re bogus”.
School two says “GIGO – if the app hands us garbage, then big deal if it crashes”.
I’m firmly in the second camp (not surprisingly, if you know me). There are a lot of reasons for this. The biggest one is security. The way you check for bad pointers on Win32 is by calling the IsBadReadPtr and IsBadWritePtr API. Michael Howard calls these APIs “CrashMyApplication” and “CorruptMemoryAndCrashMySystem” respectively. The problem with IsBadReadPtr/IsBadWritePtr is that they do exactly what they’re advertised as doing: They read and/or write to the memory location specified, with an exception handler wrapped around the read/write. If an exception is thrown, they fail, if not, they succeed.
There are two problems with this. The only thing that IsBadReadPtr/IsBadWritePtr verifies is that at the instant that the API is called, there was valid memory at that location. There’s nothing to prevent another thread in the application from unmapping the virtual address passed into IsBadReadPtr immediately after the call is made. Which means that any error checks you made based on the results of this API aren’t valid (this is called out in the documentation for IsBadWritePtr/IsBadReadPtr).
The other one is worse. What happens if the memory address passed into IsBadReadPtr is a stack guard page (a guard page is a page kept at the bottom of the stack – when the system top level exception handler sees a fault on a guard page, it will grow the threads stack (up to the threads stack limit))? Well, the IsBadReadPtr will catch the guard page exception and will handle it (because IsBadReadPtr handles all exceptions). So the system exception handler doesn’t see the exception. Which means that when that thread later runs, its stack won’t grow past the current limit. By calling IsBadReadPtr in your API, you’ve turned an easily identifiable application bug into a really subtle stack overflow bug that may not be encountered for many minutes (or hours) later.
The other problem with aggressively checking for bad parameters on an API is that what happens if the app doesn’t check the return code from the API? This means that they could easily have a bug in their code that passes a bogus pointer into IsBadWritePtr, thus corrupting memory. But, since they didn’t check the return code, they don’t know about their bug. And, again, much later the heap corruption bug that’s caused by the call to IsBadWritePtr shows up. If the API had crashed, then they’d find the problem right away.
Now, having said all this, if you go with school two, you’ve still got a problem – you can’t trust the user’s buffers. At all. This means you’ve got to be careful when touching those buffers to ensure that you’re not going to deadlock the process by (for instance holding onto a critical section while writing to the user’s buffer).
The other thing to keep in mind is that there are some situations where it’s NOT a good idea to crash the user’s app. For example, if you’re using RPC, then RPC uses structured exception handling to communicate RPC errors back to the application (as opposed to API return codes). So sometimes you have no choice but to catch the exceptions and return them. The other case is if someone has written and shipped an existing API that uses IsBadReadPtr to check for bad pointers on input, it may not be possible to remove this because there may be applications that depend on this behavior.
So in general, it’s a bad idea to use IsBadXxxPtr on your input parameters to check for correctness. Your users may curse you for crashing their app when they screw up, but in the long term, it’s a better idea.
Comments
Anonymous
May 18, 2004
IMHO, the user shouldn't be able to crash the app. The app should verify all information from any untrustworthy source (user input, file, network, etc.) and provide feedback when the data is corrupt. This makes it a "firewall" of sorts. The app itself is trustworthy, or at least it should be once it's debugged.
The application is better equipped to deal with errors than API's, because (1) it knows where the data comes from, and (2) it has a feedback mechanism.Anonymous
May 18, 2004
The comment has been removedAnonymous
May 18, 2004
I guess what you're really asking (or what you seem to be asking) is not "should I check my input parameters" but rather "should I throw exceptions that could potentially crash my caller, or should I return error codes which might be ignored." The pros and cons of these approaches have been discussed recently. I like to throw exceptions, as it seems you also do.Anonymous
May 18, 2004
Why doesn't IsBadReadPtr() detect stack guard pages and return a result indicating it as bad?Anonymous
May 18, 2004
Joseph: Exactly. It goes to how much verification of an input is done in an API.
rentzsch: How would IsBadReadPtr() detect them?
I guess it could filter out guard pages exceptions and pass them on, but I'm not sure that's enough. And it doesn't change the fact that IsBadWritePtr works by corrupting memory (it reads the value, writes a new value, and then writes the original value back).Anonymous
May 18, 2004
The comment has been removedAnonymous
May 18, 2004
The comment has been removedAnonymous
May 18, 2004
It's a toughie, to be sure.
My own personal mantra for error handling is this:
Either fail loudly, or not at all. Except in debug mode - where you should do basic validation on all function parameters.
For example, what happens if either of these fail? What do you do?
CloseHandle (not much you can do if the file didn't close correctly...)
DeleteObject() (not much you can do if your font or brush can't be deleted).
Although, to be frank, error handling still bugs the heck out of me. It's a never ending job. I have the feeling that like optimization, it's a halting problem that one can only ever approximate a solution to, but never get a full and complete one.Anonymous
May 18, 2004
It is because of this point exactly that I prefer to pass by reference
ie
bool func( cSomeClass &inClass );
rather than
bool func( cSomeClass *inClass );
I can't be passed bogus data without some fancy C++ fanangling.
GregorAnonymous
May 18, 2004
An interesting point Gregor. But you can't do that in C.
Which kinda leaves it out when you're writing APIs :(
For managed code, it's more reasonable (since IL exposes reference parameters as a top level construct) but in managed code, there's a strong exception handling framework, and you can pretty safely assume you're not being handed a bad reference.Anonymous
May 19, 2004
Do IsBadReadPtr() and IsBadWritePtr()actually need to read and write to memory ? Can't they just check some attributes stored in the memory manager ? On x86 CPUs, it would be global/local descriptors. But you're right, they could change right after checked them.Anonymous
May 19, 2004
The comment has been removedAnonymous
May 19, 2004
Well, that depends on the size of the memory. You could have a huge block of memory described by one descriptor.Anonymous
May 19, 2004
The comment has been removedAnonymous
May 19, 2004
The comment has been removedAnonymous
May 19, 2004
The comment has been removedAnonymous
May 19, 2004
I believe that the pages that are marked as guard pages are physically mapped into memory, which is why it worked. But if you kept on going, the thread's stack wouldn't be able to expand further.Anonymous
May 20, 2004
The comment has been removedAnonymous
May 20, 2004
The comment has been removedAnonymous
May 21, 2004
Guess that’s what was happening to Visual Studio 6 when it found the code too complex… It would just die peacefully and quietly, no crash’n’burn, no “Send my remnants home”, no “Restart and recover documents”. Save often.
VS.NET is so much more stable.Anonymous
May 21, 2004
How true.Anonymous
August 18, 2005
In Windows Vista and IE7 we have changed the parameter validation code in WinInet to be more consistent...Anonymous
March 26, 2007
PingBack from http://blog.not-a-kernel-guy.com/2007/03/26/163Anonymous
January 20, 2009
PingBack from http://www.hilpers.com/957711-umfrage-isbadreadptrAnonymous
June 07, 2009
PingBack from http://greenteafatburner.info/story.php?id=1431