-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Saving errno and GetLastError in ProtSEHFilter on Windows #61
Comments
Hi @fstromback, thanks for raising this issue. Do you know for sure that Windows vectored exception handlers need to save and restore the error values? The documentation for I'm not in a position to investigate this at the moment, so please go ahead and see if you can figure out if vectored exception handlers need to save & restore error values. |
Thank you for your reply, @gareth-rees ! I wrote a small test program to inspect the behaviour on Windows: #include <Windows.h>
#include <iostream>
using namespace std;
size_t page_size;
void *memory;
LONG WINAPI handle_exception(struct _EXCEPTION_POINTERS *info) {
EXCEPTION_RECORD *record = info->ExceptionRecord;
if (record->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) {
ULONG_PTR addr = record->ExceptionInformation[1];
ULONG_PTR mem = (ULONG_PTR)memory;
if (addr >= mem && addr < mem + page_size) {
DWORD old;
VirtualProtect(memory, page_size, PAGE_READWRITE, &old);
// Call that will fail, and thus modify the value from GetLastError()
VirtualProtect(0, 0, 0, 0);
return EXCEPTION_CONTINUE_EXECUTION;
}
}
return EXCEPTION_CONTINUE_SEARCH;
}
int main() {
AddVectoredExceptionHandler(1, &handle_exception);
{
SYSTEM_INFO info;
GetSystemInfo(&info);
page_size = info.dwPageSize;
}
memory = VirtualAlloc(NULL, page_size, MEM_RESERVE | MEM_COMMIT, PAGE_NOACCESS);
// This call will fail with an error code (different to what is in the "handle_exception" function).
VirtualAlloc(memory, page_size, MEM_RESERVE, PAGE_NOACCESS);
DWORD before = GetLastError();
*(int *)memory = 10;
DWORD after = GetLastError();
cout << "Final contents: " << *(int *)memory << endl;
cout << "Error before write: " << before << endl;
cout << "Error after write: " << after << endl;
return 0;
} When I run it, I get the following output:
I get the same behaviour when I use SetLastError rater than calling system calls that fail, so the case of system calls failing does not seem to be a special case. Another observation from this is that Windows API-functions do not seem to modify GetLastError() unless they fail (I imagine there might be exceptions though). I believe this is different to errno on Linux, as all system calls modify errno, regardless of whether they succeed or not. Based on these observations, the MPS needs to save/restore GetLastError unless we know that the system calls executed inside the exception handler will not fail (VirtualProtect in these circumstances might qualify). What do you think about this? Is this something that should be fixed, or is a situation that is safe anyway? I'm happy to write and submit a patch if you think one is needed. |
@fstromback Your test program convinces me that there is an issue here! Please go ahead and make a pull request fixing it. (See #32 for the steps that are likely to be involved.) |
This is a patch for the problem outlined in issue Ravenbrook#61 - that the value in GetLastError() is not automatically saved and restored when a vectored exception handler is called. This is solved the same way as errno was handled on POSIX systems. This patch does *not* save and restore errno on Windows, since it seems like the MPS does not call any functions that modify errno on the critical path on Windows (generally only functions from POSIX do that).
This is to illustrate that the value in GetLastError() may be clobbered by the exception handler on Windows in some circumstances. As this commit is before the patch, the test currently fails (clearly showing the issue).
This is a patch for the problem outlined in issue Ravenbrook#61 - that the value in GetLastError() is not automatically saved and restored when a vectored exception handler is called. This is solved the same way as errno was handled on POSIX systems. This patch does *not* save and restore errno on Windows, since it seems like the MPS does not call any functions that modify errno on the critical path on Windows (generally only functions from POSIX do that).
This is to illustrate that the value in GetLastError() may be clobbered by the exception handler on Windows in some circumstances. As this commit is before the patch, the test currently fails (clearly showing the issue).
This is a patch for the problem outlined in issue Ravenbrook#61 - that the value in GetLastError() is not automatically saved and restored when a vectored exception handler is called. This is solved the same way as errno was handled on POSIX systems. This patch does *not* save and restore errno on Windows, since it seems like the MPS does not call any functions that modify errno on the critical path on Windows (generally only functions from POSIX do that).
This is a patch for the problem outlined in issue Ravenbrook#61 - that the value in GetLastError() is not automatically saved and restored when a vectored exception handler is called. This is solved the same way as errno was handled on POSIX systems. This patch does *not* save and restore errno on Windows, since it seems like the MPS does not call any functions that modify errno on the critical path on Windows (generally only functions from POSIX do that).
In issue #10 @gareth-rees noted that signal handlers might modify errno on Linux systems, which could confuse client code.
The Windows variety, while it does not use signal handlers, uses an exception filter that fills a similar function. Inside that function, MPS might call Windows API functions (e.g. VirtualProtect) which might modify the value in GetLastError, at least on failure (I don't know if the value there is reset on success).
As such, I was thinking if a similar saving/restoring of the value in GetLastError is required on Windows as well, potentially also the value in errno.
I have not yet observed any issues arising from this potential problem, so I do not know if it is an issue (either in theory or in practice). I can try to investigate further if this is something you do not already have a definitive answer to.
The text was updated successfully, but these errors were encountered: