Fix crash when loading a core with unexpected register section size

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

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!