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.)

Comments (42)
  1. Michiel says:

    To turn off the compiler smartness, add make it a "void const volatile*"

  2. Anonymous says:

    See the C++ standard, section 7.1.5.1.  The volatile keyword is only a hint.  It does not guarantee that optimizations are not applied.  Thus you can’t assume all Visual C++ compilers, past, present, and future, will not apply an optimization to a volatile variable.

  3. dal says:

    If fixing IsBadxxxPtr is really that simple why doesn’t MSFT do that?

    I suspect this API, flawed as it is, is just useful enough that it’s worth hanging onto.

  4. Zian says:

    Raymond just spent 4 paragraphs explaining why changing the function would be a bad idea.

    In brief:

    1. Application compatability

    2. Fixing a bad program (that uses IsBadXxxPointer) can be harder than you think

  5. Zian,

    To preempt Raymond: You must be new here. No extent of verbosity will deter the nitpickers.

    PMP

  6. geraldtubing says:

    dal:

    In addition to what Zian said, Raymond’s "fix" (as you call it) is basically a replacement function for debugging purposes. It does not silently test whether the memory can be written/read and return TRUE/FALSE depending on that; it just always returns FALSE, and if it really is impossible to read from/write to that pointer, it will throw an exception, ie. the program will crash or it will break into the debugger.

    IIRC IsBadXXXPtr basically does what Raymond’s function does, but the body is wrapped in a exception handler which silently catches any exceptions and returns TRUE which signifies that you should not read from / write to that memory location.

  7. invert says:

    Why not introduce a IsGoodXXXPtr?

  8. invert,

    Because, as the first link at the top of the article explains, the concept is flawed. Turning it into !IsBadXXXPtr doesn’t change that.

    PMP

  9. John says:

    I must be missing something here …

    a) If IsBadXXXPtr() so bad then why did it get introduced in the first place?

    b) why doesn’t someone stub it out so it doesn’t do anything?  Then it wouldn’t matter if anyone called it or not?

  10. Tom says:

    @invert: Dear gods, I hope that’s sarcasm.

  11. Dave says:

    I propose this:

    bool IsBadCodeIdea(LPCTSTR szYourIdea);

    Pass in a string expressing your idea for IsBadXXXPtr() fixes. At present it always returns true.

  12. constants flawed says:

    The concept is flawed with any constants defined and stored within a pointer. Even a null pointer is a flawed concept, becase that prevents allocation at address 0x00000000.

  13. Jake says:

    John,

    (Sigh…)

    b) why doesn’t someone stub it out so it doesn’t do anything?  Then it wouldn’t matter if anyone called it or not?

    <Snip>

    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).

    </Snip>

  14. Poor Raymond. How does he do it?

    PMP

  15. JS Bangs says:

    It’s heartening to see that in this case, at least, the regulars are doing Raymond’s work of making fun of the nitpickers and people who didn’t read the article.

    IsBadXxxPtr should never have been written, but it’s too late now, so the best thing we can do is tell people not to use it and try not to make it worse.

  16. Doug says:

    Hi Raymond,

    A quick question.  In your previous posts about this and the links that came up, it was shown that there’s also a problem with stack expansion and IsBadXXXPtr.  (From memory, the access might hit a stack guard page, which in the end results in an incorrect result and that stack not being expandable in the future.)

    I assume this problem remains with your replacement?  I think it does, but I’m just checking.

    I realise your propsed replacement is for debugging purposes only.

    Ta,

    Doug

  17. Doug says:

    Ignore the last – I overlooked the fact you’d removed the exception handling.  Apologies.

  18. KJK::Hyperion says:

    Anonymous: times are changing. Compiler vendors are working on a memory model for C++, and "volatile" is pretty much inescapably going to mark "barrier" variables. Visual Studio 2003 and later already treat volatile objects as barriers, and starting with Visual Studio 2005 barriers are enforced up the whole call stack. C/C++ can no longer afford to ignore the consequences of multi-processing.

  19. Craptain says:

    Maybe an IsBadComment() function would help this blogpost.

  20. Zian says:

    @PMP

    Actually, I’ve been following this blog for over a year.

    Anyway, I was getting slightly tired of the nitpickers. If Raymond can have fun with them, why not us?

  21. Apologies, Zian; I read your post the wrong way. Perhaps I’m developing the social skills of a thermonuclear device.

    PMP

  22. kokomo says:

    Invert is right. Create IsGoodXXXPtr(). Since IsGoodXXXPtr() is the exact opposite of IsBadXxxPtr(), that should fix the problem. ;-)

    No seriously.. these recent stories are not as good as the Bob stories. More Bob stories please Raymond?

  23. John Hensley says:

    The second poster is misreading the standard. "volatile" is a hint, but it is a hint that the implementation must follow as far as can be determined from the observable behavior of the program.

  24. Brian says:

    Perhaps someone should suggest to the MSDN team that they change the documentation for IsBad__Ptr to read: "<b>DO NOT CALL THIS FUNCTION</b>"

  25. There's no idea that couldn't be improved says:

    Jake wrote:

    I propose this:

    >

    bool IsBadCodeIdea(LPCTSTR szYourIdea);

    Even better:

    LPCTSTR GiveMeBetterIdea(LPCTSTR szYourIdea);

     LPCTSTR idea = _T("Let’s iterate");

     LPCTSTR heureka;

     while (idea = GiveMeBetterIdea(idea))

           heureka = idea;

  26. peterchen says:

    @constants flawed:

    C (and IIRC C++, too) doesn’t require the null pointer to be all-zeroes.

    (does that make me a comment nitpick?)

    As I understand, the idea was that, if the platform supports it,

    void * p = 0;

    would assign a "magic bit pattern" to p, that raises a hardware exception when dereferenced (without an explicit null check). Similary, (p==0) would compare p to the magic bit pattern, not test for all zeroes.

    In that sense, 0 is a C language level constant, but not a machine code one.

    and why is the concept flawed for constants? If it would work, it would still tell you if your constant would make a good read pointer.

  27. asdf says:

    KJK::Hyperion: volatile is highly unlikely to change much judging by all the proposals submitted and the general trend on the C++0x mailing lists. The problem with supporting it is that it’s not fine grained enough for a low-level primitive (in terms of current and future computers with increasingly aggressive and peculiar visibility rules and the various optimizations compilers would have to disable) and there’s too much legacy code due to vendors overloading volatile for their own purpose.

    I don’t doubt that Visual Studio allows this in the same way that Visual Studio allows type punning and breaking aliasing rules since a lot of people (including Microsoft) depend on this hand-wavy specified behavior.

  28. poochner says:

    While I think you could actually "fix" IsBadXxxPtr(), the method that comes to mind would be a lot* slower than the "try it and see if you get an exception" method.  The kernel could check the current memory mappings for the process.  However, since the routines are marked as "obsolete, and should not be used," this is moot.

    *possibly a major understatement.

  29. Sohail says:

    People don’t really do stupid things like pass in a Customer ID to that function do they?

  30. josh says:

    "Even a null pointer is a flawed concept, becase that prevents allocation at address 0x00000000."

    Bah.  The ability to have a "maybe a pointer to x" type outweighs the need to have that type be able to point to addresses near (whatever your bit pattern for null is).  If you really need to use every bit of your address space then you probably have enough control over the environment that it’s safe to dereference a "null" pointer, should you decide otherwise.  (and you’re certainly not running Windows, that prevents allocation of a LOT more places)

  31. Mr Cranky says:

    Is this so hard?

    inline BOOL IsBad***Ptr(CONST VOID *p, UINT_PTR cb)

    {

     return TRUE;

    }

    Surely that would wind down the usage expeditiously enough.

  32. Fluffy says:

    Shouldn’t that be:

    inline BOOL IsBadReadPtr2( CONST VOID *p, UINT_PTR cb )

    {

    __try { memcmp( p, p, cb ); }

    __except( EXCEPTION_EXECUTE_HANDLER ) { return( FALSE ); }

    return( TRUE );

    }

  33. Anon says:

    "Sohail

    People don’t really do stupid things like pass in a Customer ID to that function do they?"

    He mentioned LPARAM being sometimes an integer and sometimes a pointer which should give you a hint.

    http://msdn2.microsoft.com/en-us/library/ms649055.aspx

    from WINBASE.H

    #define MAKEINTATOM(i)  (LPTSTR)((DWORD)((WORD)(i)))

    Mind you, I think all the "cast an integer to a pointer" functions in the Windows API only work with 16 bit integers, so provided the first 64K is off limits, it’s possible to do this

    if ( arg < 0xFFFF )

       // we have an integer

    else

       // we have a pointer

    Which is safe. But it could be that when this stuff was designed things were just much more single threaded so IsBadWritePtr could be made to work reliably. On a Risc platform where structures are naturally aligned you can use the low two bits as an "this is an integer" flag. It’s still a evil though, since someone might port to x86.

  34. KJK::Hyperion says:

    asdf: doesn’t matter it’s not fine-grained enough – there are third-party specifications for fine-grained intrinsics (see: IA64’s __sync_xxx family) – it still needs to be defined in terms of a memory model. In practice, a "volatile" read or write is simply compiled as a full barrier, because it’s the safest bet

  35. JamesNT says:

    The idea that some of you are having such a hard time with the point Mr. Chen is trying to make just kills me.

    JamesNT

  36. ulric says:

    ho god.. IsBadReadPtr… I must have written here this previously, but this call was everywhere in our multi-million lines of code application.

    one of the "ui and services" dev just went crazy with it, checking every function parameters with it, and of course that code was copied by others.  

    Sometimes in 2000, after some internal debate I  silently went and used a #define in a global header to change that call to be only check for  "pointer is not NULL".  

    The application gained quite a bit of performance and no one cared from then on about this.

    Checking if the pointer was not null was all that anyone in our app actually could actually get.  What, ho, what, sort of stack or mem corruption was the dev trying to catch, and what exactly did he expect to be able to achieve by return E_POINTER.  If the ram is corrupted, you’re dead!

    A little knowledge is a dangerous thing.

    That IsBadReadPtr was being called thousands of times a second.

  37. Richard says:

    John Hensley wrote:

    > The second poster is misreading the standard.

    > "volatile" is a hint, but it is a hint that the

    > implementation must follow as far as can be

    > determined from the observable behavior of the

    > program.

    More than that: the way the program reads and modifies volatile data (and calls to library functions) is the *definition* of the observable behaviour of the program, and must not be changed by any optimization.

    C++ standard 1.9/6.

  38. SuperKoko says:

    @Fluffy:

    Even better:

    inline BOOL IsBadReadPtr2( CONST VOID *p, UINT_PTR cb ) {

     return IsBadReadPtr(p,cb);

    }

    Errr… How much improved is it?

  39. 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?

    LOL. Of course, people will blame Microsoft and the next Windows, and they will be 100% right! I can’t believe you had to write that…

    "That’s because it’s a bug in your program"

    Simple logic:

    • Program runs fine with Windows X.
    • Program crashes/does not work with Windows X+1.

    Possible conclusions:

    • Windows X+1 has limited compatibility with Windows X.
  40. Windows X+1 has a bug.

  41. But,

    • Program has a bug

    will totally make me LOL…

  • Worf says:

    Thank you Raymond.

    I se IsBadXXXPtr in a lot of code I inherit, and all the do is verify that a pointer is valid. To me, it adds verbosity to code that shouldn’t be needed, and I’m sure it hides bugs. Crashing early and with a debugger attached makes life easier.

    I do Windows CE BSP development, and the earlier and faster drivers and utility libraries crash during testing, the less post-project customer support is needed. If you can, get the mobile/embedded team to deprecate those calls – backwards compatibility is less an issue on embedded devices.

  • Igor says:

    volatile is the only way to tell the compiler to keep that damn overoptimized variable spilled to the stack.

  • Comments are closed.