Saving errno and GetLastError in ProtSEHFilter on Windows

[fstromback]

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.

[gareth-rees]

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 AddVectoredExceptionHandler doesn't mention it (unlike in the case of sigaction) but that's not dispositive as none of the vectored exception handling examples call any Windows API functions.

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.

[fstromback]

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:

Final contents: 10
Error before write: 487
Error after write:  998

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.

[gareth-rees]

@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.)