[RFC] fix gdb segv when objfile can't be opened

  Here is a new version of the patch.  It changes the following from the original
patch:

1) It adds a testsuite which locks down:
  a) The stack frame is printed when the breakpoint is hit
  b) 'next' runs without causing a segv
2) No longer set descriptor->size in gdb_bfd_map_section when an error is hit.
   It doesn't look like this is needed as gdb_bfd_map_section is the only place
   that consumes that descriptor->size field.
3) Update the description in gdb_bfd.h to indicate that if there is an error
   mapping the section, a warning is generated, *SIZE is set to 0, and NULL is
   returned.

I considered catching the error in the caller of gdb_bfd_map_section, which is
dwarf2_read_section in dwarf2read.c.  It didn't seem to make any more sense to
catch the error here than it would directly in gdb_bfd_map_section.  If
anything, it would make more sense to catch the error in the caller of
dwarf2_read_section, which in this case is dwarf2_get_section_info.  However
there are many callers of dwarf2_read_section.  It doesn't make sense to force
all of them to add a TRY/CATCH around calling dwarf2_read_section.

Please let me know thoughts/feedback.

Also see the note in the test case about not being able to make 'shlib=...'
work when compiling a shared library that links against another shared
library.  The workaround I used is kind of ugly.

Thanks,
Mike
---

From d59bb69db0f10f8cb7d91568907b505f05424df7 Mon Sep 17 00:00:00 2001
From: Mike Gulick <mgulick@mathworks.com>
Date: Wed, 18 Oct 2017 16:04:27 -0400
Subject: [PATCH v2] fix gdb segv when objfile can't be opened

This fixes PR 16577.

This patch changes gdb_bfd_map_section to issue a warning rather than an
error if it is unable to read the object file, and sets the size of the
section/frame that it attempted to read to 0 on error.

The description of gdb_bfd_map_section states that it will try to read
or map the contents of the section SECT, and if successful, the section
data is returned and *SIZE is set to the size of the section data.  This
function was throwing an error and leaving *SIZE as-is.  Setting the
section size to 0 indicates to dwarf2_build_frame_info that there is no
data to read, otherwise it will try to read from an invalid frame
pointer.

Changing the error to a warning allows this to be handled gracefully.
Additionally, the error was clobbering the breakpoint output indicating
the current frame (function name, arguments, source file, and line number).
E.g.

Thread 3 "foo" hit Breakpoint 1, BFD: reopening /tmp/jna-1013829440/jna2973250704389291330.tmp: No such file or directory

BFD: reopening /tmp/jna-1013829440/jna2973250704389291330.tmp: No such file or directory

(gdb)

While the "BFD: reopening ..." messages will still appear interspersed in the
breakpoint output, the current frame info is now displayed:

Thread 3 "foo" hit Breakpoint 1, BFD: reopening /tmp/jna-1013829440/jna1875755897659885075.tmp: No such file or directory

BFD: reopening /tmp/jna-1013829440/jna1875755897659885075.tmp: No such file or directory

warning: Can't read data for section '.eh_frame' in file '/tmp/jna-1013829440/jna1875755897659885075.tmp'
do_something () at file.cpp:80
80	{
(gdb)
---
 gdb/gdb_bfd.c                              |  11 ++-
 gdb/gdb_bfd.h                              |  17 +++--
 gdb/testsuite/gdb.base/solib-vanish-lib1.c |  22 ++++++
 gdb/testsuite/gdb.base/solib-vanish-lib2.c |  20 ++++++
 gdb/testsuite/gdb.base/solib-vanish-main.c |  62 +++++++++++++++++
 gdb/testsuite/gdb.base/solib-vanish.exp    | 108 +++++++++++++++++++++++++++++
 6 files changed, 228 insertions(+), 12 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/solib-vanish-lib1.c
 create mode 100644 gdb/testsuite/gdb.base/solib-vanish-lib2.c
 create mode 100644 gdb/testsuite/gdb.base/solib-vanish-main.c
 create mode 100644 gdb/testsuite/gdb.base/solib-vanish.exp
  

On 2017-10-23 07:19 PM, Mike Gulick wrote:
> Here is a new version of the patch.  It changes the following from the original
> patch:
> 
> 1) It adds a testsuite which locks down:
>   a) The stack frame is printed when the breakpoint is hit
>   b) 'next' runs without causing a segv
> 2) No longer set descriptor->size in gdb_bfd_map_section when an error is hit.
>    It doesn't look like this is needed as gdb_bfd_map_section is the only place
>    that consumes that descriptor->size field.
> 3) Update the description in gdb_bfd.h to indicate that if there is an error
>    mapping the section, a warning is generated, *SIZE is set to 0, and NULL is
>    returned.
> 
> I considered catching the error in the caller of gdb_bfd_map_section, which is
> dwarf2_read_section in dwarf2read.c.  It didn't seem to make any more sense to
> catch the error here than it would directly in gdb_bfd_map_section.  If
> anything, it would make more sense to catch the error in the caller of
> dwarf2_read_section, which in this case is dwarf2_get_section_info.  However
> there are many callers of dwarf2_read_section.  It doesn't make sense to force
> all of them to add a TRY/CATCH around calling dwarf2_read_section.
> 
> Please let me know thoughts/feedback.
> 
> Also see the note in the test case about not being able to make 'shlib=...'
> work when compiling a shared library that links against another shared
> library.  The workaround I used is kind of ugly.

Hi Mike,

I started looking at your patch and thought I would take a look at this problem.
The problem seems to be that gdb_compile_shlib builds the shared library in two
steps:

  .c -> .o -> .so

because it's not possible to do it in one pass.  As of today, if you pass an
shlib= option to gdb_compile_shlib, it will be passed to both compilation steps,
which means Dejagnu will pass the path to lib1.so while compiling the .c to a .o,
which makes gcc complain like this:

  warning: .../solib-vanish-lib2.so: linker input file unused because linking not done

I think the solution would be not to pass shlib= for this step, only for the final
linking.  I'll try to write a patch about this.

Simon
  

On 2017-10-23 07:19 PM, Mike Gulick wrote:
> Here is a new version of the patch.  It changes the following from the original
> patch:
> 
> 1) It adds a testsuite which locks down:
>   a) The stack frame is printed when the breakpoint is hit
>   b) 'next' runs without causing a segv
> 2) No longer set descriptor->size in gdb_bfd_map_section when an error is hit.
>    It doesn't look like this is needed as gdb_bfd_map_section is the only place
>    that consumes that descriptor->size field.
> 3) Update the description in gdb_bfd.h to indicate that if there is an error
>    mapping the section, a warning is generated, *SIZE is set to 0, and NULL is
>    returned.
> 
> I considered catching the error in the caller of gdb_bfd_map_section, which is
> dwarf2_read_section in dwarf2read.c.  It didn't seem to make any more sense to
> catch the error here than it would directly in gdb_bfd_map_section.  If
> anything, it would make more sense to catch the error in the caller of
> dwarf2_read_section, which in this case is dwarf2_get_section_info.  However
> there are many callers of dwarf2_read_section.  It doesn't make sense to force
> all of them to add a TRY/CATCH around calling dwarf2_read_section.
> 
> Please let me know thoughts/feedback.
> 
> Also see the note in the test case about not being able to make 'shlib=...'
> work when compiling a shared library that links against another shared
> library.  The workaround I used is kind of ugly.

Thanks a lot.  Could you also write the corresponding ChangeLog entries?  See
here for details: http://sourceware.org/gdb/wiki/ContributionChecklist
Note that we usually put that as part of the commit log, and only move it
to the actual ChangeLog files before pushing to git.  The changes to the
source files will go in gdb/ChangeLog and the changes to the testsuite in
gdb/testsuite/ChangeLog.

You can also put the PR number as part of the ChangeLog (see wiki and
previous CL entries for examples).

I don't think you have a copyright assignment for GDB yet?  Since this is
more than a few lines, you will need one if you want to contribute (I'll
contact you off-list for that).

I noted a a few comments below (mostly minor/formatting stuff, overall I
think this is a good quality submission).

> Thanks,
> Mike
> ---
> 
> From d59bb69db0f10f8cb7d91568907b505f05424df7 Mon Sep 17 00:00:00 2001
> From: Mike Gulick <mgulick@mathworks.com>
> Date: Wed, 18 Oct 2017 16:04:27 -0400
> Subject: [PATCH v2] fix gdb segv when objfile can't be opened
> 
> This fixes PR 16577.
> 
> This patch changes gdb_bfd_map_section to issue a warning rather than an
> error if it is unable to read the object file, and sets the size of the
> section/frame that it attempted to read to 0 on error.
> 
> The description of gdb_bfd_map_section states that it will try to read
> or map the contents of the section SECT, and if successful, the section
> data is returned and *SIZE is set to the size of the section data.  This
> function was throwing an error and leaving *SIZE as-is.  Setting the
> section size to 0 indicates to dwarf2_build_frame_info that there is no
> data to read, otherwise it will try to read from an invalid frame
> pointer.
> 
> Changing the error to a warning allows this to be handled gracefully.
> Additionally, the error was clobbering the breakpoint output indicating
> the current frame (function name, arguments, source file, and line number).
> E.g.
> 
> Thread 3 "foo" hit Breakpoint 1, BFD: reopening /tmp/jna-1013829440/jna2973250704389291330.tmp: No such file or directory
> 
> BFD: reopening /tmp/jna-1013829440/jna2973250704389291330.tmp: No such file or directory
> 
> (gdb)
> 
> While the "BFD: reopening ..." messages will still appear interspersed in the
> breakpoint output, the current frame info is now displayed:
> 
> Thread 3 "foo" hit Breakpoint 1, BFD: reopening /tmp/jna-1013829440/jna1875755897659885075.tmp: No such file or directory
> 
> BFD: reopening /tmp/jna-1013829440/jna1875755897659885075.tmp: No such file or directory
> 
> warning: Can't read data for section '.eh_frame' in file '/tmp/jna-1013829440/jna1875755897659885075.tmp'
> do_something () at file.cpp:80
> 80	{
> (gdb)
> ---
>  gdb/gdb_bfd.c                              |  11 ++-
>  gdb/gdb_bfd.h                              |  17 +++--
>  gdb/testsuite/gdb.base/solib-vanish-lib1.c |  22 ++++++
>  gdb/testsuite/gdb.base/solib-vanish-lib2.c |  20 ++++++
>  gdb/testsuite/gdb.base/solib-vanish-main.c |  62 +++++++++++++++++
>  gdb/testsuite/gdb.base/solib-vanish.exp    | 108 +++++++++++++++++++++++++++++
>  6 files changed, 228 insertions(+), 12 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/solib-vanish-lib1.c
>  create mode 100644 gdb/testsuite/gdb.base/solib-vanish-lib2.c
>  create mode 100644 gdb/testsuite/gdb.base/solib-vanish-main.c
>  create mode 100644 gdb/testsuite/gdb.base/solib-vanish.exp
> 
> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
> index cc02740..b33de07 100644
> --- a/gdb/gdb_bfd.c
> +++ b/gdb/gdb_bfd.c
> @@ -702,9 +702,14 @@ gdb_bfd_map_section (asection *sectp, bfd_size_type *size)
>  
>    data = NULL;
>    if (!bfd_get_full_section_contents (abfd, sectp, &data))
> -    error (_("Can't read data for section '%s' in file '%s'"),
> -	   bfd_get_section_name (abfd, sectp),
> -	   bfd_get_filename (abfd));
> +    {
> +      warning (_("Can't read data for section '%s' in file '%s'"),
> +	       bfd_get_section_name (abfd, sectp),
> +	       bfd_get_filename (abfd));
> +      /* Section is invalid -- set size to 0 and return NULL */

Finish the comment with a dot and two spaces.

> +      *size = 0;
> +      return (const gdb_byte *) NULL;
> +    }
>    descriptor->data = data;
>  
>   done:
> diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
> index b1ff857..e427781 100644
> --- a/gdb/gdb_bfd.h
> +++ b/gdb/gdb_bfd.h
> @@ -115,15 +115,14 @@ void gdb_bfd_mark_parent (bfd *child, bfd *parent);
>  
>  void gdb_bfd_record_inclusion (bfd *includer, bfd *includee);
>  
> -/* Try to read or map the contents of the section SECT.  If
> -   successful, the section data is returned and *SIZE is set to the
> -   size of the section data; this may not be the same as the size
> -   according to bfd_get_section_size if the section was compressed.
> -   The returned section data is associated with the BFD and will be
> -   destroyed when the BFD is destroyed.  There is no other way to free
> -   it; for temporary uses of section data, see
> -   bfd_malloc_and_get_section.  SECT may not have relocations.  This
> -   function will throw on error.  */
> +/* Try to read or map the contents of the section SECT.  If successful, the
> +   section data is returned and *SIZE is set to the size of the section data;
> +   this may not be the same as the size according to bfd_get_section_size if the
> +   section was compressed.  The returned section data is associated with the BFD
> +   and will be destroyed when the BFD is destroyed.  There is no other way to
> +   free it; for temporary uses of section data, see bfd_malloc_and_get_section.
> +   SECT may not have relocations.  If there is an error reading the section,
> +   this issues a warning, sets *SIZE to 0, and returns NULL.  */
>  
>  const gdb_byte *gdb_bfd_map_section (asection *section, bfd_size_type *size);
>  
> diff --git a/gdb/testsuite/gdb.base/solib-vanish-lib1.c b/gdb/testsuite/gdb.base/solib-vanish-lib1.c
> new file mode 100644
> index 0000000..f8e8182
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/solib-vanish-lib1.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2017 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +extern int bar (int);
> +
> +int foo (int x) {
> +  return bar (x);
> +}

Unless the purpose of the test is about formatting, we try to follow GNU
style (same as for source code) in tests.  For example, here it would be:

int
foo (int x)
{
  return bar (x);
}

> diff --git a/gdb/testsuite/gdb.base/solib-vanish-lib2.c b/gdb/testsuite/gdb.base/solib-vanish-lib2.c
> new file mode 100644
> index 0000000..7799d92
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/solib-vanish-lib2.c
> @@ -0,0 +1,20 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2017 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +int bar (int y) {
> +  return y + 1; /* break here */
> +}

Same here.

> diff --git a/gdb/testsuite/gdb.base/solib-vanish-main.c b/gdb/testsuite/gdb.base/solib-vanish-main.c
> new file mode 100644
> index 0000000..0e21e9c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/solib-vanish-main.c
> @@ -0,0 +1,62 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2017 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <dlfcn.h>
> +#include <stdlib.h>
> +
> +#ifndef VANISH_LIB
> +#define VANISH_LIB "./solib-vanish-lib1.so"
> +#endif

I think we can remove this, it could only obfuscate a problem
if we remove the -DVANISH_LIB in the .exp file.

> +
> +int main()

Put "int" on its own line.

> +{
> +  void *handle;
> +  int (*foo)(int);
> +  char *error;
> +
> +  /* Open library */
> +  handle = dlopen(VANISH_LIB, RTLD_NOW);
> +  if (!handle) {
> +    fprintf(stderr, "%s\n", dlerror());

Please make this file follow GNU style as well (space before parenthesis,
curly braces position, etc).

> +    exit(EXIT_FAILURE);
> +  }
> +
> +  /* Clear any existing error */
> +  dlerror();
> +
> +  /* Remove library */
> +  if (remove(VANISH_LIB) == -1) {

Can we rename the lib instead of deleting it?  It's always preferable to keep
test artifacts in case we need to inspect them.

> +    perror("remove " VANISH_LIB);
> +    exit(EXIT_FAILURE);
> +  }
> +
> +  /* Get function pointer */
> +  foo = dlsym(handle, "foo");
> +  error = dlerror();
> +  if (error != NULL) {
> +    fprintf(stderr, "%s\n", error);
> +    exit(EXIT_FAILURE);
> +  }
> +
> +  /* Call function */
> +  (*foo)(1);
> +
> +  /* Close and exit */
> +  dlclose(handle);
> +  exit(EXIT_SUCCESS);
> +}
> diff --git a/gdb/testsuite/gdb.base/solib-vanish.exp b/gdb/testsuite/gdb.base/solib-vanish.exp
> new file mode 100644
> index 0000000..de92d8d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/solib-vanish.exp
> @@ -0,0 +1,108 @@
> +# Copyright 2017 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# This test case verifies that GDB gracefully handles a shared library file
> +# vanishing after being dlopen'ed.  This consists of three nested function calls:
> +#
> +# main() -> foo() -> bar()
> +#
> +# where:
> +# - foo exists in solib-vanish-lib1.so, which is dlopen'ed by main()
> +# - bar exists in solib-vanish-lib2.so, which is dynamically linked into
> +#   solib-vanish-lib1.so
> +#
> +# Immediately after dlopen'ing solib-vanish-lib1.so, the so file is deleted.
> +# The main executable and solib-vanish-lib2.so are still accessible.
> +#
> +# If a breakpoint is set on bar(), gdb throws an error when this breakpoint is
> +# hit:
> +#
> +#   (gdb) r
> +#   Starting program: /local/gdb/git/pr_16577_repro/simple/solib-vanish-main
> +#
> +#   Breakpoint 1, BFD: reopening ./solib-vanish-lib1.so: No such file or directory
> +#
> +#   BFD: reopening ./solib-vanish-lib1.so: No such file or directory
> +#
> +#   (gdb)
> +#
> +# Notice that this does not print the current frame, i.e.:
> +#   bar (y=1) at solib-vanish-lib2.c:19
> +#   19	  return y + 1; /* break here */
> +#   (gdb)
> +#
> +# The current gdb git tip segfaults if we then try to step:
> +#   (gdb) n
> +#   Segmentation fault

Thanks for documenting the test case, I always like when test cases explain what they do!

> +# This test verifies that:
> +# 1) GDB does not segfault when stepping
> +# 2) The stack frame is printed
> +
> +if { [skip_shlib_tests] } {
> +    return 0
> +}
> +
> +# Library 2
> +set lib2name "solib-vanish-lib2"
> +set srcfile_lib2 ${srcdir}/${subdir}/${lib2name}.c
> +set binfile_lib2 [standard_output_file ${lib2name}.so]
> +set lib2_flags {debug}
> +
> +# Library 1
> +set lib1name "solib-vanish-lib1"
> +set srcfile_lib1 ${srcdir}/${subdir}/${lib1name}.c
> +set binfile_lib1 [standard_output_file ${lib1name}.so]
> +# I can't make this work for some reason:
> +#set lib1_flags [list debug shlib=${binfile_lib2}]
> +set lib1_flags [list debug ldflags=-l:${lib2name}.so ldflags=-Wl,-rpath=[standard_output_file ""] libdir=[standard_output_file ""]]

With

  https://sourceware.org/ml/gdb-patches/2017-10/msg00856.html

the commented out line should work.

> +# Main program
> +set testfile "solib-vanish-main"
> +set srcfile ${srcdir}/${subdir}/${testfile}.c
> +set executable ${testfile}
> +set binfile [standard_output_file ${executable}]
> +set bin_flags [list debug shlib_load additional_flags=-DVANISH_LIB=\"${binfile_lib1}\"]
> +
> +if { [gdb_compile_shlib ${srcfile_lib2} ${binfile_lib2} $lib2_flags] != ""
> +     || [gdb_compile_shlib ${srcfile_lib1} ${binfile_lib1} $lib1_flags] != ""
> +     || [gdb_compile ${srcfile} ${binfile} executable $bin_flags] != "" } {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +clean_restart $testfile
> +
> +if { ![runto_main] } {
> +    fail "can't run to main"
> +    return
> +}
> +
> +delete_breakpoints
> +
> +set lib2_lineno [gdb_get_line_number "break here" ${srcfile_lib2}]
> +
> +gdb_breakpoint "${lib2name}.c:${lib2_lineno}" {allow-pending}
> +
> +gdb_test "continue" \
> +    "Breakpoint .*at .*/${lib2name}.c:${lib2_lineno}.*break here.*" \
> +    "breakpoint prints location"

I think you could use the "gdb_continue_to_breakpoint" proc.

> +
> +# This should not segfault
> +gdb_test "next" \
> +    "" \
> +    "next succeeds"
> +
> 

Thanks,

Simon