AfxIsValidAddress (and Others) Don’t Work as Advertised

MFC exposes a some memory debugging facilities such as AfxIsValidAddress, which (for debug builds) supposedly -

Tests any memory address to ensure that it is contained entirely within the program’s memory space.

Or does it?   AfxIsValidAddress only delegates the call to the undocumented ATL::AtlIsValidAddress, which reads:

// Verify that a pointer points to valid memory
inline BOOL AtlIsValidAddress(const void* p,
                  size_t nBytes, BOOL bReadWrite = TRUE)
{
      (bReadWrite);
      (nBytes);
      return (p != NULL);
}

The first two lines are just no-ops to silence compiler warnings about unused parameters. The promised verification that p is-contained-entirely-within-the-program’s-memory-space, amounts to the test that p is not null.

AfxIsValidString identically does not hold to its word and tests only if the input string is non-null:

inline BOOL AtlIsValidString(LPCSTR psz,
                          size_t nMaxLength = UINT_MAX)
{
      (nMaxLength);
      return (psz != NULL);
}

AfxAssertValidObject  is undocumented, and runs a bunch of redundant AfxIsValidAddress-es. CObject::AssertValid is documented (‘performs a validity check on this object by checking its internal state’!!), and is just as disappointing:

void CObject::AssertValid() const
{
     ASSERT(this != NULL);
}

The reason is probably that somewhere along the way MS realized there is no sane way to keep such promises.

There are several Win32 API similar in functionality: IsBadWritePtr, IsBadHugeWritePtr, IsBadReadPtr, IsBadHugeReadPtr, IsBadCodePtr, IsBadStringPtr. It has been known since at least 2004 that these functions are broken beyond repair and should never be used. The almighty Raymond Chen and Larry Osterman both discuss the reasons in detail, so just a short rehash: IsBad*Ptr all work by accessing the tested address and catching any thrown exceptions. Problem is that a certain few of these access violations (namely, those on stack guard pages) should never be caught – the OS uses them to properly enlarge thread stacks.  In the words of Michael Howard, who first realized this (or so I think – both Larry and Raymond attribute the CrashMyApplication nicknames to him):

You should also not catch all exceptions, but only types that you know about. Catching all exceptions is just as bad as using IsBad*Ptr.

I’m guessing that AfxIsValidAddress – and sisters – worked the same way, until someone realized they too were probably generating more debugging effort than they were saving. However, while the Win32 guys decided to leave their API semantics as are and clearly document them as obsolete and dangerous, the MFC guys turned their API into no-ops and did cleanup work that cannot be described as anything but sloppy. Not only is the documentation wrong, source comments are too:

// AfxIsValidAddress() returns TRUE if the passed parameter points
// to at least nBytes of accessible memory. If bReadWrite is TRUE,
// the memory must be writeable; if bReadWrite is FALSE, the memory
// may be const.

BOOL AFXAPI AfxIsValidAddress( ...

– and even the MFC sources themselves still contain many naive uses of these no-op tests (search around, you’ll find plenty).

Bonus: What If You Really Have To Test Memory for Validity?

This is after all not so far fetched. Years ago I had to work around a bug in nVidia GPU drivers, where occasionally some API (namely IDirect3DVolumeTexture9::LockBox, applied on a huuuuuuuge texture) returned success but gave a bogus memory buffer. The workaround was to VirtualQuery the address, and if not accessible – retry the process on a smaller volume texture. It went something like -

...
D3DLOCKED_BOX box;
HRESULT res = pVolumeTexture->LockBox(0, &box, NULL,
                                     D3DLOCK_DISCARD);
_ASSERT(res==D3D_OK  && box.pBits);
// this should have sufficed, but...

MEMORY_BASIC_INFORMATION meminf;
VirtualQuery(box.pBits, &meminf, sizeof(meminf));

BOOL  bOk = (meminf.State == MEM_COMMIT)  &&
      ( 0 !=  (meminf.Protect &
      ( PAGE_READWRITE | PAGE_WRITECOPY | PAGE_EXECUTE_READWRITE));

if (!bOk)
 //fallback
...

Of course VirtualQuery is innately sluggish, so this is completely unacceptable as mere parameter validation. Use this only when you know you have to, not just to be on the safe side.

About these ads

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s