This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers
> Thanks for writing up the commit log and ChangeLog. The commit looks
> fine to me, but you should add your name there somewhere too.
I ended up putting my name as the tester. That way, if someone sees
the revision log and wonders what kind of testing was done, he can
contact me. The rest was entirely you, so left you as the sole author.
> One comment on a comment below.
>
> > diff --git a/gdb/arch/amd64.c b/gdb/arch/amd64.c
> > index d31d8f1..b562018 100644
> > --- a/gdb/arch/amd64.c
> > +++ b/gdb/arch/amd64.c
> > @@ -33,10 +33,12 @@
> >
> > /* Create amd64 target descriptions according to XCR0. If IS_X32 is
> > true, create the x32 ones. If IS_LINUX is true, create target
> > - descriptions for Linux. */
> > + descriptions for Linux. If SEGMENTS is true, the include the segment
> > + registers. */
>
> "the include" -> "then include"
>
> I think it'd be clearer to say:
>
> include the "org.gnu.gdb.i386.segments" feature registers.
>
> because "segment registers" along makes one think of cs,ds,ss...,
> but those are part of the core register set.
Makes sense. I made the change you suggested, and corrected the typo
as well, and then pushed the attached patch to master.
gdb/ChangeLog:
* gdb/amd64-tdep.h (amd64_create_target_description): Add
"segments" parameter.
* gdb/amd64-tdep.c (amd64_none_init_abi, amd64_x32_none_init_abi)
(_initialize_amd64_tdep): Update call to
amd64_create_target_description.
(amd64_target_description): Add "segments" parameter. Adjust
the implementation to use it.
* gdb/amd64-linux-tdep.c (amd64_linux_read_description): Update
call to amd64_create_target_description.
* gdb/amd64-windows-tdep.c (amd64_windows_init_abi): Likewise.
* gdb/arch/amd64.h (amd64_create_target_description): Add
"segments" register.
* gdb/arch/amd64.c (amd64_create_target_description): Add
"segments" parameter. Call create_feature_i386_64bit_segments
only if SEGMENTS is true.
* gdb/gdbserver/win32-i386-low.c (i386_arch_setup): Update
call to amd64_create_target_description.
Thanks a lot for your help!
--
Joel
>From de52b9607d2623f18b7a7dbee3e1123d8d63f5da Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 26 Jun 2018 16:33:27 +0100
Subject: [PATCH] x86_64-windows GDB crash due to fs_base/gs_base registers
GDB is currently crashing anytime we try to access the fs_base/gs_base
registers, either to read them, or to write them. This can be observed
under various scenarios:
- Explicit reference to those registers (eg: print $fs_base) --
probably relatively rare;
- Calling a function in the inferior, with the crash happening
because we are trying to read those registers in order to save
their value ahead of making the function call;
- Just a plain "info registers";
The crash was introduced by the following commit:
| commit 48aeef91c248291dd03583798904612426b1f40a
| Date: Mon Jun 26 18:14:43 2017 -0700
| Subject: Include the fs_base and gs_base registers in amd64 target descriptions.
The Windows-nat implementation was unfortunately not prepared to deal
with those new registers. In particular, the way it fetches registers
is done by using a table where the index is the register number, and
the value at that index is the offset in the area in the thread's CONTEXT
data where the corresponding register value is stored.
For instance, in amd64-windows-nat.c, we can find the mappings static
array containing the following 57 elements in it:
#define context_offset(x) (offsetof (CONTEXT, x))
static const int mappings[] =
{
context_offset (Rax),
[...]
context_offset (FloatSave.MxCsr)
};
That array is then used by windows_fetch_one_register via:
char *context_offset = ((char *) &th->context) + mappings[r];
The problem is that fs_base's register number is 172, which is
well past the end of the mappings array (57 elements in total).
We end up getting an undefined offset, which happens to be so large
that it then causes the address where we try to read the register
value (a little bit later) to be invalid, thus crashing GDB with
a SEGV.
This patch side-steps the issue entirely by removing support for
those registers in GDB on x86_64-windows, because a look at the
CONTEXT structure indicates no support for getting those registers.
A more comprehensive fix would patch the potential buffer overflow
of the mappings array, but this can be done as a separate commit.
gdb/ChangeLog:
* gdb/amd64-tdep.h (amd64_create_target_description): Add
"segments" parameter.
* gdb/amd64-tdep.c (amd64_none_init_abi, amd64_x32_none_init_abi)
(_initialize_amd64_tdep): Update call to
amd64_create_target_description.
(amd64_target_description): Add "segments" parameter. Adjust
the implementation to use it.
* gdb/amd64-linux-tdep.c (amd64_linux_read_description): Update
call to amd64_create_target_description.
* gdb/amd64-windows-tdep.c (amd64_windows_init_abi): Likewise.
* gdb/arch/amd64.h (amd64_create_target_description): Add
"segments" register.
* gdb/arch/amd64.c (amd64_create_target_description): Add
"segments" parameter. Call create_feature_i386_64bit_segments
only if SEGMENTS is true.
* gdb/gdbserver/win32-i386-low.c (i386_arch_setup): Update
call to amd64_create_target_description.
Tested on x86_64-windows using AdaCore's testsuite (by Joel Brobecker
<brobecker at adacore dot com>).
---
gdb/ChangeLog | 20 ++++++++++++++++++++
gdb/amd64-linux-tdep.c | 3 ++-
gdb/amd64-tdep.c | 17 ++++++++++-------
gdb/amd64-tdep.h | 3 ++-
gdb/amd64-windows-tdep.c | 2 +-
gdb/arch/amd64.c | 9 ++++++---
gdb/arch/amd64.h | 2 +-
gdb/gdbserver/win32-i386-low.c | 2 +-
8 files changed, 43 insertions(+), 15 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0cbccf5f8a..c47c111466 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,23 @@
+2018-06-29 Pedro Alves <palves@redhat.com>
+
+ * gdb/amd64-tdep.h (amd64_create_target_description): Add
+ "segments" parameter.
+ * gdb/amd64-tdep.c (amd64_none_init_abi, amd64_x32_none_init_abi)
+ (_initialize_amd64_tdep): Update call to
+ amd64_create_target_description.
+ (amd64_target_description): Add "segments" parameter. Adjust
+ the implementation to use it.
+ * gdb/amd64-linux-tdep.c (amd64_linux_read_description): Update
+ call to amd64_create_target_description.
+ * gdb/amd64-windows-tdep.c (amd64_windows_init_abi): Likewise.
+ * gdb/arch/amd64.h (amd64_create_target_description): Add
+ "segments" register.
+ * gdb/arch/amd64.c (amd64_create_target_description): Add
+ "segments" parameter. Call create_feature_i386_64bit_segments
+ only if SEGMENTS is true.
+ * gdb/gdbserver/win32-i386-low.c (i386_arch_setup): Update
+ call to amd64_create_target_description.
+
2018-06-29 Pedro Alves <palves@redhat.com>
* thread.c (thread_target_id_str): New, factored out from ...
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index ef9248d708..7fe37d83f6 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -1594,7 +1594,8 @@ amd64_linux_read_description (uint64_t xcr0_features_bit, bool is_x32)
}
if (*tdesc == NULL)
- *tdesc = amd64_create_target_description (xcr0_features_bit, is_x32, true);
+ *tdesc = amd64_create_target_description (xcr0_features_bit, is_x32,
+ true, true);
return *tdesc;
}
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 9f8f018dd1..190e086ebf 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -3225,7 +3225,8 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
static void
amd64_none_init_abi (gdbarch_info info, gdbarch *arch)
{
- amd64_init_abi (info, arch, amd64_target_description (X86_XSTATE_SSE_MASK));
+ amd64_init_abi (info, arch, amd64_target_description (X86_XSTATE_SSE_MASK,
+ true));
}
static struct type *
@@ -3266,25 +3267,27 @@ static void
amd64_x32_none_init_abi (gdbarch_info info, gdbarch *arch)
{
amd64_x32_init_abi (info, arch,
- amd64_target_description (X86_XSTATE_SSE_MASK));
+ amd64_target_description (X86_XSTATE_SSE_MASK, true));
}
/* Return the target description for a specified XSAVE feature mask. */
const struct target_desc *
-amd64_target_description (uint64_t xcr0)
+amd64_target_description (uint64_t xcr0, bool segments)
{
static target_desc *amd64_tdescs \
- [2/*AVX*/][2/*MPX*/][2/*AVX512*/][2/*PKRU*/] = {};
+ [2/*AVX*/][2/*MPX*/][2/*AVX512*/][2/*PKRU*/][2/*segments*/] = {};
target_desc **tdesc;
tdesc = &amd64_tdescs[(xcr0 & X86_XSTATE_AVX) ? 1 : 0]
[(xcr0 & X86_XSTATE_MPX) ? 1 : 0]
[(xcr0 & X86_XSTATE_AVX512) ? 1 : 0]
- [(xcr0 & X86_XSTATE_PKRU) ? 1 : 0];
+ [(xcr0 & X86_XSTATE_PKRU) ? 1 : 0]
+ [segments ? 1 : 0];
if (*tdesc == NULL)
- *tdesc = amd64_create_target_description (xcr0, false, false);
+ *tdesc = amd64_create_target_description (xcr0, false, false,
+ segments);
return *tdesc;
}
@@ -3314,7 +3317,7 @@ _initialize_amd64_tdep (void)
for (auto &a : xml_masks)
{
- auto tdesc = amd64_target_description (a.mask);
+ auto tdesc = amd64_target_description (a.mask, true);
selftests::record_xml_tdesc (a.xml, tdesc);
}
diff --git a/gdb/amd64-tdep.h b/gdb/amd64-tdep.h
index 7d3791ad93..94e012632f 100644
--- a/gdb/amd64-tdep.h
+++ b/gdb/amd64-tdep.h
@@ -106,7 +106,8 @@ extern void amd64_init_abi (struct gdbarch_info info,
extern void amd64_x32_init_abi (struct gdbarch_info info,
struct gdbarch *gdbarch,
const target_desc *default_tdesc);
-extern const struct target_desc *amd64_target_description (uint64_t xcr0);
+extern const struct target_desc *amd64_target_description (uint64_t xcr0,
+ bool segments);
/* Fill register REGNUM in REGCACHE with the appropriate
floating-point or SSE register value from *FXSAVE. If REGNUM is
diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index 83a7f2f32e..904875bacc 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -1226,7 +1226,7 @@ amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
frame_unwind_append_unwinder (gdbarch, &amd64_windows_frame_unwind);
amd64_init_abi (info, gdbarch,
- amd64_target_description (X86_XSTATE_SSE_MASK));
+ amd64_target_description (X86_XSTATE_SSE_MASK, false));
windows_init_abi (info, gdbarch);
diff --git a/gdb/arch/amd64.c b/gdb/arch/amd64.c
index d31d8f1f75..4ad17c3fdc 100644
--- a/gdb/arch/amd64.c
+++ b/gdb/arch/amd64.c
@@ -33,10 +33,12 @@
/* Create amd64 target descriptions according to XCR0. If IS_X32 is
true, create the x32 ones. If IS_LINUX is true, create target
- descriptions for Linux. */
+ descriptions for Linux. If SEGMENTS is true, then include
+ the "org.gnu.gdb.i386.segments" feature registers. */
target_desc *
-amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux)
+amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux,
+ bool segments)
{
target_desc *tdesc = allocate_target_description ();
@@ -57,7 +59,8 @@ amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux)
regnum = create_feature_i386_64bit_sse (tdesc, regnum);
if (is_linux)
regnum = create_feature_i386_64bit_linux (tdesc, regnum);
- regnum = create_feature_i386_64bit_segments (tdesc, regnum);
+ if (segments)
+ regnum = create_feature_i386_64bit_segments (tdesc, regnum);
if (xcr0 & X86_XSTATE_AVX)
regnum = create_feature_i386_64bit_avx (tdesc, regnum);
diff --git a/gdb/arch/amd64.h b/gdb/arch/amd64.h
index c0c4dc27ef..4a659657da 100644
--- a/gdb/arch/amd64.h
+++ b/gdb/arch/amd64.h
@@ -19,4 +19,4 @@
#include <stdint.h>
target_desc *amd64_create_target_description (uint64_t xcr0, bool is_x32,
- bool is_linux);
+ bool is_linux, bool segments);
diff --git a/gdb/gdbserver/win32-i386-low.c b/gdb/gdbserver/win32-i386-low.c
index 16fe2c85b2..62221688cb 100644
--- a/gdb/gdbserver/win32-i386-low.c
+++ b/gdb/gdbserver/win32-i386-low.c
@@ -436,7 +436,7 @@ i386_arch_setup (void)
#ifdef __x86_64__
tdesc = amd64_create_target_description (X86_XSTATE_SSE_MASK, false,
- false);
+ false, false);
const char **expedite_regs = amd64_expedite_regs;
#else
tdesc = i386_create_target_description (X86_XSTATE_SSE_MASK, false);
--
2.17.1