| Date: | June 25, 2007 / year-entry #228 |
| Tags: | other |
| Orig Link: | https://blogs.msdn.microsoft.com/oldnewthing/20070625-00/?p=26283 |
| Comments: | 42 |
| Summary: | 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... |
On the other hand, some people suggested making it clear that
There's a lot of code that uses
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 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
"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
(As an aside: I've seen people try to write replacements
for |
Comments (42)
Comments are closed. |
To turn off the compiler smartness, add make it a "void const volatile*"
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.
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.
Raymond just spent 4 paragraphs explaining why changing the function would be a bad idea.
In brief:
Application compatability
Fixing a bad program (that uses IsBadXxxPointer) can be harder than you think
Zian,
To preempt Raymond: You must be new here. No extent of verbosity will deter the nitpickers.
PMP
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.
Why not introduce a IsGoodXXXPtr?
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
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?
@invert: Dear gods, I hope that’s sarcasm.
I propose this:
bool IsBadCodeIdea(LPCTSTR szYourIdea);
Pass in a string expressing your idea for IsBadXXXPtr() fixes. At present it always returns true.
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.
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>
Poor Raymond. How does he do it?
PMP
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.
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
Ignore the last – I overlooked the fact you’d removed the exception handling. Apologies.
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.
Maybe an IsBadComment() function would help this blogpost.
@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?
Apologies, Zian; I read your post the wrong way. Perhaps I’m developing the social skills of a thermonuclear device.
PMP
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?
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.
PingBack from http://disorderedthoughtprocesses.com/2007/06/25/baffling-how-i-learned-to-stop-worrying-and-love-the-gui-and-high-level-languages/
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>"
Jake wrote:
>
Even better:
LPCTSTR GiveMeBetterIdea(LPCTSTR szYourIdea);
LPCTSTR idea = _T("Let’s iterate");
LPCTSTR heureka;
while (idea = GiveMeBetterIdea(idea))
heureka = idea;
@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.
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.
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.
People don’t really do stupid things like pass in a Customer ID to that function do they?
"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)
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.
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 );
}
"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.
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
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
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.
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.
@Fluffy:
Even better:
inline BOOL IsBadReadPtr2( CONST VOID *p, UINT_PTR cb ) {
return IsBadReadPtr(p,cb);
}
Errr… How much improved is it?
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…
Simple logic:
Program crashes/does not work with Windows X+1.
Possible conclusions:
Windows X+1 has a bug.
But,
will totally make me LOL…
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.
volatile is the only way to tell the compiler to keep that damn overoptimized variable spilled to the stack.