Save and restore GetLastError() on Windows.

This is a patch for the problem outlined in issue #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).
Ravenbrook · Mar 12, 2021 · 56a1ac8 · 56a1ac8
1 parent eb93fed
commit 56a1ac8
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 44 deletions.
diff --git a/code/protw3.c b/code/protw3.c
@@ -16,6 +16,17 @@
 SRCID(protw3, "$Id$");
 
 
+/* Save and restore the value in GetLastError on Windows. See
+ * <design/thread-manager#.sol.thread.lasterror>
+ *
+ * These macros must be used after #include "mpswin.h" so that
+ * GetLastError() and SetLastError() are defined.
+ */
+
+#define LAST_ERROR_SAVE BEGIN DWORD _saved_last_error = GetLastError(); BEGIN
+#define LAST_ERROR_RESTORE END; SetLastError(_saved_last_error); END
+
+
 void ProtSet(Addr base, Addr limit, AccessSet mode)
 {
   DWORD newProtect;
@@ -51,51 +62,56 @@ LONG WINAPI ProtSEHfilter(LPEXCEPTION_POINTERS info)
   if(er->ExceptionCode != EXCEPTION_ACCESS_VIOLATION)
     return EXCEPTION_CONTINUE_SEARCH;
 
-  MutatorContextInitFault(&context, info);
-
-  /* assert that the exception is continuable */
-  /* Note that Microsoft say that this field should be 0 or */
-  /* EXCEPTION_NONCONTINUABLE, but this is not true */
-  AVER((er->ExceptionFlags & EXCEPTION_NONCONTINUABLE) == 0);
-
-  /* er->ExceptionRecord is pointer to next exception in chain */
-  /* er->ExceptionAddress is where exception occurred */
-
-  AVER(er->NumberParameters >= 2);
-
-  switch (er->ExceptionInformation[0]) {
-  case 0:       /* read */
-  case 8:       /* execute */
-    mode = AccessREAD;
-    break;
-  case 1:       /* write */
-    /* Pages cannot be made write-only, so an attempt to write must
-       also cause a read-access if necessary */
-    mode = AccessREAD | AccessWRITE;
-    break;
-  default:
-    /* <https://docs.microsoft.com/en-gb/windows/desktop/api/winnt/ns-winnt-_exception_record> */
-    NOTREACHED;
-    mode = AccessREAD | AccessWRITE;
-    break;
-  }
-
-  address = er->ExceptionInformation[1];
-
-  base = (Addr)address;
-  limit = AddrAdd(address, sizeof(Addr));
-
-  if(base < limit) {
-    if(ArenaAccess(base, mode, &context))
-      action = EXCEPTION_CONTINUE_EXECUTION;
-    else
+  /* This is the first point where we call a Windows API function that
+   * might change the last error.
+   */
+  LAST_ERROR_SAVE {
+    MutatorContextInitFault(&context, info);
+
+    /* assert that the exception is continuable */
+    /* Note that Microsoft say that this field should be 0 or */
+    /* EXCEPTION_NONCONTINUABLE, but this is not true */
+    AVER((er->ExceptionFlags & EXCEPTION_NONCONTINUABLE) == 0);
+
+    /* er->ExceptionRecord is pointer to next exception in chain */
+    /* er->ExceptionAddress is where exception occurred */
+
+    AVER(er->NumberParameters >= 2);
+
+    switch (er->ExceptionInformation[0]) {
+    case 0:       /* read */
+    case 8:       /* execute */
+      mode = AccessREAD;
+      break;
+    case 1:       /* write */
+      /* Pages cannot be made write-only, so an attempt to write must
+         also cause a read-access if necessary */
+      mode = AccessREAD | AccessWRITE;
+      break;
+    default:
+      /* <https://docs.microsoft.com/en-gb/windows/desktop/api/winnt/ns-winnt-_exception_record> */
+      NOTREACHED;
+      mode = AccessREAD | AccessWRITE;
+      break;
+    }
+
+    address = er->ExceptionInformation[1];
+
+    base = (Addr)address;
+    limit = AddrAdd(address, sizeof(Addr));
+
+    if(base < limit) {
+      if(ArenaAccess(base, mode, &context))
+        action = EXCEPTION_CONTINUE_EXECUTION;
+      else
+        action = EXCEPTION_CONTINUE_SEARCH;
+    } else {
+      /* Access on last sizeof(Addr) (ie 4 on this platform) bytes */
+      /* in memory.  We assume we can't get this page anyway */
+      /* <code/vmw3.c#assume.not-last> so it can't be our fault. */
       action = EXCEPTION_CONTINUE_SEARCH;
-  } else {
-    /* Access on last sizeof(Addr) (ie 4 on this platform) bytes */
-    /* in memory.  We assume we can't get this page anyway */
-    /* <code/vmw3.c#assume.not-last> so it can't be our fault. */
-    action = EXCEPTION_CONTINUE_SEARCH;
-  }
+    }
+  } LAST_ERROR_RESTORE;
 
   return action;
 }

diff --git a/design/thread-manager.txt b/design/thread-manager.txt
@@ -78,6 +78,11 @@ blocked in a system call, and the MPS signal handlers updating
 
 .. _GitHub issue #10: https://github.com/ravenbrook/mps/issues/10
 
+_`.req.thread.lasterror`: It would be nice if on Windows systems the
+MPS does not cause system calls in the mutator to update the value
+returned from ``GetLastError()`` when the exception handler is called
+due to a fault. This may cause the MPS to destroy the previous value
+there.
 
 Design
 ------
@@ -136,6 +141,12 @@ independent thread, should save and restore its value." All MPS
 signals handlers therefore save and restore ``errno`` using the macros
 ``ERRNO_SAVE`` and ``ERRNO_RESTORE``.
 
+_`.sol.thread.lasterror`: The documentation for ``AddVectoredExceptionHandler``
+does not mention ``GetLastError()`` at all, but testing_ the behaviour
+reveals that any value in ``GetLastError()`` is not preserved. Therefore,
+this value is saved using ``LAST_ERROR_SAVE`` and ``LAST_ERROR_RESTORE``.
+
+.. _testing: https://github.com/Ravenbrook/mps/issues/61
 
 Interface
 ---------