[PATCH] Fix crash when loading a core with unexpected register section size

[PATCH] Fix crash when loading a core with unexpected register section size

[Antoine Tremblay]

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(-)

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);
-- 
2.7.4

Re: [PATCH] Fix crash when loading a core with unexpected register section size

[Pedro Alves]

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

Re: [PATCH] Fix crash when loading a core with unexpected register section size

[Antoine Tremblay]

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.

[PATCH v2] Fix crash when loading a core with unexpected register section size

[Antoine Tremblay]

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-arm.

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: f962539ad23759 ("Warn if core file register
section is larger than expected") introduced 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
	existence before checking for REGSET_VARIABLE_SIZE.
---
 gdb/corelow.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index a075d9e..ecde954 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -515,6 +515,8 @@ get_core_register_section (struct regcache *regcache,
   struct bfd_section *section;
   bfd_size_type size;
   char *contents;
+  bool variable_size_section = (regset != NULL
+				&& regset->flags & REGSET_VARIABLE_SIZE);
 
   xfree (section_name);
 
@@ -539,7 +541,7 @@ get_core_register_section (struct regcache *regcache,
       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);
-- 
2.7.4

Re: [PATCH] Fix crash when loading a core with unexpected register section size

[Pedro Alves]

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

Re: [PATCH] Fix crash when loading a core with unexpected register section size

[Antoine Tremblay]

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.

Re: [PATCH] Fix crash when loading a core with unexpected register section size

[Pedro Alves]

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

Re: [PATCH] Fix crash when loading a core with unexpected register section size

[Antoine Tremblay]

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

Re: [PATCH] Fix crash when loading a core with unexpected register section size

[Pedro Alves]

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

Re: [PATCH] Fix crash when loading a core with unexpected register section size

[Antoine Tremblay]

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!

[pushed][PATCH v2] Fix crash when loading a core with unexpected register section size

[Antoine Tremblay]

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 on ARM.

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: f962539ad23759 ("Warn if core file register
section is larger than expected") introduced this issue.
Thus releases > 7.8.2 are affected.

Also, this would have been caught by gdb.base/corefile.exp but the
problem is that this triggers only if the core dump is missing some data
so that it's not recognized as a linux core dump, or it's not a linux core
dump and the core file register section is larger than expected.

So if you just create a core and read it on linux with ARM the osabi is
detected properly and iterate_over_regset_sections is present and so the
problem is not triggered.

Thus creating a linux test for this with a crafted core that meets the
problem requirements is non-trivial.

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
	existence before checking for REGSET_VARIABLE_SIZE.
---
 gdb/ChangeLog | 5 +++++
 gdb/corelow.c | 4 +++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4e85fcd..ce3f317 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2017-01-26  Antoine Tremblay  <antoine.tremblay@ericsson.com>
+
+	* corelow.c (get_core_register_section): Check for regset
+	existence before checking for REGSET_VARIABLE_SIZE.
+
 2017-01-26  Yao Qi  <yao.qi@linaro.org>
 	    Pedro Alves  <palves@redhat.com>
 
diff --git a/gdb/corelow.c b/gdb/corelow.c
index a075d9e..ecde954 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -515,6 +515,8 @@ get_core_register_section (struct regcache *regcache,
   struct bfd_section *section;
   bfd_size_type size;
   char *contents;
+  bool variable_size_section = (regset != NULL
+				&& regset->flags & REGSET_VARIABLE_SIZE);
 
   xfree (section_name);
 
@@ -539,7 +541,7 @@ get_core_register_section (struct regcache *regcache,
       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);
-- 
2.7.4