[PATCH 2/2] Allow ASLR to be disabled on Windows

[Eli Zaretskii]

> Date: Wed,  6 Oct 2021 14:47:04 -0600
> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>
> Cc: Tom Tromey <tromey@adacore.com>
> 
> Note that, in order to see the necessary declarations for this code to
> work, the patch bumps _WIN32_WINNT from 0x0501 to 0x0602 -- that is,
> Windows 8.  I don't know whether this may is a problem for anyone.

It is for me.

Please, let's not unsupport old Windows versions just because some
optional feature needs a new API.  Instead, we should test for the
availability of that API at run time, and disable the feature if the
API is unavailable.  We already do that with several APIs, see
initialize_loadable in nat/windows-nat.c.  Why not do the same with
the APIs you need to support this feature?  Using that technique will
makes GDB able to run on older versions of Windows if the user doesn't
need to disable ASLR.

For example, I use GDB a lot on Windows XP, where ASLR doesn't even
exist, and therefore the problem you want to solve doesn't exist.  But
the changes you propose will prevent GDB from even starting on XP or
any other systems older than Windows 8.

> +#include <winbase.h>
> +#include <processthreadsapi.h>

I don't think this is the right way of including Windows headers.
Doesn't it work to just include <windows.h> (which nat/windows-nat.h
already does)?  The MS documentation seems to say it should be enough.

If including windows.h alone doesn't work, then we should have a
configure-time test for processthreadapi.h, instead of including it
unconditionally, as that header might not exist (e.g., my MinGW
doesn't have it).  Moreover, some prototypes and macros that are only
available since Windows 8 should be defined by hand in our sources, if
they are not already defined, because using this:

> -/* We don't support Windows versions before XP, so we define
> +/* We don't support Windows versions before Windows 8, so we define
>     _WIN32_WINNT correspondingly to ensure the Windows API headers
>     expose the required symbols.  */
>  #if defined (__MINGW32__) || defined (__CYGWIN__)
>  # ifdef _WIN32_WINNT
> -#  if _WIN32_WINNT < 0x0501
> +#  if _WIN32_WINNT < 0x0602
>  #   undef _WIN32_WINNT
> -#   define _WIN32_WINNT 0x0501
> +#   define _WIN32_WINNT 0x0602
>  #  endif
>  # else
> -#  define _WIN32_WINNT 0x0501
> +#  define _WIN32_WINNT 0x0602
>  # endif
>  #endif	/* __MINGW32__ || __CYGWIN__ */

will not help when building on older systems, and unnecessarily limits
the GDB support to Windows 8 and later systems.

> +	  if (startup_info != nullptr)
> +	    info_ex.StartupInfo = *startup_info;
> +	  info_ex.StartupInfo.cb = sizeof (info_ex);
> +	  SIZE_T size = 0;
> +	  /* Ignore the result here.  The documentation says the first
> +	     call always fails, by design.  */
> +	  InitializeProcThreadAttributeList (nullptr, 1, 0, &size);
> +	  info_ex.lpAttributeList
> +	    = (PPROC_THREAD_ATTRIBUTE_LIST) alloca (size);
> +	  InitializeProcThreadAttributeList (info_ex.lpAttributeList,
> +					     1, 0, &size);
> +
> +	  gdb::optional<BOOL> return_value;
> +	  DWORD attr_flags =
> +	    (PROCESS_CREATION_MITIGATION_POLICY_FORCE_RELOCATE_IMAGES_ALWAYS_OFF
> +	     | PROCESS_CREATION_MITIGATION_POLICY_BOTTOM_UP_ASLR_ALWAYS_OFF);
> +	  if (!UpdateProcThreadAttribute (info_ex.lpAttributeList, 0,
> +					  PROC_THREAD_ATTRIBUTE_MITIGATION_POLICY,
> +					  &attr_flags,
> +					  sizeof (attr_flags),
> +					  nullptr, nullptr))
> +	    tried_and_failed = true;

As mentioned above, these new APIs should be loaded dynamically and
called through a function pointer.  And the requisite preprocessor
macros should be defined if they aren't defined by the headers.  If
the new APIs are not available, the wrapper should simply call
CreateProcess without doing anything else.

> +  bool supports_disable_randomization () override
> +  {
> +    return true;
> +  }

This cannot be unconditional, it should depend on whether the requisite
APIs could be dynamically loaded at run time.

> --- a/gdbserver/win32-low.h
> +++ b/gdbserver/win32-low.h
> @@ -158,6 +158,11 @@ class win32_process_target : public process_stratum_target
>    bool stopped_by_sw_breakpoint () override;
>  
>    bool supports_stopped_by_sw_breakpoint () override;
> +
> +  bool supports_disable_randomization () override
> +  {
> +    return true;
> +  }

Same here.

Thanks.