There's no point improving the implementation of a bad idea

IsBadXxxPtr is a bad idea and you shouldn’t call it. In the comments, many people proposed changes to the function to improve the implementation. But what’s the point? IsBadXxxPtr is just a bad idea. There’s no point improving the implementation of a bad idea.

On the other hand, some people suggested making it clear that IsBadXxxPtr is a bad idea by making it worse. While this is tempting in a “I’m forcing you to do the right thing” sense, it carries with it serious compatibility problems.

There’s a lot of code that uses IsBadXxxPtr even though it’s a bad idea, and making IsBadXxxPtr worse would risk breaking those programs that managed to get away with it up until now. The danger of this is that people would upgrade to the next version of Windows and their program would stop working. Who do you think the blame will be placed on?

Sure, you might tell these people, “That’s because it’s a bug in your program. Go contact the vendor for an update.” Of course, that’s assuming you can prove that the reason why the program stopped working was this IsBadXxxPtr stuff. How can you tell that that was the problem? Maybe it was caused by some other problem, possibly even a bug in Windows itself. Or is your answer just going to be “Any program that crashes must be crashing due to misuse of IsBadXxxPtr?”

And, as I’ve noted before, contacting the vendor may not be enough. Most large corporations have programs that run their day-to-day operations. Some of them may have been written by a consultant ten years ago. Even if they have the source code, they may not have the expertise, resources, or simply inclination go to in and fix it. This happens more often than you think. To these customers, the behavior change is simply a regression.

Even if you have the source code and expertise, fixing the problem may not be as simple as it looks. You may have designed your program poorly and relied on IsBadXxxPtr to cover for your failings. For example, you may have decided that “The lParam to this message is a pointer to a CUSTOMER structure, or it could just be the customer ID number. I’ll use IsBadReadPtr, and if the pointer is bad, then the value must be the customer ID number.” Or you may have changed the definition of a function parameter and now need to detect whether your caller is calling the “old function” or the “new one”. Or it could simply be that once you remove the call to IsBadXxxPtr, your program crashes constantly because the IsBadXxxPtr was covering up for a huge number of other programming errors (such as uninitialized variables).

“But what if I’m just using it for debugging purposes?” For debugging purposes, allow me to propose the following drop-in replacement functions:

inline BOOL IsBadReadPtr2(CONST VOID *p, UINT_PTR cb)
{
  memcmp(p, p, cb);
  return FALSE;
}
inline BOOL IsBadWritePtr2(LPVOID p, UINT_PTR cb)
{
  memmove(p, p, cb);
  return FALSE;
}

It’s very simple: To see if a pointer is bad for reading, just read it (and similarly writing). If the pointer is bad, the read (or write) will raise an exception, and then you can investigate the bad pointer at the point it is found. We read from the memory by comparing it to itself and write to the memory by copying it to itself. These have no effect but they do force the memory to be read or written. Of course, this trick assumes that the compiler didn’t optimize out the otherwise pointless “compare memory to itself” and “copy memory to itself” operations. (Note also that the replacement IsBadWritePtr2 is not thread-safe, since another thread might be modifying the memory while we’re copying it. But then again, the original IsBadWritePtr wasn’t thread-safe either, so there’s no loss of amenity there.)

(As an aside: I’ve seen people try to write replacements for IsBadXxxPtr and end up introducing a bug along the way. There are many corner cases in this seemingly-simple family of functions.)