diff mbox series

[3/4] libbacktrace: work with aslr on windows

Message ID 20230120105409.54949-3-gcc@hazardy.de
State Accepted
Headers
Series [1/4] libbacktrace: change all pc related variables to uintptr_t |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Björn Schäpers Jan. 20, 2023, 10:54 a.m. UTC 
  From: Björn Schäpers <bjoern@hazardy.de>

Any underflow which might happen, will be countered by an overflow in
dwarf.c.

Tested on x86_64-linux and i686-w64-mingw32.

-- >8 --

Fixes https://github.com/ianlancetaylor/libbacktrace/issues/89 and
https://github.com/ianlancetaylor/libbacktrace/issues/82.

	* pecoff.c (coff_add): Set the base_address of the module, to
	find the debug information on moved applications.

Signed-off-by: Björn Schäpers <bjoern@hazardy.de>
---
 libbacktrace/pecoff.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Comments

Eli Zaretskii Jan. 20, 2023, 1:39 p.m. UTC | #1 
> From: Björn Schäpers <gcc@hazardy.de>
> Date: Fri, 20 Jan 2023 11:54:08 +0100
> 
> @@ -856,7 +870,12 @@ coff_add (struct backtrace_state *state, int descriptor,
>  				  + (sections[i].offset - min_offset));
>      }
>  
> -  if (!backtrace_dwarf_add (state, /* base_address */ 0, &dwarf_sections,
> +#ifdef HAVE_WINDOWS_H
> +    module_handle = (uintptr_t) GetModuleHandleW (NULL);
> +    base_address = module_handle - image_base;
> +#endif
> +
> +  if (!backtrace_dwarf_add (state, base_address, &dwarf_sections,
>  			    0, /* FIXME: is_bigendian */
>  			    NULL, /* altlink */
>  			    error_callback, data, fileline_fn,

Why do you force using the "wide" APIs here?  Won't GetModuleHandle do
the job, whether it resolves to GetModuleHandleA or GetModuleHandleW?
Gabriel Ravier Jan. 20, 2023, 4:46 p.m. UTC | #2 
On 1/20/23 14:39, Eli Zaretskii via Gcc wrote:
>> From: Björn Schäpers <gcc@hazardy.de>
>> Date: Fri, 20 Jan 2023 11:54:08 +0100
>>
>> @@ -856,7 +870,12 @@ coff_add (struct backtrace_state *state, int descriptor,
>>   				  + (sections[i].offset - min_offset));
>>       }
>>   
>> -  if (!backtrace_dwarf_add (state, /* base_address */ 0, &dwarf_sections,
>> +#ifdef HAVE_WINDOWS_H
>> +    module_handle = (uintptr_t) GetModuleHandleW (NULL);
>> +    base_address = module_handle - image_base;
>> +#endif
>> +
>> +  if (!backtrace_dwarf_add (state, base_address, &dwarf_sections,
>>   			    0, /* FIXME: is_bigendian */
>>   			    NULL, /* altlink */
>>   			    error_callback, data, fileline_fn,
> Why do you force using the "wide" APIs here?  Won't GetModuleHandle do
> the job, whether it resolves to GetModuleHandleA or GetModuleHandleW?

I would expect the reason to be either that:

- using wide APIs with Windows is generally considered to be a best 
practice, even when not strictly needed (and in this case I can't see 
any problem with doing so, unless maybe we want to code to work with 
Windows 95 or something like that...)

- using the narrow API somehow has an actual drawback, for example maybe 
it might not work if the name of the exe file the NULL will tell it to 
get a handle to contains wide characters
Gabriel Ravier Jan. 20, 2023, 8:39 p.m. UTC | #3 
On 1/20/23 20:19, Eli Zaretskii wrote:
>> Date: Fri, 20 Jan 2023 17:46:59 +0100
>> Cc: gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
>> From: Gabriel Ravier <gabravier@gmail.com>
>>
>> On 1/20/23 14:39, Eli Zaretskii via Gcc wrote:
>>>> From: Björn Schäpers <gcc@hazardy.de>
>>>> Date: Fri, 20 Jan 2023 11:54:08 +0100
>>>>
>>>> @@ -856,7 +870,12 @@ coff_add (struct backtrace_state *state, int descriptor,
>>>>    				  + (sections[i].offset - min_offset));
>>>>        }
>>>>    
>>>> -  if (!backtrace_dwarf_add (state, /* base_address */ 0, &dwarf_sections,
>>>> +#ifdef HAVE_WINDOWS_H
>>>> +    module_handle = (uintptr_t) GetModuleHandleW (NULL);
>>>> +    base_address = module_handle - image_base;
>>>> +#endif
>>>> +
>>>> +  if (!backtrace_dwarf_add (state, base_address, &dwarf_sections,
>>>>    			    0, /* FIXME: is_bigendian */
>>>>    			    NULL, /* altlink */
>>>>    			    error_callback, data, fileline_fn,
>>> Why do you force using the "wide" APIs here?  Won't GetModuleHandle do
>>> the job, whether it resolves to GetModuleHandleA or GetModuleHandleW?
>> I would expect the reason to be either that:
>>
>> - using wide APIs with Windows is generally considered to be a best
>> practice, even when not strictly needed (and in this case I can't see
>> any problem with doing so, unless maybe we want to code to work with
>> Windows 95 or something like that...)
> There's no reason to forcibly break GDB on platforms where wide APIs
> are not available.
Are there even any platforms that have GetModuleHandleA but not 
GetModuleHandleW ? MSDN states that Windows XP and Windows Server 2003 
are the first versions to support both of the APIs, so if this is 
supposed to work on Windows 98, for instance, whether we're using 
GetModuleHandleA or GetModuleHandleW won't matter.
>
>> - using the narrow API somehow has an actual drawback, for example maybe
>> it might not work if the name of the exe file the NULL will tell it to
>> get a handle to contains wide characters
> Native Windows port of GDB doesn't support Unicode file names anyway,
> which is why you used the *A APIs elsewhere in the patch, and
> rightfully so.  So there's no reason to use "wide" APIs in this one
> place, and every reason not to.

(just as a clarification, I did not write this patch)
Eli Zaretskii Jan. 21, 2023, 4:05 a.m. UTC | #4 
> Date: Fri, 20 Jan 2023 21:39:56 +0100
> Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> From: Gabriel Ravier <gabravier@gmail.com>
> 
> >> - using wide APIs with Windows is generally considered to be a best
> >> practice, even when not strictly needed (and in this case I can't see
> >> any problem with doing so, unless maybe we want to code to work with
> >> Windows 95 or something like that...)
> > There's no reason to forcibly break GDB on platforms where wide APIs
> > are not available.
> Are there even any platforms that have GetModuleHandleA but not 
> GetModuleHandleW ? MSDN states that Windows XP and Windows Server 2003 
> are the first versions to support both of the APIs, so if this is 
> supposed to work on Windows 98, for instance, whether we're using 
> GetModuleHandleA or GetModuleHandleW won't matter.

I'm not sure I follow the logic.  A program that calls
GetModuleHandleW will refuse to start on Windows that doesn't have
that API.  So any version before XP is automatically excluded the
moment you use code which calls that API directly (i.e. not through a
function pointer or somesuch).
LIU Hao Jan. 21, 2023, 9:18 a.m. UTC | #5 
在 2023-01-21 12:05, Eli Zaretskii via Gcc 写道:
> I'm not sure I follow the logic.  A program that calls
> GetModuleHandleW will refuse to start on Windows that doesn't have
> that API.  So any version before XP is automatically excluded the
> moment you use code which calls that API directly (i.e. not through a
> function pointer or somesuch).

Are _you_ still willing to maintain backward compatibility with Windows 9x? Even mingw-w64 has been 
defaulting to Windows Server 2003 since 2007. Why would anyone build a modern compiler for such old 
operating systems?

With any Windows that is modern enough, wide APIs should always be preferred to ANSI ones, 
especially when the argument is constant. Almost all ANSI APIs (the only exception I know of is 
`OutputDebugStringA` which does the inverse) translate their ANSI string arguments to wide strings 
and delegate to wide ones, so by calling wide APIs explicitly, such overhead can be avoided.


-- 
Best regards,
LIU Hao
Eli Zaretskii Jan. 21, 2023, 9:25 a.m. UTC | #6 
> Date: Sat, 21 Jan 2023 17:18:14 +0800
> Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> From: LIU Hao <lh_mouse@126.com>
> 
> 在 2023-01-21 12:05, Eli Zaretskii via Gcc 写道:
> > I'm not sure I follow the logic.  A program that calls
> > GetModuleHandleW will refuse to start on Windows that doesn't have
> > that API.  So any version before XP is automatically excluded the
> > moment you use code which calls that API directly (i.e. not through a
> > function pointer or somesuch).
> 
> Are _you_ still willing to maintain backward compatibility with Windows 9x? Even mingw-w64 has been 
> defaulting to Windows Server 2003 since 2007. Why would anyone build a modern compiler for such old 
> operating systems?

I'm only saying that we should not deliberately break those old
platforms unless we have a good reason.  And I see no such good reason
in this case: GetModuleHandleA will do the job exactly like
GetModuleHandleW will.

> With any Windows that is modern enough, wide APIs should always be preferred to ANSI ones, 
> especially when the argument is constant. Almost all ANSI APIs (the only exception I know of is 
> `OutputDebugStringA` which does the inverse) translate their ANSI string arguments to wide strings 
> and delegate to wide ones, so by calling wide APIs explicitly, such overhead can be avoided.

The overhead is only relevant in code that is run in performance
critical places.  I don't think this is such a place.
Gabriel Ravier Jan. 21, 2023, 10:47 a.m. UTC | #7 
On 1/21/23 05:05, Eli Zaretskii wrote:
>> Date: Fri, 20 Jan 2023 21:39:56 +0100
>> Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
>> From: Gabriel Ravier <gabravier@gmail.com>
>>
>>>> - using wide APIs with Windows is generally considered to be a best
>>>> practice, even when not strictly needed (and in this case I can't see
>>>> any problem with doing so, unless maybe we want to code to work with
>>>> Windows 95 or something like that...)
>>> There's no reason to forcibly break GDB on platforms where wide APIs
>>> are not available.
>> Are there even any platforms that have GetModuleHandleA but not
>> GetModuleHandleW ? MSDN states that Windows XP and Windows Server 2003
>> are the first versions to support both of the APIs, so if this is
>> supposed to work on Windows 98, for instance, whether we're using
>> GetModuleHandleA or GetModuleHandleW won't matter.
> I'm not sure I follow the logic.  A program that calls
> GetModuleHandleW will refuse to start on Windows that doesn't have
> that API.  So any version before XP is automatically excluded the
> moment you use code which calls that API directly (i.e. not through a
> function pointer or somesuch).
A program that calls GetModuleHandleA will also refuse to start on 
Windows if it doesn't have that API. The set of Windows versions that do 
not have GetModuleHandleA is, according to MSDN, the same as the set of 
Windows versions that do not have GetModuleHandleW.
Eli Zaretskii Jan. 21, 2023, 11:42 a.m. UTC | #8 
> Date: Sat, 21 Jan 2023 11:47:42 +0100
> Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> From: Gabriel Ravier <gabravier@gmail.com>
> 
> 
> On 1/21/23 05:05, Eli Zaretskii wrote:
> >> Date: Fri, 20 Jan 2023 21:39:56 +0100
> >> Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> >> From: Gabriel Ravier <gabravier@gmail.com>
> >>
> >>>> - using wide APIs with Windows is generally considered to be a best
> >>>> practice, even when not strictly needed (and in this case I can't see
> >>>> any problem with doing so, unless maybe we want to code to work with
> >>>> Windows 95 or something like that...)
> >>> There's no reason to forcibly break GDB on platforms where wide APIs
> >>> are not available.
> >> Are there even any platforms that have GetModuleHandleA but not
> >> GetModuleHandleW ? MSDN states that Windows XP and Windows Server 2003
> >> are the first versions to support both of the APIs, so if this is
> >> supposed to work on Windows 98, for instance, whether we're using
> >> GetModuleHandleA or GetModuleHandleW won't matter.
> > I'm not sure I follow the logic.  A program that calls
> > GetModuleHandleW will refuse to start on Windows that doesn't have
> > that API.  So any version before XP is automatically excluded the
> > moment you use code which calls that API directly (i.e. not through a
> > function pointer or somesuch).
> A program that calls GetModuleHandleA will also refuse to start on 
> Windows if it doesn't have that API. The set of Windows versions that do 
> not have GetModuleHandleA is, according to MSDN, the same as the set of 
> Windows versions that do not have GetModuleHandleW.

MSDN lies (because it wants to pretend that older versions don't
exist).  Try this much more useful site:

  http://winapi.freetechsecrets.com/win32/WIN32GetModuleHandle.htm
Björn Schäpers Nov. 20, 2023, 7:57 p.m. UTC | #9 
An updated version, using neither A or W, but just the macro.

Am 21.01.2023 um 12:42 schrieb Eli Zaretskii:
>> Date: Sat, 21 Jan 2023 11:47:42 +0100
>> Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
>> From: Gabriel Ravier <gabravier@gmail.com>
>>
>>
>> On 1/21/23 05:05, Eli Zaretskii wrote:
>>>> Date: Fri, 20 Jan 2023 21:39:56 +0100
>>>> Cc: gcc@hazardy.de, gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
>>>> From: Gabriel Ravier <gabravier@gmail.com>
>>>>
>>>>>> - using wide APIs with Windows is generally considered to be a best
>>>>>> practice, even when not strictly needed (and in this case I can't see
>>>>>> any problem with doing so, unless maybe we want to code to work with
>>>>>> Windows 95 or something like that...)
>>>>> There's no reason to forcibly break GDB on platforms where wide APIs
>>>>> are not available.
>>>> Are there even any platforms that have GetModuleHandleA but not
>>>> GetModuleHandleW ? MSDN states that Windows XP and Windows Server 2003
>>>> are the first versions to support both of the APIs, so if this is
>>>> supposed to work on Windows 98, for instance, whether we're using
>>>> GetModuleHandleA or GetModuleHandleW won't matter.
>>> I'm not sure I follow the logic.  A program that calls
>>> GetModuleHandleW will refuse to start on Windows that doesn't have
>>> that API.  So any version before XP is automatically excluded the
>>> moment you use code which calls that API directly (i.e. not through a
>>> function pointer or somesuch).
>> A program that calls GetModuleHandleA will also refuse to start on
>> Windows if it doesn't have that API. The set of Windows versions that do
>> not have GetModuleHandleA is, according to MSDN, the same as the set of
>> Windows versions that do not have GetModuleHandleW.
> 
> MSDN lies (because it wants to pretend that older versions don't
> exist).  Try this much more useful site:
> 
>    http://winapi.freetechsecrets.com/win32/WIN32GetModuleHandle.htm
From 52cbe06b1c165172191f66ff7e55a49adecf661d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Sch=C3=A4pers?= <bjoern@hazardy.de>
Date: Sun, 30 Apr 2023 23:52:37 +0200
Subject: [PATCH 2/3] libbacktrace: work with aslr on windows
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Any underflow which might happen, will be countered by an overflow in
dwarf.c.

Tested on x86_64-linux and i686-w64-mingw32.

-- >8 --

Fixes https://github.com/ianlancetaylor/libbacktrace/issues/89 and
https://github.com/ianlancetaylor/libbacktrace/issues/82.

	* pecoff.c (coff_add): Set the base_address of the module, to
	find the debug information on moved applications.

Signed-off-by: Björn Schäpers <bjoern@hazardy.de>
---
 libbacktrace/pecoff.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/libbacktrace/pecoff.c b/libbacktrace/pecoff.c
index 56af4828e27..9cc13b47947 100644
--- a/libbacktrace/pecoff.c
+++ b/libbacktrace/pecoff.c
@@ -39,6 +39,18 @@ POSSIBILITY OF SUCH DAMAGE.  */
 #include "backtrace.h"
 #include "internal.h"
 
+#ifdef HAVE_WINDOWS_H
+#ifndef WIN32_MEAN_AND_LEAN
+#define WIN32_MEAN_AND_LEAN
+#endif
+
+#ifndef NOMINMAX
+#define NOMINMAX
+#endif
+
+#include <windows.h>
+#endif
+
 /* Coff file header.  */
 
 typedef struct {
@@ -610,6 +622,8 @@ coff_add (struct backtrace_state *state, int descriptor,
   int debug_view_valid;
   int is_64;
   uintptr_t image_base;
+  uintptr_t base_address = 0;
+  uintptr_t module_handle;
   struct dwarf_sections dwarf_sections;
 
   *found_sym = 0;
@@ -856,7 +870,12 @@ coff_add (struct backtrace_state *state, int descriptor,
 				  + (sections[i].offset - min_offset));
     }
 
-  if (!backtrace_dwarf_add (state, /* base_address */ 0, &dwarf_sections,
+#ifdef HAVE_WINDOWS_H
+    module_handle = (uintptr_t) GetModuleHandle (NULL);
+    base_address = module_handle - image_base;
+#endif
+
+  if (!backtrace_dwarf_add (state, base_address, &dwarf_sections,
 			    0, /* FIXME: is_bigendian */
 			    NULL, /* altlink */
 			    error_callback, data, fileline_fn,
Eli Zaretskii Nov. 20, 2023, 8:07 p.m. UTC | #10 
> Date: Mon, 20 Nov 2023 20:57:38 +0100
> Cc: gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> From: Björn Schäpers <gcc@hazardy.de>
> 
> +#ifndef NOMINMAX
> +#define NOMINMAX
> +#endif

Why is this part needed?

Otherwise, LGTM, thanks.  (But I'm don't have the approval rights, so
please wait for Ian to chime in.)
Björn Schäpers Nov. 21, 2023, 7:35 p.m. UTC | #11 
I'll guess it is not needed here, but otherwise <windows.h> defines the macros 
max and min, they then conflict e.g. with C++'s std::max/std::min. So I consider 
it best practice to always define it, before including <windows.h>.

Am 20.11.2023 um 21:07 schrieb Eli Zaretskii:
>> Date: Mon, 20 Nov 2023 20:57:38 +0100
>> Cc: gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
>> From: Björn Schäpers <gcc@hazardy.de>
>>
>> +#ifndef NOMINMAX
>> +#define NOMINMAX
>> +#endif
> 
> Why is this part needed?
> 
> Otherwise, LGTM, thanks.  (But I'm don't have the approval rights, so
> please wait for Ian to chime in.)
LIU Hao Nov. 22, 2023, 1:13 a.m. UTC | #12 
在 2023/11/22 03:35, Björn Schäpers 写道:
> I'll guess it is not needed here, but otherwise <windows.h> defines the macros max and min, they 
> then conflict e.g. with C++'s std::max/std::min. So I consider it best practice to always define it, 
> before including <windows.h>.

The mingw-w64 header does not define them for C++ [1] and GCC defines `NOMINMAX` in 'os_defines.h', 
so either way it is not necessary.


[1] 
https://github.com/mingw-w64/mingw-w64/blob/3ebb92804e3125d1be8f61bcd42f15a8db15ba1e/mingw-w64-headers/include/minmax.h#L9



-- 
Best regards,
LIU Hao
Ian Lance Taylor Nov. 30, 2023, 7:25 p.m. UTC | #13 
On Mon, Nov 20, 2023 at 11:58 AM Björn Schäpers <gcc@hazardy.de> wrote:
>
> An updated version, using neither A or W, but just the macro.

Thanks.  Committed as follows.

Ian
1017495bc91d40570f58c37e88ca013164782129
diff --git a/libbacktrace/pecoff.c b/libbacktrace/pecoff.c
index 56af4828e27..f976a963bf3 100644
--- a/libbacktrace/pecoff.c
+++ b/libbacktrace/pecoff.c
@@ -39,6 +39,18 @@ POSSIBILITY OF SUCH DAMAGE.  */
 #include "backtrace.h"
 #include "internal.h"
 
+#ifdef HAVE_WINDOWS_H
+#ifndef WIN32_MEAN_AND_LEAN
+#define WIN32_MEAN_AND_LEAN
+#endif
+
+#ifndef NOMINMAX
+#define NOMINMAX
+#endif
+
+#include <windows.h>
+#endif
+
 /* Coff file header.  */
 
 typedef struct {
@@ -610,6 +622,7 @@ coff_add (struct backtrace_state *state, int descriptor,
   int debug_view_valid;
   int is_64;
   uintptr_t image_base;
+  uintptr_t base_address = 0;
   struct dwarf_sections dwarf_sections;
 
   *found_sym = 0;
@@ -856,7 +869,16 @@ coff_add (struct backtrace_state *state, int descriptor,
 				  + (sections[i].offset - min_offset));
     }
 
-  if (!backtrace_dwarf_add (state, /* base_address */ 0, &dwarf_sections,
+#ifdef HAVE_WINDOWS_H
+  {
+    uintptr_t module_handle;
+
+    module_handle = (uintptr_t) GetModuleHandle (NULL);
+    base_address = module_handle - image_base;
+  }
+#endif
+
+  if (!backtrace_dwarf_add (state, base_address, &dwarf_sections,
 			    0, /* FIXME: is_bigendian */
 			    NULL, /* altlink */
 			    error_callback, data, fileline_fn,
diff mbox series

Patch

diff --git a/libbacktrace/pecoff.c b/libbacktrace/pecoff.c
index 87b3c0cc647..296f1357b5f 100644
--- a/libbacktrace/pecoff.c
+++ b/libbacktrace/pecoff.c
@@ -39,6 +39,18 @@  POSSIBILITY OF SUCH DAMAGE.  */
 #include "backtrace.h"
 #include "internal.h"
 
+#ifdef HAVE_WINDOWS_H
+#ifndef WIN32_MEAN_AND_LEAN
+#define WIN32_MEAN_AND_LEAN
+#endif
+
+#ifndef NOMINMAX
+#define NOMINMAX
+#endif
+
+#include <windows.h>
+#endif
+
 /* Coff file header.  */
 
 typedef struct {
@@ -610,6 +622,8 @@  coff_add (struct backtrace_state *state, int descriptor,
   int debug_view_valid;
   int is_64;
   uintptr_t image_base;
+  uintptr_t base_address = 0;
+  uintptr_t module_handle;
   struct dwarf_sections dwarf_sections;
 
   *found_sym = 0;
@@ -856,7 +870,12 @@  coff_add (struct backtrace_state *state, int descriptor,
 				  + (sections[i].offset - min_offset));
     }
 
-  if (!backtrace_dwarf_add (state, /* base_address */ 0, &dwarf_sections,
+#ifdef HAVE_WINDOWS_H
+    module_handle = (uintptr_t) GetModuleHandleW (NULL);
+    base_address = module_handle - image_base;
+#endif
+
+  if (!backtrace_dwarf_add (state, base_address, &dwarf_sections,
 			    0, /* FIXME: is_bigendian */
 			    NULL, /* altlink */
 			    error_callback, data, fileline_fn,