| Date: | January 17, 2006 / year-entry #23 |
| Tags: | code |
| Orig Link: | https://blogs.msdn.microsoft.com/oldnewthing/20060117-14/?p=32633 |
| Comments: | 32 |
| Summary: | Occasionally I see someone trying to use the ReadProcessMemory function as an inter-process communication mechanism. This is ill-advised for several reasons. First, you cannot use ReadProcessMemory across security contexts, at least not without doing some extra work. If somebody uses "runas" to run your program under a different identity, your two processes will not be... |
Occasionally I see someone trying to use the First, you cannot use You could go to the extra work to get What's more, once you grant What? Granting read access can corrupt your stack? If a process grows its stack into the stack guard page, the unhandled exception filter catches the guard exception and extends the stack. But when it happen inside a private "catch all exceptions" handler, such as the one that the You might think you could catch the stack access violation and try to shut down the thread cleanly, but that is not possible for multiple reasons. First, structured exception handling executes on the stack of the thread that encountered the exception. If that thread has a corrupted stack, it becomes impossible to dispatch that exception since the stack that the exception filters want to run on is no longer viable. Even if you could somehow run these exception filters on some sort of "emergency stack", you still can't fix the problem. At the point of the exception, the thread could be in the middle of anything. Maybe it was inside the heap manager with the heap lock held and with heap data structures in a state of flux. In order for the process to stay alive, the heap data structures need to be made consistent and the heap lock released. But you don't know how to do that. There are plenty of other inter-process communication mechanisms available to you. One of them is anonymous shared memory, which I discussed a few years ago. Anonymous shared memory still has the problem that any process running under the same token as the one you are communicating with can read the shared memory block, but at least the scope of the exposure is limited to the data you explicitly wanted to share. (In a sense, you can't do any better than that. The process you are communicating with can do anything it wants with the data once it gets it from you. Even if you somehow arranged so that only the destination process can access the memory, there's nothing stopping that destination process from copying it somewhere outside your shared memory block, at which point your data can be read from the destination process by anybody running with the same token anyway.) |
Comments (32)
Comments are closed. |
A brief explanation of how stack guard pages work would have clarified the stack-corruption point a lot. Well, actually it would’ve just saved me the trouble of reading Osterberg’s post and (horrors!) thinking it through. Oh, never mind. Cool post!
But… This can’t be "exception handling" in the same sense as the C++ feature, where the stack is unwound and so on, because it makes no sense to unwind the whole stack and start over every time the stack grows. Am I correct in understanding that this guard-page-stack-growth thing is something that happens normally in healthy processes? And is the exception being trapped in one place for the whole process, not at each and every function call?
So are structured "catch everything" handlers just a really bad idea?
Any chance of a post going into all the gory detail on stack guard pages?
Here’s a thing that’s helpful, but it doesn’t address the stack-growth issue directly:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/memory/base/creating_guard_pages.asp
I have the same question as Boor, does this mean that
__try
{
}
__except ( EXCEPTION_EXECUTE_HANDLER )
{
}
is a really bad idea?
Furthermore, this makes me wonder how one checks for a bad pointer (for read, write, etc) without causing bad problems with the system? It sounds like you can’t (really) trust IsBadReadPtr or IsBadWritePtr.
Are the better solutions that I’m missing?
Ok, I get how the current implementation of IsBadReadPtr is bad if you pass it mem that overlaps a PAGE_GUARD page, but I don’t get how PROCESS_VM_READ or ReadProcessMemory fits in with this.
Boor: exception handling in this context means Structured Exception Handling (SEH). It’s a completely different beast than C++’s exception model. It’s more closely related to C’s signal handlers (infact the CRT implements those using SEH) except it sets up a EH chain instead of making you create and register some random function. For more info search for __try, __except, GetExceptionCode, and UnhandledExceptionFilter on MSDN and in the CRT source code (the code for _alloca and _resetstkoflw are helpful too). A less than stellar article that talks about how stack overflow works is Q315937: http://support.microsoft.com/default.aspx?scid=kb;en-us;315937
I had a hard time understanding what the problem is with stack guard pages, because there’s not enough information in the posting to understand exactly what happens when the guard page is access (and that’s certainly not something I know off the top of my head).
Anyway, for anyone else who’s in that boat, here’s a good article:
http://msdn.microsoft.com/archive/en-us/dnaraskdr/html/drgui49.asp
However, it’s a probably a better idea to use the _resetstkoflw() function than to use the inline assembly the article describes to fix the problem.
What about this ancient chestnut using shared memory, between multiple instances of a program? Is this dangerous too?
#pragma data_seg(".shared")
HTASK ghTask1=0;
#pragma data_seg()
#pragma comment(linker, "/SECTION:.shared,RWS")
Never mind, Raymond. I found your answer:
http://blogs.msdn.com/oldnewthing/archive/2004/08/04/208003.aspx
Tony,
It’s not because they are not thread-safe that they are broken.
if a thread executes a=b+c; and another thread modifies b before the result of the addition is copied into a, does it mean that the ADD opcode is broken too ?
And as far as the purpose of IsBadReadPtr is concerned : Debugging code and assertions come to mind.
I don’t think ReadProcessMemory can interfere with guard pages. When handling access faults on guard pages, the memory manager checks to see if the current thread belongs to the same process. If not, it doesn’t clear the guard bit. ReadProcessMemory on a guard page in a different process should return an error (probably something like ERROR_PARTIAL_COPY) but there should be no bad side effects.
I believe it works this way to make life simpler for debuggers and other tools that might have a legitimate reason to inspect other process’ memory.
IsBadXxxPtr on the other hand is evil, no doubt about that.
Serge,
In the example you cite, your application can either know that the add operation is operating on memory which is not being accessed by another thread, or can appropriately wrap it in synchronization primitives. That’s because your application controls where the arguments to the add are located.
In the case of IsBad*Ptr, the implicit assumption is that the argument you pass isn’t under your control (otherwise you’d already "know" whether it was valid), in which case you really do have to worry about whether its validity might change the moment you get the result back.
I’ve heard the argument that they’re useful for asserts, but honestly, all you can do is drop into the debugger anyway so why not just let the offending pointer access crash? (My mantra is always to crash as early as possible, rather than attempt to soldier on in a bad situation – it just makes it easier to find problems.)
So does this mean that using an Unhandled Exception Filter to do (say) Crash dump generation is a bad idea, because in the case where there’s a stack overflow, there’s no way to expand the stack safely and in the way that the OS expects?
That.. erm… scares me. Recently for work, I put together a great little crash catcher utility – when a crash occurs, it gets caught in the UEF which fires up a dormant thread and puts together a small interchange file with details about the thread context, exception records, process id, etc, of the crashed process. Then it spawns a helper process, and suspends all of its threads but the crash catcher thread which waits for the helper to exit.
The helper then digs into the crashed process using ReadProcessMemory to pull configuration information and other stuff out of it, creates a minidump, and automatically emails the stack trace to a mailing list, and sticks the minidump on a server for post-mortem debugging use.
However, now you’ve got me wondering if any thread runs out of stack space and needs to grow the stack, will my app die in a horrible unrecoverable way?
Any advice?
This by the way is also incorrect:
> the unhandled exception filter catches the guard
> exception and extends the stack
The stack is extended by Mm in the kernel mode access fault handler. If everything goes fine, no exception is raised to user mode. If stack cannot be extended for some reason then user mode will get a stack overflow exception.
Simon – reliably doing anything after a stack overflow is difficult. Normally you have a bit less than a page of stack space left for exception handlers to run, and this is enough for most purposes (the default UEF should work for example). You can also increase the amount of available space with the new SetThreadStackGuarantee API.
In general however it’s best to avoid stack overflows altogether. All critical Windows components commit enough stack space upfront so that they never even need to extend the stack (which can result in a stack overflow if the system is running out of memory).
IsBadReadPtr and IsBadWritePtr are broken, with essentially no good way to fix them when you have multiple threads.
The reason is that in the presence of multiple threads there is no way to be sure that whatever result the function returns hasn’t been invalidated by the time the next instruction gets executed. The thread calling IsBad*Ptr could have been pre-empted by another thread, perhaps freeing the memory being tested.
Frankly, if your code is calling IsBad*Ptr, you really need to ask yourself why. What are you going to do with the result? Presumeably you are going to try to prevent yourself from accessing memory which may be bad, but why did you get into the situation of not knowing whether your memory is valid in the first place?
Tuesday, January 17, 2006 1:50 PM by Tony Cox [MS]
> but why did you get into the situation of
> not knowing whether your memory is valid in
> the first place?
Because you might not be (or might not yet be) the writer of the code that calls your DLL.
Tuesday, January 17, 2006 6:44 PM by Tony Cox [MS]
> I’ve heard the argument that they’re useful
> for asserts, but honestly, all you can do is
> drop into the debugger anyway so why not
> just let the offending pointer access crash?
> (My mantra is always to crash as early as
> possible,
Bingo. First check the arguments that the caller passed you and crash immediately, instead of starting some processing that will result in a corrupt state when you crash on a later access through the pointer.
Oops, I meant to say "Tony’s point" not "Simon’s point"… sorry :-)
In a public API exported from a library, using IsBadReadPtr() and IsBadWritePtr() can allow you to catch bad pointers and display an easily diagnosable error to the client, instead of crashing in an obscure place where the execution path may be difficult to follow — assembly without frame pointers, or maybe even a different thread. Sure, you can make it crash if you really try, but 99% of the time you’d get a breakpoint right inside the API or an assert in a debug build.
There is a more serious issue with IsBadWritePtr(): it can actually corrupt data that is being modified by another thread since it uses a non-atomic read-write pair of operations to test, MOV+MOV. LOCK ADD mem,0 would have avoided this problem.
Doing error checking early is still better than not doing error checking early. Everyone seems to agree (even if one only agrees half the time) that in situations where we can catch an error early then it is better to do so. Even though there exist situations where we can’t catch the error early due to some other thread shafting ours, there exist situations where we can catch the error early due to our caller passing us a pointer that was already bad.
Sorry for two in a row but I hope this will explain it better:
Just because some bugs are hard to debug, that doesn’t mean we have to make all bugs hard to debug.
As Tony says, the difference between "a = b + c" and IsBad*Ptr is that "a = b + c" can be *made* correct in multi-threaded situations with the proper synchronization, whereas IsBad*Ptr cannot.
Besides, there’s not much point using IsBad*Ptr for debugging/assertions when actually trying to read/write the memory will break you into the debugger anyway. What would an extra check give you?
Many people write code where they return something like E_POINTER if IsBadXxxPtr fails. I think we all agree that this is a really bad idea.
If you use IsBadXxxPtr simply to assert on bad parameters, and you never return an error without making sure it gets noticed (you could raise an exception, or create a crash dump etc) then it’s somewhat better but still not very nice. One problem is that you don’t have any information about why the call failed. By the time you start investigating the failing address could already be valid. Did it fail originally because of an access violation, a guard page exception, an inpage error, or something else?
Just using the pointer will probably be easier to debug because you will know exactly what kind of exception it was.
Read my entire post. If you don’t control your input, then the IsBad*Ptr APIs don’t give reliable results, because another thread could invalidate the pointer at any time.
And if you plan on crashing, just access the memory and crash. Why futz around with API calls like IsBadWritePtr at all?
The problem with the IsBad*Ptr APIs is that while they purport to give you a method of testing the validity of memory without actually accessing it (and hence potentially inducing a crash), in reality they don’t reliably offer this functionality in the one case you might want it (when you are being passed foreign pointers from outside your DLL).
If want you want to do is drop into the debugger when you are handed a bad pointer, then just try to dereference the pointer, don’t call an API which has the potential to give you an incorrect result.
It’s worse than broken. It’s impossible to make correct.
Stay far away.
If caller of a function is lying about the size of an [out] buffer, a better approach is to just memset the entire buffer to a known pattern before writing data to it. I believe AppVerifier does this for some of the string APIs.
Again, simply using the buffer is much better than what you would get with IsBadXxxPtr.
Purpleh: Another rather crucial difference is that the underlying use SEH in C++ exception handling on win32 is an implementation detail. C++ exceptions are portable; relying on SEH’s __try and __catch most certainly is not.
IsBadXxxPtr should be useful in buffer cases where you may only write to part of the buffer. Such as:
int GetName( char *buf, int maxLen ) {
assert( !IsBadWritePtr( buf, maxLen ) );
// …
return lenUsed;
}
where the assert catches cases where the length passed does not match the actual buffer size. Without the assert, such cases will only be caught if the actual length is smaller than the length used.
It should also be useful in cases like:
void OptionalGet( int *pResult ) {
assert( !IsBadWritePtr( pResult, sizeof(int) );
if (!rand())
*pResult = 42;
}
where the assert catches cases where rand() returns non-zero.
In both examples IsBadWritePtr can help detect the bug early. I am surprised to hear it is broken.
Dave, writing outside the bounds of an array does *not* necessarily result in an invalid pointer. If it did, buffer overruns would just be a nuisance instead of potential security problems.
And if your own application is lying to you about the length of the buffer (and who else but your own application would be calling your function?), you’ve got serious problems that IsBad*Ptr isn’t going to fix.
To top it all off, IsBad*Ptr is slow (it just tries to read/write and then catches the exception if it occurs). If you were doing that every time you had an array read/write, you’d probably cause a noticable impact on application performance.
> Dave, writing outside the bounds of an array
> does *not* necessarily result in an invalid
> pointer.
I didn’t say it did.
> And if your own application is lying to you
> about the length of the buffer (and who else
> but your own application would be calling
> your function?), you’ve got serious problems
> that IsBad*Ptr isn’t going to fix.
Look at the code. IsBad*Ptr isn’t trying to fix the problem. It is trying to detect the problem. It won’t always succeed, but it’s better than nothing.
> To top it all off, IsBad*Ptr is slow (it
> just tries to read/write and then catches
> the exception if it occurs). If you were
> doing that every time you had an array
> read/write, you’d probably cause a noticable
> impact on application performance.
Look at the code. It’s only called from within an assert, which means it cannot affect the performance of a RELEASE build.