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.