From b92b17f0218b693a517966e71f212a8d041e3676 Mon Sep 17 00:00:00 2001 From: Chun-wei Fan Date: Wed, 2 Nov 2022 23:55:48 +0800 Subject: [PATCH 1/5] build: Check for invalid parameter overriding on Windows Allow one to override the invalid parameter handler if we have the following items: * _set_invalid_parameter_hander() or _set_thread_local_parameter_handler() * _CrtSetReportMode() as a function or macro Currently, we are doing this on Visual Studio to allow GSpawn to work on Windows as well as having the log writer support color output, as we might be passing in file descriptors that are invalid, which will cause the CRT to abort unless the default invalid parameter handler is overridden. --- glib/glib-private.h | 6 ++++++ glib/gmessages.c | 6 +++--- glib/gspawn-win32-helper.c | 6 +++--- meson.build | 14 ++++++++++++++ 4 files changed, 26 insertions(+), 6 deletions(-) diff --git a/glib/glib-private.h b/glib/glib-private.h index de9122754..f83f76e76 100644 --- a/glib/glib-private.h +++ b/glib/glib-private.h @@ -121,6 +121,12 @@ GMainContext * g_get_worker_context (void); gboolean g_check_setuid (void); GMainContext * g_main_context_new_with_next_id (guint next_id); +#if (defined (HAVE__SET_THREAD_LOCAL_INVALID_PARAMETER_HANDLER) || \ + defined (HAVE__SET_INVALID_PARAMETER_HANDLER)) && \ + defined (HAVE__CRT_SET_REPORT_MODE) +# define USE_INVALID_PARAMETER_HANDLER +#endif + #ifdef G_OS_WIN32 GLIB_AVAILABLE_IN_ALL gchar *_glib_get_locale_dir (void); diff --git a/glib/gmessages.c b/glib/gmessages.c index 3685dc1d8..528567c4f 100644 --- a/glib/gmessages.c +++ b/glib/gmessages.c @@ -2112,7 +2112,7 @@ g_log_writer_supports_color (gint output_fd) #ifdef G_OS_WIN32 gboolean result = FALSE; -#if (defined (_MSC_VER) && _MSC_VER >= 1400) +#ifdef USE_INVALID_PARAMETER_HANDLER _invalid_parameter_handler oldHandler, newHandler; int prev_report_mode = 0; #endif @@ -2143,7 +2143,7 @@ g_log_writer_supports_color (gint output_fd) */ #ifdef G_OS_WIN32 -#if (defined (_MSC_VER) && _MSC_VER >= 1400) +#ifdef USE_INVALID_PARAMETER_HANDLER /* Set up our empty invalid parameter handler, for isatty(), * in case of bad fd's passed in for isatty(), so that * msvcrt80.dll+ won't abort the program @@ -2185,7 +2185,7 @@ g_log_writer_supports_color (gint output_fd) result = win32_is_pipe_tty (output_fd); reset_invalid_param_handler: -#if defined (_MSC_VER) && (_MSC_VER >= 1400) +#ifdef USE_INVALID_PARAMETER_HANDLER _CrtSetReportMode(_CRT_ASSERT, prev_report_mode); _set_invalid_parameter_handler (oldHandler); #endif diff --git a/glib/gspawn-win32-helper.c b/glib/gspawn-win32-helper.c index 72b49baf6..9ca1cacfb 100644 --- a/glib/gspawn-win32-helper.c +++ b/glib/gspawn-win32-helper.c @@ -33,7 +33,7 @@ * Please see http://msdn.microsoft.com/zh-tw/library/ks2530z6%28v=vs.80%29.aspx * for an explanation on this. */ -#if (defined (_MSC_VER) && _MSC_VER >= 1400) +#ifdef USE_INVALID_PARAMETER_HANDLER #include #endif @@ -168,7 +168,7 @@ checked_dup2 (int oldfd, int newfd, int report_fd) return newfd; } -#if (defined (_MSC_VER) && _MSC_VER >= 1400) +#ifdef USE_INVALID_PARAMETER_HANDLER /* * This is the (empty) invalid parameter handler * that is used for Visual C++ 2005 (and later) builds @@ -221,7 +221,7 @@ main (int ignored_argc, char **ignored_argv) wchar_t **wargv; char c; -#if (defined (_MSC_VER) && _MSC_VER >= 1400) +#ifdef USE_INVALID_PARAMETER_HANDLER /* set up our empty invalid parameter handler */ _invalid_parameter_handler oldHandler, newHandler; newHandler = myInvalidParameterHandler; diff --git a/meson.build b/meson.build index 1158ef177..40842c918 100644 --- a/meson.build +++ b/meson.build @@ -2373,6 +2373,20 @@ install_data('m4macros/glib-2.0.m4', 'm4macros/glib-gettext.m4', 'm4macros/gsett install_tag : 'devel', ) +# Check whether we support overriding the invalid parameter handler on Windows for _get_osfhandle(), +# g_fsync() (i.e. _commit()), etc +if host_system == 'windows' + if cc.has_function('_set_thread_local_invalid_parameter_handler', prefix: '#include ') + glib_conf.set('HAVE__SET_THREAD_LOCAL_INVALID_PARAMETER_HANDLER', 1) + endif + if cc.has_function('_set_invalid_parameter_handler', prefix: '#include ') + glib_conf.set('HAVE__SET_INVALID_PARAMETER_HANDLER', 1) + endif + if cc.has_header_symbol('crtdbg.h', '_CrtSetReportMode') + glib_conf.set('HAVE__CRT_SET_REPORT_MODE', 1) + endif +endif + configure_file(output : 'config.h', configuration : glib_conf) if host_system == 'windows' From 08e0fef6324cc78cb745b9a7053b570ae9dbe48d Mon Sep 17 00:00:00 2001 From: Chun-wei Fan Date: Thu, 3 Nov 2022 00:00:31 +0800 Subject: [PATCH 2/5] glib-private.c: Add private API to override invalid parameter handler ...if supported, as in the previous commit. We will eventually use these private API to override the invalid parameter handler as needed in the other parts of GLib and the tests. We also now use _set_thread_local_invalid_parameter_handler() instead of just _set_invalid_parameter_handler() to be safer, if that is available. This can be expanded upon in the future if we desire to use a stricter or more customized invalid parameter handler. --- glib/glib-private.c | 78 +++++++++++++++++++++++++++++++++++++++++++++ glib/glib-private.h | 24 ++++++++++++++ 2 files changed, 102 insertions(+) diff --git a/glib/glib-private.c b/glib/glib-private.c index b78f0a22e..905da361a 100644 --- a/glib/glib-private.c +++ b/glib/glib-private.c @@ -24,6 +24,10 @@ #include "glib-private.h" #include "glib-init.h" +#ifdef USE_INVALID_PARAMETER_HANDLER +#include +#endif + /** * glib__private__: * @arg: Do not use this argument @@ -60,7 +64,81 @@ glib__private__ (void) g_win32_reopen_noninherited, g_win32_handle_is_socket, #endif + + g_win32_push_empty_invalid_parameter_handler, + g_win32_pop_invalid_parameter_handler, }; return &table; } + +#ifdef USE_INVALID_PARAMETER_HANDLER +/* + * This is the (empty) invalid parameter handler + * that is used for Visual C++ 2005 (and later) builds + * so that we can use this instead of the system automatically + * aborting the process, when calling _get_osfhandle(), isatty() + * and _commit() (via g_fsync()) and so on with an invalid file + * descriptor. + * + * This is necessary so that the gspawn helper and the test programs + * will continue to run as expected, since we are purposely or + * forced to use invalid FDs. + * + * Please see https://learn.microsoft.com/en-us/cpp/c-runtime-library/parameter-validation?view=msvc-170 + * for an explanation on this. + */ +static void +empty_invalid_parameter_handler (const wchar_t *expression, + const wchar_t *function, + const wchar_t *file, + unsigned int line, + uintptr_t pReserved) +{ +} + +/* fallback to _set_invalid_parameter_handler() if we don't have _set_thread_local_invalid_parameter_handler() */ +#ifndef HAVE__SET_THREAD_LOCAL_INVALID_PARAMETER_HANDLER +# define _set_thread_local_invalid_parameter_handler _set_invalid_parameter_handler +#endif + +#endif +/* + * g_win32_push_empty_invalid_parameter_handler: + * @handler: a possibly uninitialized GWin32InvalidParameterHandler + */ +void +g_win32_push_empty_invalid_parameter_handler (GWin32InvalidParameterHandler *handler) +{ +#ifdef USE_INVALID_PARAMETER_HANDLER + /* use the empty invalid parameter handler to override the default invalid parameter_handler */ + handler->pushed_handler = empty_invalid_parameter_handler; + handler->old_handler = _set_thread_local_invalid_parameter_handler (handler->pushed_handler); + + /* Disable the message box for assertions. */ + handler->pushed_report_mode = 0; + handler->prev_report_mode = _CrtSetReportMode(_CRT_ASSERT, handler->pushed_report_mode); +#endif +} + +/* + * g_win32_pop_invalid_parameter_handler: + * @handler: a GWin32InvalidParameterHandler processed with + * g_win32_push_empty_invalid_parameter_handler() + */ +void +g_win32_pop_invalid_parameter_handler (GWin32InvalidParameterHandler *handler) +{ +#ifdef USE_INVALID_PARAMETER_HANDLER + G_GNUC_UNUSED _invalid_parameter_handler popped_handler; + G_GNUC_UNUSED int popped_report_mode; + + /* Restore previous/default invalid parameter handler, check the value returned matches the one we previously pushed */ + popped_handler = _set_thread_local_invalid_parameter_handler (handler->old_handler); + g_return_if_fail (handler->pushed_handler == popped_handler); + + /* Restore the message box for assertions, check the value returned matches the one we previously pushed */ + popped_report_mode = _CrtSetReportMode(_CRT_ASSERT, handler->prev_report_mode); + g_return_if_fail (handler->pushed_report_mode == popped_report_mode); +#endif +} diff --git a/glib/glib-private.h b/glib/glib-private.h index f83f76e76..e3a4f1d73 100644 --- a/glib/glib-private.h +++ b/glib/glib-private.h @@ -127,6 +127,21 @@ GMainContext * g_main_context_new_with_next_id (guint next_id); # define USE_INVALID_PARAMETER_HANDLER #endif +#ifdef USE_INVALID_PARAMETER_HANDLER +struct _GWin32InvalidParameterHandler +{ + _invalid_parameter_handler old_handler; + _invalid_parameter_handler pushed_handler; + int prev_report_mode; + int pushed_report_mode; +}; +#else +struct _GWin32InvalidParameterHandler +{ + int unused_really; +}; +#endif + #ifdef G_OS_WIN32 GLIB_AVAILABLE_IN_ALL gchar *_glib_get_locale_dir (void); @@ -135,8 +150,13 @@ gchar *_glib_get_locale_dir (void); GDir * g_dir_open_with_errno (const gchar *path, guint flags); GDir * g_dir_new_from_dirp (gpointer dirp); +typedef struct _GWin32InvalidParameterHandler GWin32InvalidParameterHandler; +void g_win32_push_empty_invalid_parameter_handler (GWin32InvalidParameterHandler *items); +void g_win32_pop_invalid_parameter_handler (GWin32InvalidParameterHandler *items); + #define GLIB_PRIVATE_CALL(symbol) (glib__private__()->symbol) + typedef struct { /* See gwakeup.c */ GWakeup * (* g_wakeup_new) (void); @@ -188,6 +208,10 @@ typedef struct { #endif + /* See glib-private.c */ + void (* g_win32_push_empty_invalid_parameter_handler) (GWin32InvalidParameterHandler *items); + + void (* g_win32_pop_invalid_parameter_handler) (GWin32InvalidParameterHandler *items); /* Add other private functions here, initialize them in glib-private.c */ } GLibPrivateVTable; From 9bcc9405d7fd3ed7cf4373804646ce00133c3509 Mon Sep 17 00:00:00 2001 From: Chun-wei Fan Date: Tue, 1 Nov 2022 13:07:44 +0800 Subject: [PATCH 3/5] glib: Port to the private invalid parameter handler APIs Use the newly-added private APIs added in the previous commit so that we can clean up the code a bit. --- glib/gmessages.c | 41 +++--------------------------- glib/gspawn-win32-helper.c | 51 +++----------------------------------- 2 files changed, 7 insertions(+), 85 deletions(-) diff --git a/glib/gmessages.c b/glib/gmessages.c index 528567c4f..f73cd3f06 100644 --- a/glib/gmessages.c +++ b/glib/gmessages.c @@ -192,6 +192,7 @@ #include "gcharset.h" #include "gconvert.h" #include "genviron.h" +#include "glib-private.h" #include "gmain.h" #include "gmem.h" #include "gprintfint.h" @@ -219,22 +220,6 @@ #define ENABLE_VIRTUAL_TERMINAL_PROCESSING 0x0004 #endif -#if defined (_MSC_VER) && (_MSC_VER >=1400) -/* This is ugly, but we need it for isatty() in case we have bad fd's, - * otherwise Windows will abort() the program on msvcrt80.dll and later - */ -#include - -_GLIB_EXTERN void -myInvalidParameterHandler(const wchar_t *expression, - const wchar_t *function, - const wchar_t *file, - unsigned int line, - uintptr_t pReserved) -{ -} -#endif - #include "gwin32.h" #endif @@ -2111,12 +2096,7 @@ g_log_writer_supports_color (gint output_fd) { #ifdef G_OS_WIN32 gboolean result = FALSE; - -#ifdef USE_INVALID_PARAMETER_HANDLER - _invalid_parameter_handler oldHandler, newHandler; - int prev_report_mode = 0; -#endif - + GWin32InvalidParameterHandler handler; #endif g_return_val_if_fail (output_fd >= 0, FALSE); @@ -2143,17 +2123,7 @@ g_log_writer_supports_color (gint output_fd) */ #ifdef G_OS_WIN32 -#ifdef USE_INVALID_PARAMETER_HANDLER - /* Set up our empty invalid parameter handler, for isatty(), - * in case of bad fd's passed in for isatty(), so that - * msvcrt80.dll+ won't abort the program - */ - newHandler = myInvalidParameterHandler; - oldHandler = _set_invalid_parameter_handler (newHandler); - - /* Disable the message box for assertions. */ - prev_report_mode = _CrtSetReportMode(_CRT_ASSERT, 0); -#endif + g_win32_push_empty_invalid_parameter_handler (&handler); if (g_win32_check_windows_version (10, 0, 0, G_WIN32_OS_ANY)) { @@ -2185,10 +2155,7 @@ g_log_writer_supports_color (gint output_fd) result = win32_is_pipe_tty (output_fd); reset_invalid_param_handler: -#ifdef USE_INVALID_PARAMETER_HANDLER - _CrtSetReportMode(_CRT_ASSERT, prev_report_mode); - _set_invalid_parameter_handler (oldHandler); -#endif + g_win32_pop_invalid_parameter_handler (&handler); return result; #else diff --git a/glib/gspawn-win32-helper.c b/glib/gspawn-win32-helper.c index 9ca1cacfb..35b25905c 100644 --- a/glib/gspawn-win32-helper.c +++ b/glib/gspawn-win32-helper.c @@ -23,20 +23,6 @@ #include -/* For _CrtSetReportMode, we don't want Windows CRT (2005 and later) - * to terminate the process if a bad file descriptor is passed into - * _get_osfhandle(). This is necessary because we use _get_osfhandle() - * to check the validity of the fd before we try to call close() on - * it as attempting to close an invalid fd will cause the Windows CRT - * to abort() this program internally. - * - * Please see http://msdn.microsoft.com/zh-tw/library/ks2530z6%28v=vs.80%29.aspx - * for an explanation on this. - */ -#ifdef USE_INVALID_PARAMETER_HANDLER -#include -#endif - #undef G_LOG_DOMAIN #include "glib.h" #define GSPAWN_HELPER @@ -168,30 +154,6 @@ checked_dup2 (int oldfd, int newfd, int report_fd) return newfd; } -#ifdef USE_INVALID_PARAMETER_HANDLER -/* - * This is the (empty) invalid parameter handler - * that is used for Visual C++ 2005 (and later) builds - * so that we can use this instead of the system automatically - * aborting the process. - * - * This is necessary as we use _get_oshandle() to check the validity - * of the file descriptors as we close them, so when an invalid file - * descriptor is passed into that function as we check on it, we get - * -1 as the result, instead of the gspawn helper program aborting. - * - * Please see http://msdn.microsoft.com/zh-tw/library/ks2530z6%28v=vs.80%29.aspx - * for an explanation on this. - */ -extern void -myInvalidParameterHandler(const wchar_t *expression, - const wchar_t *function, - const wchar_t *file, - unsigned int line, - uintptr_t pReserved); -#endif - - #ifndef HELPER_CONSOLE int _stdcall WinMain (struct HINSTANCE__ *hInstance, @@ -220,16 +182,7 @@ main (int ignored_argc, char **ignored_argv) char **argv; wchar_t **wargv; char c; - -#ifdef USE_INVALID_PARAMETER_HANDLER - /* set up our empty invalid parameter handler */ - _invalid_parameter_handler oldHandler, newHandler; - newHandler = myInvalidParameterHandler; - oldHandler = _set_invalid_parameter_handler(newHandler); - - /* Disable the message box for assertions. */ - _CrtSetReportMode(_CRT_ASSERT, 0); -#endif + GWin32InvalidParameterHandler handler; /* Fetch the wide-char argument vector */ wargv = CommandLineToArgvW (GetCommandLineW(), &argc); @@ -398,11 +351,13 @@ main (int ignored_argc, char **ignored_argv) /* argv[ARG_CLOSE_DESCRIPTORS] is "y" if file descriptors from 3 * upwards should be closed */ + GLIB_PRIVATE_CALL (g_win32_push_empty_invalid_parameter_handler) (&handler); if (argv[ARG_CLOSE_DESCRIPTORS][0] == 'y') for (i = 3; i < 1000; i++) /* FIXME real limit? */ if (!g_hash_table_contains (fds, GINT_TO_POINTER (i))) if (_get_osfhandle (i) != -1) close (i); + GLIB_PRIVATE_CALL (g_win32_pop_invalid_parameter_handler) (&handler); /* We don't want our child to inherit the error report and * helper sync fds. From 4f426c56d08aa1b206a77294e656aa1bf0cc0ebe Mon Sep 17 00:00:00 2001 From: Chun-wei Fan Date: Tue, 1 Nov 2022 13:14:40 +0800 Subject: [PATCH 4/5] fileutils.c: Fix the clearfd test on Windows ...when the test program aborts while checking the FD's were indeed closed, since we need to override the invalid parameter handler to do such checks, if the CRT demands so, so that the test program will proceed normally. This will fix issue #2800. --- glib/tests/fileutils.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/glib/tests/fileutils.c b/glib/tests/fileutils.c index 52f6f36d8..eba8a8b5a 100644 --- a/glib/tests/fileutils.c +++ b/glib/tests/fileutils.c @@ -35,6 +35,7 @@ /* Test our stdio wrappers here; this disables redefining (e.g.) g_open() to open() */ #define G_STDIO_WRAP_ON_UNIX #include +#include "glib-private.h" #ifdef G_OS_UNIX #include @@ -2462,8 +2463,13 @@ assert_fd_was_closed (int fd) * was still valid */ if (g_test_undefined ()) { - int result = g_fsync (fd); - int errsv = errno; + int result, errsv; + GWin32InvalidParameterHandler handler; + + GLIB_PRIVATE_CALL (g_win32_push_empty_invalid_parameter_handler) (&handler); + result = g_fsync (fd); + errsv = errno; + GLIB_PRIVATE_CALL (g_win32_pop_invalid_parameter_handler) (&handler); g_assert_cmpint (result, !=, 0); g_assert_cmpint (errsv, ==, EBADF); From 41d8ed37cdd825f89f5f5aab690b14a431ea510e Mon Sep 17 00:00:00 2001 From: Chun-wei Fan Date: Tue, 1 Nov 2022 13:59:36 +0800 Subject: [PATCH 5/5] glib/tests/meson.build: Expect fileutils to pass on Windows The underlying issue was fixed, so expect that to pass. --- glib/tests/meson.build | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/glib/tests/meson.build b/glib/tests/meson.build index 1f1275d80..bbb68f984 100644 --- a/glib/tests/meson.build +++ b/glib/tests/meson.build @@ -26,10 +26,7 @@ glib_tests = { 'can_fail' : host_system == 'darwin', }, 'error' : {}, - 'fileutils' : { - # FIXME: https://gitlab.gnome.org/GNOME/glib/-/issues/2800 - 'can_fail' : host_system == 'windows' and cc.get_id() == 'msvc', - }, + 'fileutils' : {}, 'gdatetime' : { 'suite' : ['slow'], 'can_fail' : host_system == 'windows',