From patchwork Tue Jun 26 21:53:33 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joel Brobecker X-Patchwork-Id: 28061 Received: (qmail 19235 invoked by alias); 26 Jun 2018 21:53:40 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 19220 invoked by uid 89); 26 Jun 2018 21:53:39 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=relatively, gs_base, fs_base, descriptions X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 26 Jun 2018 21:53:37 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 8E7C0560F2; Tue, 26 Jun 2018 17:53:35 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id R7SSIAsCmBiU; Tue, 26 Jun 2018 17:53:35 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 5816D560ED; Tue, 26 Jun 2018 17:53:35 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 6BF1D8795C; Tue, 26 Jun 2018 14:53:33 -0700 (PDT) Date: Tue, 26 Jun 2018 14:53:33 -0700 From: Joel Brobecker To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers Message-ID: <20180626215333.GC8075@adacore.com> References: <1529952947-48942-1-git-send-email-brobecker@adacore.com> <1529952947-48942-3-git-send-email-brobecker@adacore.com> <693520df-0ffd-4486-3066-9ad314bb18ff@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <693520df-0ffd-4486-3066-9ad314bb18ff@redhat.com> User-Agent: Mutt/1.9.4 (2018-02-28) > I spent a bit playing with this today. See comments below. Thank you! > remote debugging when the remote target does not supply an explicit > xml target description. The only thing that can be done is add > new registers at the end. > > So to fix this, we could also make windows_nat_target::{fetch,store}_registers > check whether the requested register is within the mappings array. We could > do that by simply changing the 'mappings' global from a plain pointer to a > gdb::array_view for example (which stores the array's size). I suspect the > current table based approach generates a little better code (the compiler may > end up generating a table anyway from the switch/case in your patch, but it's > not garanteed), though it shouldn't really matter in practice. > > But see below. I am OK with taking the bounded-buffer approach, as it would clearly already be an improvement. I thought about something like that, but using a C-oriented solution, which was not elegant, so discarded it then. Your suggestion takes care of that aspect. However, thinking further about this, we have this huge gap between the majority of the registers, and the next ones. What if we wanted one day to add more? We'd have to triple the size of the table and keep it mostly empty. The counter point is that I tend to think that this is probably unlikely. > Since Windows doesn't expose these to userspace, I'm thinking that it > doesn't make much sense to show the registers at all? The (xml) target > features are designed exactly such that if you don't have some > optional feature (like org.gnu.gdb.i386.segments), then the corresponding > registers don't exist at all in gdb. > > The attached patch implements that. Makes sense to me indeed. I tested your patch on x86_64-windows, and didn't detect any issues with it, so I suggest we push that (see attached patch with comments added and initial revision log provided as well -- hopefully a modest thank you for sending the patch showing how it's done). ... and then decide whether we want to reorganize a bit the way we get the index of each register in the CONTEXT structure. I would say that we do want to do something. Perhaps, the path of least resistance is to just change the mappings structure from a C array to a gdb::array_view as you suggested. I may have a preference for the approach I took, but it is a large diff, and it's not clear whether it's going to be beneficial in the long run... You pick! ;-) I'll take care of your comments if you chose the first patch. I'll send a new one if you prefere the gdb::array_view approach. Thanks Pedro! From 12fceda7e7835925c92ef8185cb43b57fd468667 Mon Sep 17 00:00:00 2001 From: Pedro Alves 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. --- 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 +- 7 files changed, 23 insertions(+), 15 deletions(-) diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c index 521e32a..454dc93 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 9f8f018..190e086 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 7d3791a..94e0126 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 83a7f2f..904875b 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 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. */ 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 c0c4dc2..4a65965 100644 --- a/gdb/arch/amd64.h +++ b/gdb/arch/amd64.h @@ -19,4 +19,4 @@ #include 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 16fe2c8..6222168 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.8.3