This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC][PATCH] fix gdb segv when objfile can't be opened
On 10/27/2017 09:17 PM, Simon Marchi wrote:
> On 2017-10-23 07:19 PM, Mike Gulick wrote:
>
> ...
>
> 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).
>
Done.
> 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'm working on this. Hopefully it will be sooner rather than later.
> I noted a a few comments below (mostly minor/formatting stuff, overall I
> think this is a good quality submission).
> ...
>>
>> 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.
>
I rewrote the comment to be a full sentence
>> ...
>> +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);
> }
>
I reformatted all of the helper files for the test using 'indent'.
>> ...
>> +int bar (int y) {
>> + return y + 1; /* break here */
>> +}
>
> Same here.
>
>> ...
>> +#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.
>
Done.
>> +
>> +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.
>
I updated the test to rename the library instead of removing it. It also now renames it
back before exiting. This *should* make the test idempotent, unless it fails before it
is able to rename the .so back to its original name.
>> ...
>> +
>> +# 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.
>
This did indeed work. Thank you!
>> ...
>> +
>> +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 works as well, so the new patch uses this instead.
Here's an updated patch which hopefully addresses all of these items.
---
>From 1e1d408184ca0d204276665e317dc0c90db61f26 Mon Sep 17 00:00:00 2001
From: Mike Gulick <mgulick@mathworks.com>
Date: Mon, 30 Oct 2017 18:12:31 -0400
Subject: [PATCH v3] 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/ChangeLog:
PR gdb/16577
* gdb_bfd.c (gdb_bfd_map_section): If unable to read object file, issue
a warning instead of throwing an error, set section size to 0 and return
NULL.
* gdb_bfd.h (gdb_bfd_map_section): Update description.
gdb/testsuite/ChangeLog:
PR gdb/16577
* gdb.base/solib-vanish.exp: New testsuite to test breakpoint
handling and stepping when the shared library for one of the stack
frames is not accessible.
* gdb.base/solib-vanish-main.c: New.
* gdb.base/solib-vanish-lib1.c: New.
* gdb.base/solib-vanish-lib2.c: New.
---
gdb/ChangeLog | 8 +++
gdb/gdb_bfd.c | 12 +++-
gdb/gdb_bfd.h | 17 +++--
gdb/testsuite/ChangeLog | 10 +++
gdb/testsuite/gdb.base/solib-vanish-lib1.c | 24 +++++++
gdb/testsuite/gdb.base/solib-vanish-lib2.c | 22 ++++++
gdb/testsuite/gdb.base/solib-vanish-main.c | 75 ++++++++++++++++++++
gdb/testsuite/gdb.base/solib-vanish.exp | 107 +++++++++++++++++++++++++++++
8 files changed, 263 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/ChangeLog b/gdb/ChangeLog
index 5a680ed..64fed04 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2017-10-30 Mike Gulick <mgulick@mathworks.com>
+
+ PR gdb/16577
+ * gdb_bfd.c (gdb_bfd_map_section): If unable to read object file, issue
+ a warning instead of throwing an error, set section size to 0 and return
+ NULL.
+ * gdb_bfd.h (gdb_bfd_map_section): Update description.
+
2017-10-30 Simon Marchi <simon.marchi@ericsson.com>
* common/common-utils.h (in_inclusive_range): New function.
diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index cc02740..9b30ecd 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -702,9 +702,15 @@ 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));
+ /* Set size to 0 to prevent further attempts to read the invalid
+ section. */
+ *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/ChangeLog b/gdb/testsuite/ChangeLog
index 9d0f813..6552835 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,13 @@
+2017-10-30 Mike Gulick <mgulick@mathworks.com>
+
+ PR gdb/16577
+ * gdb.base/solib-vanish.exp: New testsuite to test breakpoint
+ handling and stepping when the shared library for one of the stack
+ frames is not accessible.
+ * gdb.base/solib-vanish-main.c: New.
+ * gdb.base/solib-vanish-lib1.c: New.
+ * gdb.base/solib-vanish-lib2.c: New.
+
2017-10-28 Maksim Dzabraev <dzabraew@gmail.com>
PR python/21213
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..ac04104
--- /dev/null
+++ b/gdb/testsuite/gdb.base/solib-vanish-lib1.c
@@ -0,0 +1,24 @@
+/* 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);
+}
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..0cf5395
--- /dev/null
+++ b/gdb/testsuite/gdb.base/solib-vanish-lib2.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/>. */
+
+int
+bar (int y)
+{
+ return y + 1; /* break 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..5c5af9a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/solib-vanish-main.c
@@ -0,0 +1,75 @@
+/* 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 <dlfcn.h>
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+int
+main ()
+{
+ void *handle;
+ int (*foo) (int);
+ char *error;
+ char *dest = VANISH_LIB ".renamed";
+
+ /* Open library. */
+ handle = dlopen (VANISH_LIB, RTLD_NOW);
+ if (!handle)
+ {
+ fprintf (stderr, "%s\n", dlerror ());
+ exit (EXIT_FAILURE);
+ }
+
+ /* Clear any existing error */
+ dlerror ();
+
+ /* Simulate deleting file by renaming it. */
+ if (rename (VANISH_LIB, dest) == -1)
+ {
+ error = strerror (errno);
+ fprintf (stderr, "rename %s -> %s: %s\n", VANISH_LIB, dest, error);
+ 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);
+
+ /* Put VANISH_LIB back where we found it. */
+ if (rename (dest, VANISH_LIB) == -1)
+ {
+ error = strerror (errno);
+ fprintf (stderr, "rename %s -> %s: %s\n", dest, VANISH_LIB, error);
+ exit (EXIT_FAILURE);
+ }
+
+ 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..5214e50
--- /dev/null
+++ b/gdb/testsuite/gdb.base/solib-vanish.exp
@@ -0,0 +1,107 @@
+# 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 moved aside
+# by renaming. 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
+
+# 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]
+set lib1_flags [list debug shlib=${binfile_lib2}]
+
+# 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}
+
+# Verify that both the location and source code are displayed
+gdb_continue_to_breakpoint "bar" \
+ ".*/${lib2name}.c:${lib2_lineno}.*break here.*"
+
+# This should not segfault
+gdb_test "next" \
+ "" \
+ "next succeeds"
+
--
2.1.4