Message ID | 1485436646-12223-1-git-send-email-antoine.tremblay@ericsson.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 39408 invoked by alias); 26 Jan 2017 13:17:48 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 39398 invoked by uid 89); 26 Jan 2017 13:17:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.0 required=5.0 tests=BAYES_20, SPF_PASS autolearn=ham version=3.3.2 spammy=5156, H*r:Security, H*r:Symantec, Hx-languages-length:2377 X-HELO: usplmg20.ericsson.net Received: from usplmg20.ericsson.net (HELO usplmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 26 Jan 2017 13:17:45 +0000 Received: from EUSAAHC006.ericsson.se (Unknown_Domain [147.117.188.90]) by (Symantec Mail Security) with SMTP id 25.79.29529.7FDF9885; Thu, 26 Jan 2017 14:47:38 +0100 (CET) Received: from elxa4wqvvz1.ca.am.ericsson.se (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.90) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 26 Jan 2017 08:17:40 -0500 From: Antoine Tremblay <antoine.tremblay@ericsson.com> To: <gdb-patches@sourceware.org> CC: Antoine Tremblay <antoine.tremblay@ericsson.com> Subject: [PATCH] Fix crash when loading a core with unexpected register section size Date: Thu, 26 Jan 2017 08:17:26 -0500 Message-ID: <1485436646-12223-1-git-send-email-antoine.tremblay@ericsson.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes |
Commit Message
Antoine Tremblay
Jan. 26, 2017, 1:17 p.m. UTC
When loading a core without an executable like so: gdb --core core for example often the gdbarch won't contain the iterate_over_regset_sections method. This will generate a call to get_core_register_section with a NULL regset like at corelow.c:628 get_core_register_section (regcache, NULL, ".reg", 0, 0, "general-purpose", 1); However a check for REGSET_VARIABLE_SIZE in get_core_register_section assumes that regset is != NULL thus leading to a crash with this backtrace: (gdb) bt #0 0x000000000065907b in get_core_register_section (regcache=regcache@entry=0x2c26260, regset=regset@entry=0x0, name=name@entry=0xdbf7b2 ".reg", min_size=min_size@entry=0, which=which@entry=0, human_name=human_name@entry=0xdbac28 "general-purpose", required=1) at ../../gdb/corelow.c:542 #1 0x0000000000659b70 in get_core_registers (ops=<optimized out>, regcache=0x2c26260, regno=<optimized out>) at ../../gdb/corelow.c:628 #2 0x000000000076e5fb in target_fetch_registers (regcache=regcache@entry=0x2c26260, regno=regno@entry=15) at ../../gdb/target.c:3590 Note that commit: f962539ad23759af4ba8f7eece1946fdc2f5087 introcuded this issue. Thus releases > 7.8.2 are affected. This patch fixes this crash by adding a check for regset existence before running the condition. gdb/ChangeLog: * corelow.c (get_core_register_section): Check for regset existance before checking for REGSET_VARIABLE_SIZE. --- gdb/corelow.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
Comments
On 01/26/2017 01:17 PM, Antoine Tremblay wrote: > When loading a core without an executable like so: > gdb --core core for example often the gdbarch won't contain the > iterate_over_regset_sections method. Can you give an example? That'd help a lot understand the issue better. Also, please add a line break, ""s and/or punctuation to make the command stand out more from the rest of the sentence. For example: When loading a core without an executable like so: $ gdb --core core for example, often the gdbarch won't contain the iterate_over_regset_sections method. For example arch-foo. > > This will generate a call to get_core_register_section with a NULL regset > like at corelow.c:628 > > get_core_register_section (regcache, NULL, ".reg", 0, 0, "general-purpose", 1); > > However a check for REGSET_VARIABLE_SIZE in get_core_register_section > assumes that regset is != NULL thus leading to a crash with this backtrace: > > (gdb) bt > #0 0x000000000065907b in get_core_register_section > (regcache=regcache@entry=0x2c26260, regset=regset@entry=0x0, > name=name@entry=0xdbf7b2 ".reg", min_size=min_size@entry=0, > which=which@entry=0, human_name=human_name@entry=0xdbac28 > "general-purpose", required=1) > at ../../gdb/corelow.c:542 > #1 0x0000000000659b70 in get_core_registers (ops=<optimized out>, > regcache=0x2c26260, regno=<optimized out>) at ../../gdb/corelow.c:628 > #2 0x000000000076e5fb in target_fetch_registers > (regcache=regcache@entry=0x2c26260, regno=regno@entry=15) at ../../gdb/target.c:3590 > > Note that commit: f962539ad23759af4ba8f7eece1946fdc2f5087 Please always paste the commit's subject as well, to make it easier for us poor humans to quickly tell what the commit was about without having to go to a terminal. The Linux guideline is to put it in parens: Note that commit f962539ad23759 ("Warn if core file register section is larger than expected") introduced [...] I personally like that style. introcuded this (typo) > issue. Thus releases > 7.8.2 are affected. > > This patch fixes this crash by adding a check for regset existence before > running the condition. > > gdb/ChangeLog: > > * corelow.c (get_core_register_section): Check for regset > existance before checking for REGSET_VARIABLE_SIZE. Indentation. "existence". > --- > gdb/corelow.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/gdb/corelow.c b/gdb/corelow.c > index a075d9e..f43f730 100644 > --- a/gdb/corelow.c > +++ b/gdb/corelow.c > @@ -515,6 +515,7 @@ get_core_register_section (struct regcache *regcache, > struct bfd_section *section; > bfd_size_type size; > char *contents; > + bool variable_size_section = false; No need to initialize by default when you're always going to initialize it again below. Or declare on first use and avoid the issue entirely. Thanks, Pedro Alves
Pedro Alves writes: > On 01/26/2017 01:17 PM, Antoine Tremblay wrote: >> When loading a core without an executable like so: >> gdb --core core for example often the gdbarch won't contain the >> iterate_over_regset_sections method. > > Can you give an example? That'd help a lot understand the issue > better. > I can't share the core that I have that reproduced this :( > Also, please add a line break, ""s and/or punctuation to make > the command stand out more from the rest of the sentence. > For example: > > When loading a core without an executable like so: > $ gdb --core core > for example, often the gdbarch won't contain the > iterate_over_regset_sections method. For example arch-foo. > OK. >> >> This will generate a call to get_core_register_section with a NULL regset >> like at corelow.c:628 >> >> get_core_register_section (regcache, NULL, ".reg", 0, 0, "general-purpose", 1); >> >> However a check for REGSET_VARIABLE_SIZE in get_core_register_section >> assumes that regset is != NULL thus leading to a crash with this backtrace: >> >> (gdb) bt >> #0 0x000000000065907b in get_core_register_section >> (regcache=regcache@entry=0x2c26260, regset=regset@entry=0x0, >> name=name@entry=0xdbf7b2 ".reg", min_size=min_size@entry=0, >> which=which@entry=0, human_name=human_name@entry=0xdbac28 >> "general-purpose", required=1) >> at ../../gdb/corelow.c:542 >> #1 0x0000000000659b70 in get_core_registers (ops=<optimized out>, >> regcache=0x2c26260, regno=<optimized out>) at ../../gdb/corelow.c:628 >> #2 0x000000000076e5fb in target_fetch_registers >> (regcache=regcache@entry=0x2c26260, regno=regno@entry=15) at ../../gdb/target.c:3590 >> >> Note that commit: f962539ad23759af4ba8f7eece1946fdc2f5087 > > Please always paste the commit's subject as well, to make > it easier for us poor humans to quickly tell what the commit > was about without having to go to a terminal. The Linux guideline > is to put it in parens: > > Note that commit f962539ad23759 ("Warn if core file register > section is larger than expected") introduced [...] > > I personally like that style. > OK thanks, > introcuded this > > (typo) > Indeed. >> issue. Thus releases > 7.8.2 are affected. >> >> This patch fixes this crash by adding a check for regset existence before >> running the condition. >> >> gdb/ChangeLog: >> >> * corelow.c (get_core_register_section): Check for regset >> existance before checking for REGSET_VARIABLE_SIZE. > > Indentation. "existence". > Yes oops. >> --- >> gdb/corelow.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/gdb/corelow.c b/gdb/corelow.c >> index a075d9e..f43f730 100644 >> --- a/gdb/corelow.c >> +++ b/gdb/corelow.c >> @@ -515,6 +515,7 @@ get_core_register_section (struct regcache *regcache, >> struct bfd_section *section; >> bfd_size_type size; >> char *contents; >> + bool variable_size_section = false; > > No need to initialize by default when you're always going to > initialize it again below. Or declare on first use and avoid > the issue entirely. > Indeed. fixed. V2 coming in next mail.
On 01/26/2017 01:54 PM, Antoine Tremblay wrote: > > Pedro Alves writes: > >> On 01/26/2017 01:17 PM, Antoine Tremblay wrote: >>> When loading a core without an executable like so: >>> gdb --core core for example often the gdbarch won't contain the >>> iterate_over_regset_sections method. >> >> Can you give an example? That'd help a lot understand the issue >> better. >> > > I can't share the core that I have that reproduced this :( I meant an example gdbarch. It sounded like this would happen with any core with that architecture? Thanks, Pedro Alves
Pedro Alves writes: > On 01/26/2017 01:54 PM, Antoine Tremblay wrote: >> >> Pedro Alves writes: >> >>> On 01/26/2017 01:17 PM, Antoine Tremblay wrote: >>>> When loading a core without an executable like so: >>>> gdb --core core for example often the gdbarch won't contain the >>>> iterate_over_regset_sections method. >>> >>> Can you give an example? That'd help a lot understand the issue >>> better. >>> >> >> I can't share the core that I have that reproduced this :( > > I meant an example gdbarch. It sounded like this would > happen with any core with that architecture? Ho yes, see v2, I added "For example arch-arm." Like you suggested.
On 01/26/2017 02:31 PM, Antoine Tremblay wrote: > > Pedro Alves writes: > >> On 01/26/2017 01:54 PM, Antoine Tremblay wrote: >>> >>> Pedro Alves writes: >>> >>>> On 01/26/2017 01:17 PM, Antoine Tremblay wrote: >>>>> When loading a core without an executable like so: >>>>> gdb --core core for example often the gdbarch won't contain the >>>>> iterate_over_regset_sections method. >>>> >>>> Can you give an example? That'd help a lot understand the issue >>>> better. >>>> >>> >>> I can't share the core that I have that reproduced this :( >> >> I meant an example gdbarch. It sounded like this would >> happen with any core with that architecture? > > Ho yes, see v2, I added "For example arch-arm." Like you suggested. Eh, "arch-" in "arch-foo" was just meant to show I was talking about an arch. I didn't mean for you to keep the "arch-" part. :-) So basically, we could have a testcase that dumps a file, and then loads with back with no executable loaded? Do we really not have such a testcase yet? Thanks, Pedro Alves
Pedro Alves writes: > On 01/26/2017 02:31 PM, Antoine Tremblay wrote: >> >> Pedro Alves writes: >> >>> On 01/26/2017 01:54 PM, Antoine Tremblay wrote: >>>> >>>> Pedro Alves writes: >>>> >>>>> On 01/26/2017 01:17 PM, Antoine Tremblay wrote: >>>>>> When loading a core without an executable like so: >>>>>> gdb --core core for example often the gdbarch won't contain the >>>>>> iterate_over_regset_sections method. >>>>> >>>>> Can you give an example? That'd help a lot understand the issue >>>>> better. >>>>> >>>> >>>> I can't share the core that I have that reproduced this :( >>> >>> I meant an example gdbarch. It sounded like this would >>> happen with any core with that architecture? >> >> Ho yes, see v2, I added "For example arch-arm." Like you suggested. > > Eh, "arch-" in "arch-foo" was just meant to show I was talking > about an arch. I didn't mean for you to keep the "arch-" part. :-) hehe I figured but wasn't sure. I'll just say arm. > > So basically, we could have a testcase that dumps a file, and then > loads with back with no executable loaded? Do we really not > have such a testcase yet? > Not exactly if it was that simple it would have been catched by gdb.base/corefile.exp The problem is that this triggers only if the core file register section is larger than expected. And if you just create a core and read it the register section is ok. However crafting a core with this problem is non-trivial at least to my current knowledge. Thanks, Antoine
On 01/26/2017 02:59 PM, Antoine Tremblay wrote: > >>> Ho yes, see v2, I added "For example arch-arm." Like you suggested. >> >> Eh, "arch-" in "arch-foo" was just meant to show I was talking >> about an arch. I didn't mean for you to keep the "arch-" part. :-) > > hehe I figured but wasn't sure. I'll just say arm. The right name is uppercase "ARM". ;-) >> So basically, we could have a testcase that dumps a file, and then >> loads with back with no executable loaded? Do we really not >> have such a testcase yet? >> > > Not exactly if it was that simple it would have been catched by > gdb.base/corefile.exp > > The problem is that this triggers only if the core file register section > is larger than expected. And if you just create a core and read it the > register section is ok. > > However crafting a core with this problem is non-trivial at least to my > current knowledge. This is all information that would have been very handy to have in the submission upfront. Please put it in the commit log. OK with that change. One piece of info missing is why didn't GDB figure out this is a Linux core anyway, assuming it's a Linux core dump. I think the answer is that there's no ".note.ABI-tag"/NT_GNU_ABI_TAG section/note in core dumps. I think we could teach generic_elf_osabi_sniff_abi_tag_sections about detecting presence of ".note.linuxcore" sections: $ objdump -h ./testsuite/core.7452 [...] 3 .note.linuxcore.siginfo/7452 00000080 0000000000000000 0000000000000000 0000075c 2**2 [...] And then we'd end up with a gdbarch that has arm_linux_iterate_over_regset_sections installed, and thus no crash. But we shouldn't crash if NT_SIGINFO notes are missing, so the patch is OK as is. Thanks, Pedro Alves
Pedro Alves writes: > On 01/26/2017 02:59 PM, Antoine Tremblay wrote: >> > >>>> Ho yes, see v2, I added "For example arch-arm." Like you suggested. >>> >>> Eh, "arch-" in "arch-foo" was just meant to show I was talking >>> about an arch. I didn't mean for you to keep the "arch-" part. :-) >> >> hehe I figured but wasn't sure. I'll just say arm. > > The right name is uppercase "ARM". ;-) > Right right :) >>> So basically, we could have a testcase that dumps a file, and then >>> loads with back with no executable loaded? Do we really not >>> have such a testcase yet? >>> >> >> Not exactly if it was that simple it would have been catched by >> gdb.base/corefile.exp >> >> The problem is that this triggers only if the core file register section >> is larger than expected. And if you just create a core and read it the >> register section is ok. >> >> However crafting a core with this problem is non-trivial at least to my >> current knowledge. > > This is all information that would have been very handy to have > in the submission upfront. Please put it in the commit log. > OK with that change. OK > > One piece of info missing is why didn't GDB figure out this is > a Linux core anyway, assuming it's a Linux core dump. > > I think the answer is that there's no ".note.ABI-tag"/NT_GNU_ABI_TAG > section/note in core dumps. I think we could teach > generic_elf_osabi_sniff_abi_tag_sections about detecting > presence of ".note.linuxcore" sections: > > $ objdump -h ./testsuite/core.7452 > [...] > 3 .note.linuxcore.siginfo/7452 00000080 0000000000000000 0000000000000000 0000075c 2**2 > [...] > > And then we'd end up with a gdbarch that has > arm_linux_iterate_over_regset_sections installed, and thus no > crash. > Quite interesting, I'll investigate that and see if we can submit an improvement along those lines. Thanks!
diff --git a/gdb/corelow.c b/gdb/corelow.c index a075d9e..f43f730 100644 --- a/gdb/corelow.c +++ b/gdb/corelow.c @@ -515,6 +515,7 @@ get_core_register_section (struct regcache *regcache, struct bfd_section *section; bfd_size_type size; char *contents; + bool variable_size_section = false; xfree (section_name); @@ -534,12 +535,16 @@ get_core_register_section (struct regcache *regcache, } size = bfd_section_size (core_bfd, section); + + variable_size_section = (regset != NULL + && regset->flags & REGSET_VARIABLE_SIZE); + if (size < min_size) { warning (_("Section `%s' in core file too small."), section_name); return; } - if (size != min_size && !(regset->flags & REGSET_VARIABLE_SIZE)) + if (size != min_size && !variable_size_section) { warning (_("Unexpected size of section `%s' in core file."), section_name);