Merge branch 'resimplify-w32-veh' into 'main'

Re-simplify exception handling on Windows

See merge request GNOME/glib!2031

glib/gwin32-private.c

@@ -18,16 +18,16 @@
 
 /* Copy @cmdline into @debugger, and substitute @pid for `%p`
  * and @event for `%e`.
- * If @debugger_size (in bytes) is overflowed, return %FALSE.
+ * If @debugger_size (in wchar_ts) is overflowed, return %FALSE.
  * Also returns %FALSE when `%` is followed by anything other
  * than `e` or `p`.
  */
 static gboolean
-_g_win32_subst_pid_and_event (char       *debugger,
+_g_win32_subst_pid_and_event_w (wchar_t       *debugger,
-                              gsize       debugger_size,
+                                gsize          debugger_size,
-                              const char *cmdline,
+                                const wchar_t *cmdline,
-                              DWORD       pid,
+                                DWORD          pid,
-                              guintptr    event)
+                                guintptr       event)
 {
   gsize i = 0, dbg_i = 0;
 /* These are integers, and they can't be longer than 20 characters
@@ -35,31 +35,31 @@ _g_win32_subst_pid_and_event (char       *debugger,
  * Use 30 just to be sure.
  */
 #define STR_BUFFER_SIZE 30
-  char pid_str[STR_BUFFER_SIZE] = {0};
+  wchar_t pid_str[STR_BUFFER_SIZE] = {0};
   gsize pid_str_len;
-  char event_str[STR_BUFFER_SIZE] = {0};
+  wchar_t event_str[STR_BUFFER_SIZE] = {0};
   gsize event_str_len;
 
-  _snprintf_s (pid_str, STR_BUFFER_SIZE, G_N_ELEMENTS (pid_str), "%lu", pid);
+  _snwprintf_s (pid_str, STR_BUFFER_SIZE, G_N_ELEMENTS (pid_str), L"%lu", pid);
   pid_str[G_N_ELEMENTS (pid_str) - 1] = 0;
-  pid_str_len = strlen (pid_str);
+  pid_str_len = wcslen (pid_str);
-  _snprintf_s (event_str, STR_BUFFER_SIZE, G_N_ELEMENTS (pid_str), "%Iu", event);
+  _snwprintf_s (event_str, STR_BUFFER_SIZE, G_N_ELEMENTS (pid_str), L"%Iu", event);
   event_str[G_N_ELEMENTS (pid_str) - 1] = 0;
-  event_str_len = strlen (event_str);
+  event_str_len = wcslen (event_str);
 #undef STR_BUFFER_SIZE
 
   while (cmdline[i] != 0 && dbg_i < debugger_size)
     {
-      if (cmdline[i] != '%')
+      if (cmdline[i] != L'%')
         debugger[dbg_i++] = cmdline[i++];
-      else if (cmdline[i + 1] == 'p')
+      else if (cmdline[i + 1] == L'p')
         {
           gsize j = 0;
           while (j < pid_str_len && dbg_i < debugger_size)
             debugger[dbg_i++] = pid_str[j++];
           i += 2;
         }
-      else if (cmdline[i + 1] == 'e')
+      else if (cmdline[i + 1] == L'e')
         {
           gsize j = 0;
           while (j < event_str_len && dbg_i < debugger_size)

glib/gwin32.c

@@ -1035,10 +1035,32 @@ g_console_win32_init (void)
  * it will be non-NULL. Only used to later de-install the handler
  * on library de-initialization.
  */
-static void *WinVEH_handle = NULL;
+static void        *WinVEH_handle = NULL;
+
+#define             DEBUGGER_BUFFER_SIZE (MAX_PATH + 1)
+/* This is the debugger that we'll run on crash */
+static wchar_t      debugger[DEBUGGER_BUFFER_SIZE];
+
+static gsize        number_of_exceptions_to_catch = 0;
+static DWORD       *exceptions_to_catch = NULL;
+
+static HANDLE       debugger_wakeup_event = 0;
+static DWORD        debugger_spawn_flags = 0;
 
 #include "gwin32-private.c"
 
+static char *
+copy_chars (char       *buffer,
+            gsize      *buffer_size,
+            const char *to_copy)
+{
+  gsize copy_count = MIN (strlen (to_copy), *buffer_size - 1);
+  memset (buffer, 0x20, copy_count);
+  strncpy_s (buffer, *buffer_size, to_copy, _TRUNCATE);
+  *buffer_size -= copy_count;
+  return &buffer[copy_count];
+}
+
 /* Handles exceptions (useful for debugging).
  * Issues a DebugBreak() call if the process is being debugged (not really
  * useful - if the process is being debugged, this handler won't be invoked
@@ -1068,24 +1090,31 @@ static void *WinVEH_handle = NULL;
  * or for control flow.
  *
  * This function deliberately avoids calling any GLib code.
+ * This is done on purpose. This function can be called when the program
+ * is in a bad state (crashing). It can also be called very early, as soon
+ * as the handler is installed. Therefore, it's imperative that
+ * it does as little as possible. Preferably, all the work that can be
+ * done in advance (when the program is not crashing yet) should be done
+ * in advance.
  */
 static LONG __stdcall
 g_win32_veh_handler (PEXCEPTION_POINTERS ExceptionInfo)
 {
   EXCEPTION_RECORD    *er;
-  char                 debugger[MAX_PATH + 1];
+  gsize                i;
-  WCHAR               *debugger_utf16;
-  const char          *debugger_env = NULL;
-  const char          *catch_list;
-  gboolean             catch = FALSE;
   STARTUPINFOW         si;
   PROCESS_INFORMATION  pi;
-  HANDLE               event;
+#define ITOA_BUFFER_SIZE 100
-  SECURITY_ATTRIBUTES  sa;
+  char                 itoa_buffer[ITOA_BUFFER_SIZE];
+#define DEBUG_STRING_SIZE 1024
+  gsize                dbgs = DEBUG_STRING_SIZE;
+  char                 debug_string[DEBUG_STRING_SIZE];
+  char                *dbgp;
 
   if (ExceptionInfo == NULL ||
       ExceptionInfo->ExceptionRecord == NULL ||
-      IsDebuggerPresent ())
+      IsDebuggerPresent () ||
+      debugger[0] == 0)
     return EXCEPTION_CONTINUE_SEARCH;
 
   er = ExceptionInfo->ExceptionRecord;
@@ -1097,102 +1126,22 @@ g_win32_veh_handler (PEXCEPTION_POINTERS ExceptionInfo)
     case EXCEPTION_ILLEGAL_INSTRUCTION:
       break;
     default:
-      catch_list = g_getenv ("G_VEH_CATCH");
+      for (i = 0; i < number_of_exceptions_to_catch; i++)
+        if (exceptions_to_catch[i] == er->ExceptionCode)
+          break;
 
-      while (!catch &&
+      if (i == number_of_exceptions_to_catch)
-             catch_list != NULL &&
+        return EXCEPTION_CONTINUE_SEARCH;
-             catch_list[0] != 0)
-        {
-          unsigned long  catch_code;
-          char          *end;
-          errno = 0;
-          catch_code = strtoul (catch_list, &end, 16);
-          if (errno != NO_ERROR)
-            break;
-          catch_list = end;
-          if (catch_list != NULL && catch_list[0] == ',')
-            catch_list++;
-          if (catch_code == er->ExceptionCode)
-            catch = TRUE;
-        }
 
-      if (catch)
-        break;
-
-      return EXCEPTION_CONTINUE_SEARCH;
-    }
-
-  fprintf_s (stderr,
-             "Exception code=0x%lx flags=0x%lx at 0x%p",
-             er->ExceptionCode,
-             er->ExceptionFlags,
-             er->ExceptionAddress);
-
-  switch (er->ExceptionCode)
-    {
-    case EXCEPTION_ACCESS_VIOLATION:
-      fprintf_s (stderr,
-                 ". Access violation - attempting to %s at address 0x%p\n",
-                 er->ExceptionInformation[0] == 0 ? "read data" :
-                 er->ExceptionInformation[0] == 1 ? "write data" :
-                 er->ExceptionInformation[0] == 8 ? "execute data" :
-                 "do something bad",
-                 (void *) er->ExceptionInformation[1]);
-      break;
-    case EXCEPTION_IN_PAGE_ERROR:
-      fprintf_s (stderr,
-                 ". Page access violation - attempting to %s at address 0x%p with status %Ix\n",
-                 er->ExceptionInformation[0] == 0 ? "read from an inaccessible page" :
-                 er->ExceptionInformation[0] == 1 ? "write to an inaccessible page" :
-                 er->ExceptionInformation[0] == 8 ? "execute data in page" :
-                 "do something bad with a page",
-                 (void *) er->ExceptionInformation[1],
-                 er->ExceptionInformation[2]);
-      break;
-    default:
-      fprintf_s (stderr, "\n");
       break;
     }
 
-  fflush (stderr);
-
-  debugger_env = g_getenv ("G_DEBUGGER");
-
-  if (debugger_env == NULL)
-    return EXCEPTION_CONTINUE_SEARCH;
-
-  /* Create an inheritable event */
   memset (&si, 0, sizeof (si));
   memset (&pi, 0, sizeof (pi));
-  memset (&sa, 0, sizeof (sa));
   si.cb = sizeof (si);
-  sa.nLength = sizeof (sa);
-  sa.bInheritHandle = TRUE;
-  event = CreateEvent (&sa, FALSE, FALSE, NULL);
-
-  /* Put process ID and event handle into debugger commandline */
-  if (!_g_win32_subst_pid_and_event (debugger, G_N_ELEMENTS (debugger),
-                                     debugger_env, GetCurrentProcessId (),
-                                     (guintptr) event))
-    {
-      CloseHandle (event);
-      return EXCEPTION_CONTINUE_SEARCH;
-    }
-  debugger[MAX_PATH] = '\0';
-
-  debugger_utf16 = g_utf8_to_utf16 (debugger, -1, NULL, NULL, NULL);
 
   /* Run the debugger */
-  if (0 != CreateProcessW (NULL,
+  if (0 != CreateProcessW (NULL, debugger, NULL, NULL, TRUE, debugger_spawn_flags, NULL, NULL, &si, &pi))
-                           debugger_utf16,
-                           NULL,
-                           NULL,
-                           TRUE,
-                           g_getenv ("G_DEBUGGER_OLD_CONSOLE") != NULL ? 0 : CREATE_NEW_CONSOLE,
-                           NULL,
-                           NULL,
-                           &si,
-                           &pi))
     {
       CloseHandle (pi.hProcess);
       CloseHandle (pi.hThread);
@@ -1202,13 +1151,67 @@ g_win32_veh_handler (PEXCEPTION_POINTERS ExceptionInfo)
        * up forever in case the debugger does not support
        * event signalling.
        */
-      WaitForSingleObject (event, 60000);
+      WaitForSingleObject (debugger_wakeup_event, 60000);
+
+      dbgp = &debug_string[0];
+
+      dbgp = copy_chars (dbgp, &dbgs, "Exception code=0x");
+      itoa_buffer[0] = 0;
+      _ui64toa_s (er->ExceptionCode, itoa_buffer, ITOA_BUFFER_SIZE, 16);
+      dbgp = copy_chars (dbgp, &dbgs, itoa_buffer);
+      dbgp = copy_chars (dbgp, &dbgs, " flags=0x");
+      itoa_buffer[0] = 0;
+      _ui64toa_s (er->ExceptionFlags, itoa_buffer, ITOA_BUFFER_SIZE, 16);
+      dbgp = copy_chars (dbgp, &dbgs, itoa_buffer);
+      dbgp = copy_chars (dbgp, &dbgs, " at 0x");
+      itoa_buffer[0] = 0;
+      _ui64toa_s ((guintptr) er->ExceptionAddress, itoa_buffer, ITOA_BUFFER_SIZE, 16);
+      dbgp = copy_chars (dbgp, &dbgs, itoa_buffer);
+
+      switch (er->ExceptionCode)
+        {
+        case EXCEPTION_ACCESS_VIOLATION:
+          dbgp = copy_chars (dbgp, &dbgs, ". Access violation - attempting to ");
+          if (er->ExceptionInformation[0] == 0)
+            dbgp = copy_chars (dbgp, &dbgs, "read data");
+          else if (er->ExceptionInformation[0] == 1)
+            dbgp = copy_chars (dbgp, &dbgs, "write data");
+          else if (er->ExceptionInformation[0] == 8)
+            dbgp = copy_chars (dbgp, &dbgs, "execute data");
+          else
+            dbgp = copy_chars (dbgp, &dbgs, "do something bad");
+          dbgp = copy_chars (dbgp, &dbgs, " at address 0x");
+          itoa_buffer[0] = 0;
+          _ui64toa_s (er->ExceptionInformation[1], itoa_buffer, ITOA_BUFFER_SIZE, 16);
+          dbgp = copy_chars (dbgp, &dbgs, itoa_buffer);
+          break;
+        case EXCEPTION_IN_PAGE_ERROR:
+          dbgp = copy_chars (dbgp, &dbgs, ". Page access violation - attempting to ");
+          if (er->ExceptionInformation[0] == 0)
+            dbgp = copy_chars (dbgp, &dbgs, "read from an inaccessible page");
+          else if (er->ExceptionInformation[0] == 1)
+            dbgp = copy_chars (dbgp, &dbgs, "write to an inaccessible page");
+          else if (er->ExceptionInformation[0] == 8)
+            dbgp = copy_chars (dbgp, &dbgs, "execute data in page");
+          else
+            dbgp = copy_chars (dbgp, &dbgs, "do something bad with a page");
+          dbgp = copy_chars (dbgp, &dbgs, " at address 0x");
+          itoa_buffer[0] = 0;
+          _ui64toa_s (er->ExceptionInformation[1], itoa_buffer, ITOA_BUFFER_SIZE, 16);
+          dbgp = copy_chars (dbgp, &dbgs, itoa_buffer);
+          dbgp = copy_chars (dbgp, &dbgs, " with status ");
+          itoa_buffer[0] = 0;
+          _ui64toa_s (er->ExceptionInformation[2], itoa_buffer, ITOA_BUFFER_SIZE, 16);
+          dbgp = copy_chars (dbgp, &dbgs, itoa_buffer);
+          break;
+        default:
+          break;
+        }
+
+      dbgp = copy_chars (dbgp, &dbgs, "\n");
+      OutputDebugStringA (debug_string);
     }
 
-  g_free (debugger_utf16);
-
-  CloseHandle (event);
-
   /* Now the debugger is present, and we can try
    * resuming execution, re-triggering the exception,
    * which will be caught by debugger this time around.
@@ -1219,20 +1222,88 @@ g_win32_veh_handler (PEXCEPTION_POINTERS ExceptionInfo)
   return EXCEPTION_CONTINUE_SEARCH;
 }
 
+static gsize
+parse_catch_list (const wchar_t *catch_buffer,
+                  DWORD         *exceptions,
+                  gsize          num_exceptions)
+{
+  const wchar_t *catch_list = catch_buffer;
+  gsize          result = 0;
+  gsize          i = 0;
+
+  while (catch_list != NULL &&
+         catch_list[0] != 0)
+    {
+      unsigned long  catch_code;
+      wchar_t       *end;
+      errno = 0;
+      catch_code = wcstoul (catch_list, &end, 16);
+      if (errno != NO_ERROR)
+        break;
+      catch_list = end;
+      if (catch_list != NULL && catch_list[0] == L',')
+        catch_list++;
+      if (exceptions && i < num_exceptions)
+        exceptions[i++] = catch_code;
+    }
+
+  return result;
+}
+
 void
 g_crash_handler_win32_init (void)
 {
+  wchar_t      debugger_env[DEBUGGER_BUFFER_SIZE];
+#define CATCH_BUFFER_SIZE 1024
+  wchar_t      catch_buffer[CATCH_BUFFER_SIZE];
+  SECURITY_ATTRIBUTES  sa;
+
   if (WinVEH_handle != NULL)
     return;
 
   /* Do not register an exception handler if we're not supposed to catch any
    * exceptions. Exception handlers are considered dangerous to use, and can
    * break advanced exception handling such as in CLRs like C# or other managed
-   * code. See: https://blogs.msdn.microsoft.com/jmstall/2006/05/24/beware-of-the-vectored-exception-handler-and-managed-code/
+   * code. See: http://www.windows-tech.info/13/785f590867bd6316.php
    */
-  if (g_getenv ("G_DEBUGGER") == NULL && g_getenv("G_VEH_CATCH") == NULL)
+  debugger_env[0] = 0;
+  if (!GetEnvironmentVariableW (L"G_DEBUGGER", debugger_env, DEBUGGER_BUFFER_SIZE))
     return;
 
+  /* Create an inheritable event */
+  memset (&sa, 0, sizeof (sa));
+  sa.nLength = sizeof (sa);
+  sa.bInheritHandle = TRUE;
+  debugger_wakeup_event = CreateEvent (&sa, FALSE, FALSE, NULL);
+
+  /* Put process ID and event handle into debugger commandline */
+  if (!_g_win32_subst_pid_and_event_w (debugger, G_N_ELEMENTS (debugger),
+                                       debugger_env, GetCurrentProcessId (),
+                                       (guintptr) debugger_wakeup_event))
+    {
+      CloseHandle (debugger_wakeup_event);
+      debugger_wakeup_event = 0;
+      debugger[0] = 0;
+      return;
+    }
+  debugger[MAX_PATH] = L'\0';
+
+  catch_buffer[0] = 0;
+  if (GetEnvironmentVariableW (L"G_VEH_CATCH", catch_buffer, CATCH_BUFFER_SIZE))
+    {
+      number_of_exceptions_to_catch = parse_catch_list (catch_buffer, NULL, 0);
+      if (number_of_exceptions_to_catch > 0)
+        {
+          exceptions_to_catch = g_new0 (DWORD, number_of_exceptions_to_catch);
+          parse_catch_list (catch_buffer, exceptions_to_catch, number_of_exceptions_to_catch);
+        }
+    }
+
+  if (GetEnvironmentVariableW (L"G_DEBUGGER_OLD_CONSOLE", (wchar_t *) &debugger_spawn_flags, 1))
+    debugger_spawn_flags = 0;
+  else
+    debugger_spawn_flags = CREATE_NEW_CONSOLE;
+
   WinVEH_handle = AddVectoredExceptionHandler (0, &g_win32_veh_handler);
 }
 

glib/tests/win32.c

@@ -33,34 +33,40 @@ static char *argv0 = NULL;
 static void
 test_subst_pid_and_event (void)
 {
-  const gchar not_enough[] = "too long when %e and %p are substituted";
+  const wchar_t not_enough[] = L"too long when %e and %p are substituted";
-  gchar debugger_3[3];
+  wchar_t debugger_3[3];
-  gchar debugger_not_enough[G_N_ELEMENTS (not_enough)];
+  wchar_t debugger_not_enough[G_N_ELEMENTS (not_enough)];
-  gchar debugger_enough[G_N_ELEMENTS (not_enough) + 1];
+  wchar_t debugger_enough[G_N_ELEMENTS (not_enough) + 1];
-  gchar debugger_big[65535] = {0};
+  char *debugger_enough_utf8;
+  wchar_t debugger_big[65535] = {0};
+  char *debugger_big_utf8;
   gchar *output;
   guintptr be = (guintptr) 0xFFFFFFFF;
   DWORD bp = G_MAXSIZE;
 
   /* %f is not valid */
-  g_assert_false (_g_win32_subst_pid_and_event (debugger_3, G_N_ELEMENTS (debugger_3),
+  g_assert_false (_g_win32_subst_pid_and_event_w (debugger_3, G_N_ELEMENTS (debugger_3),
-                                                "%f", 0, 0));
+                                                  L"%f", 0, 0));
 
-  g_assert_false (_g_win32_subst_pid_and_event (debugger_3, G_N_ELEMENTS (debugger_3),
+  g_assert_false (_g_win32_subst_pid_and_event_w (debugger_3, G_N_ELEMENTS (debugger_3),
-                                                "string longer than 10", 0, 0));
+                                                  L"string longer than 10", 0, 0));
   /* 200 is longer than %e, so the string doesn't fit by 1 byte */
-  g_assert_false (_g_win32_subst_pid_and_event (debugger_not_enough, G_N_ELEMENTS (debugger_not_enough),
+  g_assert_false (_g_win32_subst_pid_and_event_w (debugger_not_enough, G_N_ELEMENTS (debugger_not_enough),
-                                                "too long when %e and %p are substituted", 10, 200));
+                                                  not_enough, 10, 200));
 
   /* This should fit */
-  g_assert_true (_g_win32_subst_pid_and_event (debugger_enough, G_N_ELEMENTS (debugger_enough),
+  g_assert_true (_g_win32_subst_pid_and_event_w (debugger_enough, G_N_ELEMENTS (debugger_enough),
-                                                "too long when %e and %p are substituted", 10, 200));
+                                                 not_enough, 10, 200));
-  g_assert_cmpstr (debugger_enough, ==, "too long when 200 and 10 are substituted");
+  debugger_enough_utf8 = g_utf16_to_utf8 (debugger_enough, -1, NULL, NULL, NULL);
+  g_assert_cmpstr (debugger_enough_utf8, ==, "too long when 200 and 10 are substituted");
+  g_free (debugger_enough_utf8);
 
-  g_assert_true (_g_win32_subst_pid_and_event (debugger_big, G_N_ELEMENTS (debugger_big),
+  g_assert_true (_g_win32_subst_pid_and_event_w (debugger_big, G_N_ELEMENTS (debugger_big),
-                                                "multipl%e big %e %entries and %pids are %provided here", bp, be));
+                                                 L"multipl%e big %e %entries and %pids are %provided here", bp, be));
+  debugger_big_utf8 = g_utf16_to_utf8 (debugger_big, -1, NULL, NULL, NULL);
   output = g_strdup_printf ("multipl%llu big %llu %lluntries and %luids are %lurovided here", (guint64) be, (guint64) be, (guint64) be, bp, bp);
-  g_assert_cmpstr (debugger_big, ==, output);
+  g_assert_cmpstr (debugger_big_utf8, ==, output);
+  g_free (debugger_big_utf8);
   g_free (output);
 }
 
@@ -95,7 +101,6 @@ test_veh_crash_access_violation (void)
   /* Run a test that crashes */
   g_test_trap_subprocess ("/win32/subprocess/access_violation", 0, 0);
   g_test_trap_assert_failed ();
-  g_test_trap_assert_stderr ("Exception code=0xc0000005*");
 }
 
 static void
@@ -105,20 +110,10 @@ test_veh_crash_illegal_instruction (void)
   /* Run a test that crashes */
   g_test_trap_subprocess ("/win32/subprocess/illegal_instruction", 0, 0);
   g_test_trap_assert_failed ();
-  g_test_trap_assert_stderr ("Exception code=0xc000001d*");
 }
 
 static void
 test_veh_debug (void)
-{
-  /* Run a test that crashes and runs a debugger */
-  g_test_trap_subprocess ("/win32/subprocess/debuggee", 0, 0);
-  g_test_trap_assert_failed ();
-  g_test_trap_assert_stderr ("Exception code=0xc0000005*Debugger invoked, attaching to*");
-}
-
-static void
-test_veh_debuggee (void)
 {
   /* Set up a debugger to be run on crash */
   gchar *command = g_strdup_printf ("%s %s", argv0, "%p %e");
@@ -129,6 +124,15 @@ test_veh_debuggee (void)
    */
   g_setenv ("G_DEBUGGER_OLD_CONSOLE", "1", TRUE);
   g_free (command);
+  /* Run a test that crashes and runs a debugger */
+  g_test_trap_subprocess ("/win32/subprocess/debuggee", 0, 0);
+  g_test_trap_assert_failed ();
+  g_test_trap_assert_stderr ("Debugger invoked, attaching to*");
+}
+
+static void
+test_veh_debuggee (void)
+{
   /* Crash */
   test_access_violation ();
 }