IsBadXxxPtr should really be called CrashProgramRandomly

Complexity « Computing Life

Complexity « Computing Life — Sat, 23 Jun 2007 15:02:06 GMT

PingBack from http://computinglife.wordpress.com/2007/06/23/how-do-you-chose-your-apis/

re: IsBadReadPtr, IsBadWritePtr APIが禁止に - Windows Vista

社本＠ワック Blog — Sat, 11 Nov 2006 12:08:31 GMT

re: IsBadReadPtr, IsBadWritePtr APIが禁止に - Windows Vista

re: IsBadXxxPtr should really be called CrashProgramRandomly

Dave Harris — Wed, 04 Oct 2006 19:17:04 GMT

> The documentation for the IsBadXxxPtr

> functions presents the technical reasons why

Not really. It says, "Dereferencing potentially invalid pointers can disable stack expansion in other threads" but it doesn't say that IsBadXxxPtr dereferences the pointer. In fact I'd read it as saying the opposite: that you should use IsBadXxxPtr in order to avoid the stack expansion problem, because IsBadXxxPtr tells you whether the dereference would succeed without actually dereferencing it, by accessing the memory manager hardware directly. That's what I'd expect, and the documentation doesn't disillusion me.

re: IsBadXxxPtr should really be called CrashProgramRandomly

Dave Harris — Wed, 04 Oct 2006 19:02:37 GMT

The assertion won't catch all bad pointers even without race conditions. For example, some might point to memory which has been free()d (hence shouldn't be used) but which is still part of the crt heap (hence owned by the app as far as IsBadXxxPtr is concerned). It can still be useful to catch at least some errors, eg of the 0x00000008 form I mentioned earlier, or due to uninitialised pointer variables etc.

The false positives are a more serious issue. The real problem with IsBadXxxPtr for assert purposes is not that it has unwanted side effects on the (potentially random) memory page, but that it lies: it can return true for memory which can, in fact, be read or written to.

However, this is unlikely. It's unlikely to happen with stack frames, because the memory should come from the stack frame of routine which is higher up the stack, and that memory ought to be committed by the time our assert is hit. So we're left with the people playing games with PAGE_NOACCESS. Don't use the assert if you want to play those games.

Generally it would be better to just use the memory. The only times I'd even consider adding the assert are (a) as documentation; and (b) when the routine only used the memory some of the time, and I wanted the caller to make it available all of the time. The same applies to ASSERT( p != NULL ). Assertions for things which the hardware will detect anyway are often unnecessary clutter.

re: IsBadXxxPtr should really be called CrashProgramRandomly

BryanK — Tue, 03 Oct 2006 16:02:42 GMT

Er, wait, the above comment makes no sense. If you get a bad pointer in a release build, you'll just crash, same as if you never used IsBad*Ptr.

So never mind. (And sorry about the duplicate comment; it made sense to me at the time...)

But I still don't see the point. You won't necessarily catch all bad pointers (see the race condition), and you'll catch some pointers as bad that aren't actually.

re: IsBadXxxPtr should really be called CrashProgramRandomly

BryanK — Tue, 03 Oct 2006 16:00:18 GMT

> from an IDE

So you expect you'll *never* get an invalid pointer when your release build is running?

This seems way too optimistic to me.

re: IsBadXxxPtr should really be called CrashProgramRandomly

Dave Harris — Tue, 03 Oct 2006 14:57:52 GMT

BryanK wrote@

> But the caller can't catch a failed assert

> like they can catch an exception.

They shouldn't be trying to catch it. They should let the program terminate.

> All they get is a dialog "assertion failed:

> IsBadReadPtr(x)", which really doesn't help

> narrow anything down.

That depends on which assert you use and what your debugging tools are. Would it help if I wrote ASSERT instead of assert? With my IDE, an assertion failure gives the option to halt the program at the offending line, with the stack and variables available for inspection. I can switch to other threads and inspect them, too.

Remember we are talking about DEBUG builds, which normally means it is being run by a programmer from an IDE like Visual Studio. An end-user would be using a RELEASE build and not have the IsBadReadPtr check at all.

re: IsBadXxxPtr should really be called CrashProgramRandomly

Michiell — Tue, 03 Oct 2006 14:17:17 GMT

I'm surprised that these functions can't be removed from the API. Just put an #if (!Vista) before them. Any program written for XP will not only continue to run, but can still be compiled. Only programs written or updated for Vista will be impacted.

That's a good thing for Microsoft too; it improves applications build for Vista only, and thus is an upgrade reason for consumers.

re: IsBadXxxPtr should really be called CrashProgramRandomly

JimD — Mon, 02 Oct 2006 03:56:37 GMT

I agree with the arguments against using IsBadxxxPtr. It just always seemed like a good way to check pre-conditions.

I see MFC 8 (Visual Studio 2005) changed the AfxIsValidAddress, AfxIsValidString to no longer make the IsBadReadPtr/IsBadWritePtr calls. They now simply check for a null pointer. No string length checks, etc.

I haven't checked if the VS2003 SP1 made the same changes, but I guess I would expect that it did.

re: IsBadXxxPtr should really be called CrashProgramRandomly

Russell Davis — Mon, 02 Oct 2006 00:08:09 GMT

Raymond, I'm confused by this paragraph:

"Swallowing the exception in IsBadXxxPtr means that the caller's exception handler doesn't get a chance to run, which means that your code rejected a pointer that would actually have been okay, if only you had let the exception handler do its thing."

Think about that for a minute. Since the caller's exception handler has to run for the pointer to be valid INSIDE the callee, that means that the caller's exception handler must be set up to handle and stop the exception without the callee even knowing an exception was raised (which is indeed how guard page exceptions are handled). So exception swallowing is NOT the problem!

As a couple other commenters have noted, the real problem is that these guard page handlers are tied to a specific thread. If you access the guard page on a different thread, the handler won't get called, and problems will arise.

So it seems that in a single threaded situation (like the scenario mentioned in that paragraph), IsBadXxxPtr will not cause guard page problems.

Am I missing something?

[There are multiple problems with swallowing exceptions. I pointed out one of them (pointer swizzling); you pointed out another (stack guard pages). There is no conflict here. -Raymond]