Re: [backtrace] Avoid segfault
On Tue, 26 Feb 2019, Tom de Vries wrote: >>> Specifically I am now seeing >>> >>> gmake[4]: *** No rule to make target 'b3test_dwz_buildid', >>> needed by 'b3test_dwz_buildid.log'. > The only way of reproducing it was to deinstall dwz. : > Fixed in patch below, committed as trivial. Great, thanks for the quick follow-up, Tom! I did a couple of further tests and am happy to report that the testsuite of libbacktrace is now completely "clean". I'll keep my eyes open. ;-) Gerald
Re: [backtrace] Avoid segfault
On 25-02-19 21:03, Tom de Vries wrote: > On 25-02-19 15:12, Gerald Pfeifer wrote: >> Specifically I am now seeing >> >> gmake[4]: *** No rule to make target 'b3test_dwz_buildid', >> needed by 'b3test_dwz_buildid.log'. >> >> in my build/test logs. (Note, this is GNU make 4.2.1, so might reproduce >> on your SUSE systems as well?) > > Hi Gerald, > > I managed to reproduce this by adding: > ... > mkdir bin > ( > cd bin > ln -s $(which false) dwz > ) > export PATH=$(pwd -P)/bin:$PATH > ... > to my libbacktrace test script. > Hmm, I realized that linking dwz to /bin/false set HAVE_DWZ to 1, so I did in fact not reproduce the failure this way. The only way of reproducing it was to deinstall dwz. > The problem is that: > ... > TESTS += b3test_dwz_buildid > ... > is guarded by HAVE_ELF and HAVE_OBJCOPY_DEBUGLINK, but not by HAVE_DWZ. > Fixed in patch below, committed as trivial. Thanks, - Tom [libbacktrace] Require dwz for b3test_dwz_buildid If dwz is not available, then the libbacktrace test b3test_dwz_buildid fails like this: ... gmake[4]: *** No rule to make target 'b3test_dwz_buildid' ... Fix this by guarding the test with HAVE_DWZ. Tested on x86_64 with and without dwz installed. 2019-02-26 Tom de Vries * Makefile.am (TESTS): Only add b3test_dwz_buildid if HAVE_DWZ. * Makefile.in: Regenerate. --- libbacktrace/Makefile.am | 4 +++ libbacktrace/Makefile.in | 72 +--- 2 files changed, 41 insertions(+), 35 deletions(-) diff --git a/libbacktrace/Makefile.am b/libbacktrace/Makefile.am index 3b5f6e374d8..7ddee4962ec 100644 --- a/libbacktrace/Makefile.am +++ b/libbacktrace/Makefile.am @@ -205,6 +205,8 @@ b2test_LDADD = libbacktrace_elf_for_test.la check_PROGRAMS += b2test TESTS += b2test_buildid +if HAVE_DWZ + b3test_SOURCES = $(btest_SOURCES) b3test_CFLAGS = $(btest_CFLAGS) b3test_LDFLAGS = -Wl,--build-id @@ -213,6 +215,8 @@ b3test_LDADD = libbacktrace_elf_for_test.la check_PROGRAMS += b3test TESTS += b3test_dwz_buildid +endif HAVE_DWZ + endif HAVE_OBJCOPY_DEBUGLINK endif HAVE_ELF diff --git a/libbacktrace/Makefile.in b/libbacktrace/Makefile.in index f60aca6463a..a896a26dff8 100644 --- a/libbacktrace/Makefile.in +++ b/libbacktrace/Makefile.in @@ -120,29 +120,31 @@ POST_UNINSTALL = : build_triplet = @build@ host_triplet = @host@ target_triplet = @target@ -check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_8) -TESTS = $(am__append_4) $(am__append_6) $(am__append_9) \ - $(am__append_10) $(am__append_14) $(am__EXEEXT_8) +check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) \ + $(am__EXEEXT_9) +TESTS = $(am__append_4) $(am__append_6) $(am__append_8) \ + $(am__append_11) $(am__append_12) $(am__append_16) \ + $(am__EXEEXT_9) @HAVE_ELF_TRUE@@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_1 = libbacktrace_elf_for_test.la @NATIVE_TRUE@am__append_2 = test_elf test_xcoff_32 test_xcoff_64 \ @NATIVE_TRUE@ test_pecoff test_unknown unittest unittest_alloc \ @NATIVE_TRUE@ btest @NATIVE_TRUE@am__append_3 = allocfail @NATIVE_TRUE@am__append_4 = allocfail.sh -@HAVE_ELF_TRUE@@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_5 = b2test \ -@HAVE_ELF_TRUE@@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@ b3test -@HAVE_ELF_TRUE@@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_6 = b2test_buildid \ -@HAVE_ELF_TRUE@@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@ b3test_dwz_buildid -@HAVE_ELF_TRUE@@NATIVE_TRUE@am__append_7 = btest_lto -@NATIVE_TRUE@am__append_8 = btest_alloc stest stest_alloc ztest \ +@HAVE_ELF_TRUE@@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_5 = b2test +@HAVE_ELF_TRUE@@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_6 = b2test_buildid +@HAVE_DWZ_TRUE@@HAVE_ELF_TRUE@@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_7 = b3test +@HAVE_DWZ_TRUE@@HAVE_ELF_TRUE@@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_8 = b3test_dwz_buildid +@HAVE_ELF_TRUE@@NATIVE_TRUE@am__append_9 = btest_lto +@NATIVE_TRUE@am__append_10 = btest_alloc stest stest_alloc ztest \ @NATIVE_TRUE@ ztest_alloc edtest edtest_alloc -@HAVE_DWZ_TRUE@@NATIVE_TRUE@am__append_9 = btest_dwz -@HAVE_DWZ_TRUE@@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_10 = btest_dwz_gnudebuglink -@HAVE_ZLIB_TRUE@@NATIVE_TRUE@am__append_11 = -lz -@HAVE_ZLIB_TRUE@@NATIVE_TRUE@am__append_12 = -lz -@HAVE_PTHREAD_TRUE@@NATIVE_TRUE@am__append_13 = ttest ttest_alloc -@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_14 = btest_gnudebuglink -@HAVE_COMPRESSED_DEBUG_TRUE@@NATIVE_TRUE@am__append_15 = ctestg ctesta \ +@HAVE_DWZ_TRUE@@NATIVE_TRUE@am__append_11 = btest_dwz +@HAVE_DWZ_TRUE@@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_12 = btest_dwz_gnudebuglink +@HAVE_ZLIB_TRUE@@NATIVE_TRUE@am__append_13 = -lz +@HAVE_ZLIB_TRUE@@NATIVE_TRUE@am__append_14 = -lz +@HAVE_PTHREAD_TRUE@@NATIVE_TRUE@am__append_15 = ttest ttest_alloc +@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_16 = btest_gnudebuglink
Re: [backtrace] Avoid segfault
On 25-02-19 15:12, Gerald Pfeifer wrote: > Specifically I am now seeing > > gmake[4]: *** No rule to make target 'b3test_dwz_buildid', > needed by 'b3test_dwz_buildid.log'. > > in my build/test logs. (Note, this is GNU make 4.2.1, so might reproduce > on your SUSE systems as well?) Hi Gerald, I managed to reproduce this by adding: ... mkdir bin ( cd bin ln -s $(which false) dwz ) export PATH=$(pwd -P)/bin:$PATH ... to my libbacktrace test script. The problem is that: ... TESTS += b3test_dwz_buildid ... is guarded by HAVE_ELF and HAVE_OBJCOPY_DEBUGLINK, but not by HAVE_DWZ. Thanks, - Tom
Re: [backtrace] Avoid segfault
Hi Tom, I'm afraid this triggers on my (FreeBSD-based) testers: 2019-01-29 Tom de Vries * install-debuginfo-for-buildid.sh.in: New script. * Makefile.am (check_PROGRAMS): Add b2test and b3test. (TESTS): Add b2test_buildid and b3test_dwz_buildid. * Makefile.in: Regenerate. * configure.ac (HAVE_ELF): Set with AM_CONDITIONAL. (READELF): Set with AC_CHECK_PROG. (install-debuginfo-for-buildid.sh): Generate with AC_CONFIG_FILES. * configure: Regenerate. * elf.c (SYSTEM_BUILD_ID_DIR): Factor out of ... (elf_open_debugfile_by_buildid): ... here. Specifically I am now seeing gmake[4]: *** No rule to make target 'b3test_dwz_buildid', needed by 'b3test_dwz_buildid.log'. in my build/test logs. (Note, this is GNU make 4.2.1, so might reproduce on your SUSE systems as well?) Gerald
Re: [backtrace] Avoid segfault
On Tue, Jan 29, 2019 at 3:17 PM Segher Boessenkool wrote: > > On Sun, Jan 27, 2019 at 01:53:18PM -0800, Ian Lance Taylor wrote: > > You need to use a temporary file, such as $@.tmp, for the final sed > > command, followed by a mv to $@. Otherwise a failure in the sed will > > leave what appears to be an up to date file. > > Or you just set .DELETE_ON_ERROR, we require GNU make after all so might > as well use it! It's useful, but it doesn't help if the file is partially written on disk and then the computer crashes. It also doesn't help if the program is not careful to check for errors on writing to standard output--many programs aren't--and the disk fills up after writing out part of the file. Yes, both cases have happened to me when I was a release engineer aeons ago, so I try to be careful. Ian
Re: [backtrace] Avoid segfault
On Sun, Jan 27, 2019 at 01:53:18PM -0800, Ian Lance Taylor wrote: > You need to use a temporary file, such as $@.tmp, for the final sed > command, followed by a mv to $@. Otherwise a failure in the sed will > leave what appears to be an up to date file. Or you just set .DELETE_ON_ERROR, we require GNU make after all so might as well use it! Segher
Re: [backtrace] Avoid segfault
On Tue, Jan 29, 2019 at 12:24 AM Tom de Vries wrote: > > On 27-01-19 22:53, Ian Lance Taylor wrote: > > On Sun, Jan 27, 2019 at 1:16 PM Tom de Vries wrote: > >> > >> On 25-01-19 18:15, Nathan Sidwell wrote: > >>> On 1/25/19 5:28 AM, Tom de Vries wrote: > > This patch fixes it by passing "" instead of NULL, in the call to > elf_add at line 3083 (for .gnu_debugaltlink), not the call to elf_add at > line 3044 (for .gnu_debuglink) mentioned above. > > Nathan, does this fix the problem for you? If not, can you provide a > reproducer, or give a hint on how one could be constructed? > >>> > >>> I still hit the problem, and am installing this as sufficiently obvious. > >>> I'm on a fedora system debugging pr88995. The debuglink_name is > >>> "../../.dwz/isl-0.16.1-7.fc29.x86_64" > >>> > >> > >> I've managed to reproduce this segfault instance by adding a test-case > >> that uses both build-id and dwz. > >> > >> OK for trunk? > > > >> +elf_for_test.c: elf.c > >> + PWD=$$(pwd -P); \ > >> + BUILD_ID_DIR="usr/lib/debug/.build-id/"; \ > >> + SEARCH='#define SYSTEM_BUILD_ID_DIR'; \ > >> + REPLACE="#define SYSTEM_BUILD_ID_DIR \"$$PWD/$$BUILD_ID_DIR\""; \ > >> + $(SED) "s%^$$SEARCH.*\$$%$$REPLACE%" \ > >> + $< \ > >> + > $@ > > > > You need to use a temporary file, such as $@.tmp, for the final sed > > command, followed by a mv to $@. Otherwise a failure in the sed will > > leave what appears to be an up to date file. > > > > Done. > > Also, I've factored out a script install-debuginfo-for-buildid.sh, > hoping this will make things more readable/maintainable. > > > Honestly I'm not sure this patch is worth doing. It adds a lot of > > complex mechanism in order to test a patch that is fairly obvious. > > Agreed, the patch is fairly obvious. > > But at the moment, there's no test-case that exercises the build-id > support in libbacktrace. IMO, that alone would be a good reason to add > this test-case. > > > While it's good practice to add a test for every change, it's not good > > practice for the testsuite to become so complex that it becomes in > > itself difficult to maintain. > > Understood. > > Please let me know if this is acceptable, or if I can do anything that > would make this easier to maintain. I guess this is OK. Please add a copyright header to the new shell script. Also please add a comment explaining what it does. Thanks. Ian
Re: [backtrace] Avoid segfault
On 27-01-19 22:53, Ian Lance Taylor wrote: > On Sun, Jan 27, 2019 at 1:16 PM Tom de Vries wrote: >> >> On 25-01-19 18:15, Nathan Sidwell wrote: >>> On 1/25/19 5:28 AM, Tom de Vries wrote: >>>> >>>> This patch fixes it by passing "" instead of NULL, in the call to >>>> elf_add at line 3083 (for .gnu_debugaltlink), not the call to elf_add at >>>> line 3044 (for .gnu_debuglink) mentioned above. >>>> >>>> Nathan, does this fix the problem for you? If not, can you provide a >>>> reproducer, or give a hint on how one could be constructed? >>> >>> I still hit the problem, and am installing this as sufficiently obvious. >>> I'm on a fedora system debugging pr88995. The debuglink_name is >>> "../../.dwz/isl-0.16.1-7.fc29.x86_64" >>> >> >> I've managed to reproduce this segfault instance by adding a test-case >> that uses both build-id and dwz. >> >> OK for trunk? > >> +elf_for_test.c: elf.c >> + PWD=$$(pwd -P); \ >> + BUILD_ID_DIR="usr/lib/debug/.build-id/"; \ >> + SEARCH='#define SYSTEM_BUILD_ID_DIR'; \ >> + REPLACE="#define SYSTEM_BUILD_ID_DIR \"$$PWD/$$BUILD_ID_DIR\""; \ >> + $(SED) "s%^$$SEARCH.*\$$%$$REPLACE%" \ >> + $< \ >> + > $@ > > You need to use a temporary file, such as $@.tmp, for the final sed > command, followed by a mv to $@. Otherwise a failure in the sed will > leave what appears to be an up to date file. > Done. Also, I've factored out a script install-debuginfo-for-buildid.sh, hoping this will make things more readable/maintainable. > Honestly I'm not sure this patch is worth doing. It adds a lot of > complex mechanism in order to test a patch that is fairly obvious. Agreed, the patch is fairly obvious. But at the moment, there's no test-case that exercises the build-id support in libbacktrace. IMO, that alone would be a good reason to add this test-case. > While it's good practice to add a test for every change, it's not good > practice for the testsuite to become so complex that it becomes in > itself difficult to maintain. Understood. Please let me know if this is acceptable, or if I can do anything that would make this easier to maintain. Thanks, - Tom [libbacktrace] Add test-cases exercising build-id and dwz Add test-cases b2test_buildid and b3test_dwz_buildid. The last one triggers the segfault fixed by "[backtrace] Avoid segfault" ( r268275 ). 2019-01-27 Tom de Vries * install-debuginfo-for-buildid.sh.in: New script. * Makefile.am (check_PROGRAMS): Add b2test and b3test. (TESTS): Add b2test_buildid and b3test_dwz_buildid. * Makefile.in: Regenerate. * configure.ac (HAVE_ELF): Set with AM_CONDITIONAL. (READELF): Set with AC_CHECK_PROG. (install-debuginfo-for-buildid.sh): Generate with AC_CONFIG_FILES. * configure: Regenerate. * elf.c (SYSTEM_BUILD_ID_DIR): Factor out of ... (elf_open_debugfile_by_buildid): ... here. --- libbacktrace/Makefile.am | 50 ++ libbacktrace/Makefile.in | 193 ++- libbacktrace/configure | 60 ++- libbacktrace/configure.ac| 3 + libbacktrace/elf.c | 4 +- libbacktrace/install-debuginfo-for-buildid.sh.in | 27 6 files changed, 294 insertions(+), 43 deletions(-) diff --git a/libbacktrace/Makefile.am b/libbacktrace/Makefile.am index 1c4ab07aa19..71a2ed478cc 100644 --- a/libbacktrace/Makefile.am +++ b/libbacktrace/Makefile.am @@ -108,6 +108,28 @@ libbacktrace_noformat_la_LIBADD = $(BACKTRACE_FILE) $(VIEW_FILE) $(ALLOC_FILE) libbacktrace_noformat_la_DEPENDENCIES = $(libbacktrace_noformat_la_LIBADD) +if HAVE_ELF +if HAVE_OBJCOPY_DEBUGLINK + +TEST_BUILD_ID_DIR=$(abs_builddir)/usr/lib/debug/.build-id/ + +check_LTLIBRARIES += libbacktrace_elf_for_test.la + +libbacktrace_elf_for_test_la_SOURCES = $(libbacktrace_la_SOURCES) +libbacktrace_elf_for_test_la_LIBADD = $(BACKTRACE_FILE) elf_for_test.lo \ + $(VIEW_FILE) $(ALLOC_FILE) + +elf_for_test.c: elf.c + SEARCH='^#define SYSTEM_BUILD_ID_DIR.*$$'; \ + REPLACE="#define SYSTEM_BUILD_ID_DIR \"$(TEST_BUILD_ID_DIR)\""; \ + $(SED) "s%$$SEARCH%$$REPLACE%" \ + $< \ + > $@.tmp + mv $@.tmp $@ + +endif HAVE_OBJCOPY_DEBUGLINK +endif HAVE_ELF + xcoff_%.c: xcoff.c SEARCH='#error "Unknown BACKTRACE_XCOFF_SIZE"'; \ REPLACE='#undef BACKTRACE_XCOFF_SIZE\ @@ -172,6 +194,28 @@ allocfail.sh: allocfail TESTS += allocfail.sh +if HAVE_ELF +if HAVE_OBJCOPY_DEBUGLINK + +b2test_SOURCES = $(btest_SOURCES) +b2test_CFLAGS = $(btest_CFLAGS) +b2test_LDFLAGS = -Wl,--build-id +b2test_LDADD = libbacktrace_elf_for_test.la + +check_PROGRAMS += b2test +TESTS += b2
Re: [backtrace] Avoid segfault
On Sun, Jan 27, 2019 at 1:16 PM Tom de Vries wrote: > > On 25-01-19 18:15, Nathan Sidwell wrote: > > On 1/25/19 5:28 AM, Tom de Vries wrote: > >> > >> This patch fixes it by passing "" instead of NULL, in the call to > >> elf_add at line 3083 (for .gnu_debugaltlink), not the call to elf_add at > >> line 3044 (for .gnu_debuglink) mentioned above. > >> > >> Nathan, does this fix the problem for you? If not, can you provide a > >> reproducer, or give a hint on how one could be constructed? > > > > I still hit the problem, and am installing this as sufficiently obvious. > > I'm on a fedora system debugging pr88995. The debuglink_name is > > "../../.dwz/isl-0.16.1-7.fc29.x86_64" > > > > I've managed to reproduce this segfault instance by adding a test-case > that uses both build-id and dwz. > > OK for trunk? > +elf_for_test.c: elf.c > + PWD=$$(pwd -P); \ > + BUILD_ID_DIR="usr/lib/debug/.build-id/"; \ > + SEARCH='#define SYSTEM_BUILD_ID_DIR'; \ > + REPLACE="#define SYSTEM_BUILD_ID_DIR \"$$PWD/$$BUILD_ID_DIR\""; \ > + $(SED) "s%^$$SEARCH.*\$$%$$REPLACE%" \ > + $< \ > + > $@ You need to use a temporary file, such as $@.tmp, for the final sed command, followed by a mv to $@. Otherwise a failure in the sed will leave what appears to be an up to date file. Honestly I'm not sure this patch is worth doing. It adds a lot of complex mechanism in order to test a patch that is fairly obvious. While it's good practice to add a test for every change, it's not good practice for the testsuite to become so complex that it becomes in itself difficult to maintain. Ian
Re: [backtrace] Avoid segfault
On 25-01-19 18:15, Nathan Sidwell wrote:
> On 1/25/19 5:28 AM, Tom de Vries wrote:
>>
>> This patch fixes it by passing "" instead of NULL, in the call to
>> elf_add at line 3083 (for .gnu_debugaltlink), not the call to elf_add at
>> line 3044 (for .gnu_debuglink) mentioned above.
>>
>> Nathan, does this fix the problem for you? If not, can you provide a
>> reproducer, or give a hint on how one could be constructed?
>
> I still hit the problem, and am installing this as sufficiently obvious.
> I'm on a fedora system debugging pr88995. The debuglink_name is
> "../../.dwz/isl-0.16.1-7.fc29.x86_64"
>
I've managed to reproduce this segfault instance by adding a test-case
that uses both build-id and dwz.
OK for trunk?
Thanks,
- Tom
[libbacktrace] Add test-cases exercising build-id and dwz
Add test-cases b2test_buildid and b3test_dwz_buildid.
The last one triggers the segfault fixed by "[backtrace] Avoid segfault"
( r268275 ).
2019-01-27 Tom de Vries
* Makefile.am (check_PROGRAMS): Add b2test and b3test.
(TESTS): Add b2test_buildid, b3test_dwz and b3test_dwz_buildid.
* Makefile.in: Regenerate.
* configure.ac (HAVE_ELF): Set with AM_CONDITIONAL.
* configure: Regenerate.
* elf.c (SYSTEM_BUILD_ID_DIR): Factor out of ...
(elf_open_debugfile_by_buildid): ... here.
---
libbacktrace/Makefile.am | 53 +++
libbacktrace/Makefile.in | 223 ++
libbacktrace/configure| 18 +++-
libbacktrace/configure.ac | 1 +
libbacktrace/elf.c| 4 +-
5 files changed, 257 insertions(+), 42 deletions(-)
diff --git a/libbacktrace/Makefile.am b/libbacktrace/Makefile.am
index 997a535dff4..becba1ae1f0 100644
--- a/libbacktrace/Makefile.am
+++ b/libbacktrace/Makefile.am
@@ -103,6 +103,25 @@ libbacktrace_noformat_la_LIBADD = $(BACKTRACE_FILE) $(VIEW_FILE) $(ALLOC_FILE)
libbacktrace_noformat_la_DEPENDENCIES = $(libbacktrace_noformat_la_LIBADD)
+if HAVE_ELF
+
+check_LTLIBRARIES += libbacktrace_elf_for_test.la
+
+libbacktrace_elf_for_test_la_SOURCES = $(libbacktrace_la_SOURCES)
+libbacktrace_elf_for_test_la_LIBADD = $(BACKTRACE_FILE) elf_for_test.lo \
+ $(VIEW_FILE) $(ALLOC_FILE)
+
+elf_for_test.c: elf.c
+ PWD=$$(pwd -P); \
+ BUILD_ID_DIR="usr/lib/debug/.build-id/"; \
+ SEARCH='#define SYSTEM_BUILD_ID_DIR'; \
+ REPLACE="#define SYSTEM_BUILD_ID_DIR \"$$PWD/$$BUILD_ID_DIR\""; \
+ $(SED) "s%^$$SEARCH.*\$$%$$REPLACE%" \
+ $< \
+ > $@
+
+endif HAVE_ELF
+
xcoff_%.c: xcoff.c
SEARCH='#error "Unknown BACKTRACE_XCOFF_SIZE"'; \
REPLACE='#undef BACKTRACE_XCOFF_SIZE\
@@ -166,6 +185,30 @@ allocfail.sh: allocfail
TESTS += allocfail.sh
+if HAVE_ELF
+
+b2test_SOURCES = $(btest_SOURCES)
+b2test_CFLAGS = $(btest_CFLAGS)
+b2test_LDADD = libbacktrace_elf_for_test.la
+
+# The file b2test_buildid is an objcopy of b2test, so these programs share the
+# same build-id. The first time b2test is run, there's no corresponding debug
+# file at ./usr/lib/debug/.build-id/aa/bb..zz.debug. The second time b2test is
+# run, the .debug file has been created for b2test_buildid, which is now picked
+# up by b2test as well.
+check_PROGRAMS += b2test
+TESTS += b2test_buildid
+
+b3test_SOURCES = $(btest_SOURCES)
+b3test_CFLAGS = $(btest_CFLAGS)
+b3test_LDADD = libbacktrace_elf_for_test.la
+
+# These programs share the same build-id. See b2test for more details.
+check_PROGRAMS += b3test
+TESTS += b3test_dwz b3test_dwz_buildid
+
+endif HAVE_ELF
+
btest_SOURCES = btest.c testlib.c
btest_CFLAGS = $(AM_CFLAGS) -g -O
btest_LDADD = libbacktrace.la
@@ -269,6 +312,16 @@ TESTS += btest_gnudebuglink
endif HAVE_OBJCOPY_DEBUGLINK
+%_buildid: %
+ buildid=$$(readelf -n $< | grep "Build ID" | $(AWK) '{print $$3}'); \
+ prefix=$$(echo $$buildid | $(SED) 's/.//3g'); \
+ remainder=$$(echo $$buildid | $(SED) 's/^.\{2\}//'); \
+ pwd=$$(pwd -P); \
+ dir=$$pwd/usr/lib/debug/.build-id; \
+ mkdir -p $$dir/$$prefix; \
+ $(OBJCOPY) --only-keep-debug $< $$dir/$$prefix/$$remainder.debug
+ $(OBJCOPY) --strip-debug $< $@
+
if HAVE_COMPRESSED_DEBUG
ctestg_SOURCES = btest.c testlib.c
diff --git a/libbacktrace/Makefile.in b/libbacktrace/Makefile.in
index f04577066f8..1d84b67d0d5 100644
--- a/libbacktrace/Makefile.in
+++ b/libbacktrace/Makefile.in
@@ -120,19 +120,33 @@ POST_UNINSTALL = :
build_triplet = @build@
host_triplet = @host@
target_triplet = @target@
-check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3)
-@NATIVE_TRUE@am__append_1 = test_elf test_xcoff_32 test_xcoff_64 \
+check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) \
+ $(am__EXEEXT_4) $(am__EXEEXT_5)
+@HAVE_ELF_TRUE@@NATIVE_TRUE@am__append_1 = libbacktrace_elf_for_test.la
+@NATIVE_TRUE@am__append_2 = test_elf test_xcoff_32 test_xcoff_64 \
@NATIVE_TRUE@ test_pecoff test_unknown unittest unittest_alloc \
-@NATIVE_TRUE@ allocf
Re: [backtrace] Avoid segfault
On Fri, Jan 25, 2019 at 12:15:28PM -0500, Nathan Sidwell wrote: > On 1/25/19 5:28 AM, Tom de Vries wrote: > > > > This patch fixes it by passing "" instead of NULL, in the call to > > elf_add at line 3083 (for .gnu_debugaltlink), not the call to elf_add at > > line 3044 (for .gnu_debuglink) mentioned above. > > > > Nathan, does this fix the problem for you? If not, can you provide a > > reproducer, or give a hint on how one could be constructed? > > I still hit the problem, and am installing this as sufficiently obvious. > I'm on a fedora system debugging pr88995. The debuglink_name is > "../../.dwz/isl-0.16.1-7.fc29.x86_64" > > I'm not sure why this is triggering now -- maybe my debuginfo packages are > out of date? I'm seeing this too. I've resolved it by uninstalling libmpc-debuginfo. If I put it back, the crash in libbacktrace reappears. Again the problem was a null filename and so strchr crashed. Thanks for looking into this Nathan. Marek
Re: [backtrace] Avoid segfault
On 1/25/19 5:28 AM, Tom de Vries wrote: This patch fixes it by passing "" instead of NULL, in the call to elf_add at line 3083 (for .gnu_debugaltlink), not the call to elf_add at line 3044 (for .gnu_debuglink) mentioned above. Nathan, does this fix the problem for you? If not, can you provide a reproducer, or give a hint on how one could be constructed? I still hit the problem, and am installing this as sufficiently obvious. I'm on a fedora system debugging pr88995. The debuglink_name is "../../.dwz/isl-0.16.1-7.fc29.x86_64" I'm not sure why this is triggering now -- maybe my debuginfo packages are out of date? nathan -- Nathan Sidwell 2019-01-25 Nathan Sidwell * elf.c (elf_add): Pass "" filename to recursive call with separated debug. Index: elf.c === --- elf.c (revision 268272) +++ elf.c (working copy) @@ -3041,7 +3041,7 @@ elf_add (struct backtrace_state *state, if (debugaltlink_view_valid) backtrace_release_view (state, _view, error_callback, data); - ret = elf_add (state, NULL, d, base_address, error_callback, data, + ret = elf_add (state, "", d, base_address, error_callback, data, fileline_fn, found_sym, found_dwarf, NULL, 0, 1, NULL, 0); if (ret < 0)
Re: [backtrace] Avoid segfault
On Fri, Jan 25, 2019 at 5:27 AM Tom de Vries wrote: > > On 25-01-19 14:17, Tom de Vries wrote: > > On 25-01-19 01:51, Ian Lance Taylor wrote: > >> On Thu, Jan 24, 2019 at 4:11 PM Nathan Sidwell wrote: > >>> > >>> I just tripped over a segfault in libbacktrace. We apply strrchr to a > >>> possibly NULL filename, with predictable results when it is. > >>> > >>> elf.c:3044 passes NULL as the filename parm: > >>> ret = elf_add (state, NULL, d, base_address, error_callback, > >>> data, > >>> fileline_fn, found_sym, found_dwarf, NULL, 0, 1, > >>> NULL, > >>> 0); > >>> > >>> This gets to elf_open_debugfile_by_debuglink which passes it on through: > >>>ddescriptor = elf_find_debugfile_by_debuglink (state, filename, > >>> debuglink_name, > >>> error_callback, data); > >>> > >>> this patch avoids the strrchr when filename is null. I reordered the > >>> way prefix & prefix len got set, finding it prefereable to: > >>>slash = filename ? NULL : strrchr (filename, '/'); > >>> but if you prefer to avoid the assignment in the conditional I'm fine > >>> with that too. > >> > >> Yeah, please don't do an assignment in a conditional. > >> > >> Why don't we just pass "" instead of NULL in the call to elf_add? If > >> that works, that is OK. > >> > > > > With this refactoring preamble ... > > ... I can more easily add a test-case btest_dwz_gnudebuglink, which > triggers a segfault due to strrchr being called with a NULL string. > > This patch fixes it by passing "" instead of NULL, in the call to > elf_add at line 3083 (for .gnu_debugaltlink), not the call to elf_add at > line 3044 (for .gnu_debuglink) mentioned above. > > Nathan, does this fix the problem for you? If not, can you provide a > reproducer, or give a hint on how one could be constructed? This is OK. s/Call/call/ in the ChangeLog entry. Thanks. Ian
Re: [backtrace] Avoid segfault
On Fri, Jan 25, 2019 at 5:17 AM Tom de Vries wrote: > > On 25-01-19 01:51, Ian Lance Taylor wrote: > > On Thu, Jan 24, 2019 at 4:11 PM Nathan Sidwell wrote: > >> > >> I just tripped over a segfault in libbacktrace. We apply strrchr to a > >> possibly NULL filename, with predictable results when it is. > >> > >> elf.c:3044 passes NULL as the filename parm: > >> ret = elf_add (state, NULL, d, base_address, error_callback, > >> data, > >> fileline_fn, found_sym, found_dwarf, NULL, 0, 1, > >> NULL, > >> 0); > >> > >> This gets to elf_open_debugfile_by_debuglink which passes it on through: > >>ddescriptor = elf_find_debugfile_by_debuglink (state, filename, > >> debuglink_name, > >> error_callback, data); > >> > >> this patch avoids the strrchr when filename is null. I reordered the > >> way prefix & prefix len got set, finding it prefereable to: > >>slash = filename ? NULL : strrchr (filename, '/'); > >> but if you prefer to avoid the assignment in the conditional I'm fine > >> with that too. > > > > Yeah, please don't do an assignment in a conditional. > > > > Why don't we just pass "" instead of NULL in the call to elf_add? If > > that works, that is OK. > > > > With this refactoring preamble ... This is OK. Thanks. Ian
Re: [backtrace] Avoid segfault
On 25-01-19 14:17, Tom de Vries wrote: > On 25-01-19 01:51, Ian Lance Taylor wrote: >> On Thu, Jan 24, 2019 at 4:11 PM Nathan Sidwell wrote: >>> >>> I just tripped over a segfault in libbacktrace. We apply strrchr to a >>> possibly NULL filename, with predictable results when it is. >>> >>> elf.c:3044 passes NULL as the filename parm: >>> ret = elf_add (state, NULL, d, base_address, error_callback, data, >>> fileline_fn, found_sym, found_dwarf, NULL, 0, 1, >>> NULL, >>> 0); >>> >>> This gets to elf_open_debugfile_by_debuglink which passes it on through: >>>ddescriptor = elf_find_debugfile_by_debuglink (state, filename, >>> debuglink_name, >>> error_callback, data); >>> >>> this patch avoids the strrchr when filename is null. I reordered the >>> way prefix & prefix len got set, finding it prefereable to: >>>slash = filename ? NULL : strrchr (filename, '/'); >>> but if you prefer to avoid the assignment in the conditional I'm fine >>> with that too. >> >> Yeah, please don't do an assignment in a conditional. >> >> Why don't we just pass "" instead of NULL in the call to elf_add? If >> that works, that is OK. >> > > With this refactoring preamble ... ... I can more easily add a test-case btest_dwz_gnudebuglink, which triggers a segfault due to strrchr being called with a NULL string. This patch fixes it by passing "" instead of NULL, in the call to elf_add at line 3083 (for .gnu_debugaltlink), not the call to elf_add at line 3044 (for .gnu_debuglink) mentioned above. Nathan, does this fix the problem for you? If not, can you provide a reproducer, or give a hint on how one could be constructed? Thanks, - Tom [libbacktrace] Fix strrchr segfault Currently, when handling a libbacktrace testcase t with .gnu_debuglink to t.debug, and t.debug having a .gnu_debugaltlink to t.alt.debug, an segfault is triggered in when calling strrchr with a NULL string from elf_find_debugfile_by_debuglink. The NULL string originates from the elf_add called for the .gnu_debugaltlink, which uses NULL as filename argument. Fix this by using "" as filename argument instead. 2019-01-25 Tom de Vries * elf.c (elf_add): When handling .gnu_debugaltlink, Call elf_add with filename == "". * Makefile.am (TESTS): Add btest_dwz_gnudebuglink. * Makefile.in: Regenerate. --- libbacktrace/Makefile.am | 6 ++ libbacktrace/Makefile.in | 22 +++--- libbacktrace/elf.c | 2 +- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/libbacktrace/Makefile.am b/libbacktrace/Makefile.am index 2f1d9af89c3..997a535dff4 100644 --- a/libbacktrace/Makefile.am +++ b/libbacktrace/Makefile.am @@ -190,6 +190,12 @@ if HAVE_DWZ TESTS += btest_dwz +if HAVE_OBJCOPY_DEBUGLINK + +TESTS += btest_dwz_gnudebuglink + +endif HAVE_OBJCOPY_DEBUGLINK + endif HAVE_DWZ stest_SOURCES = stest.c diff --git a/libbacktrace/Makefile.in b/libbacktrace/Makefile.in index 0b73e3d6981..f04577066f8 100644 --- a/libbacktrace/Makefile.in +++ b/libbacktrace/Makefile.in @@ -127,11 +127,12 @@ check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) @NATIVE_TRUE@ ztest ztest_alloc edtest edtest_alloc @NATIVE_TRUE@am__append_2 = allocfail.sh @HAVE_DWZ_TRUE@@NATIVE_TRUE@am__append_3 = btest_dwz -@HAVE_ZLIB_TRUE@@NATIVE_TRUE@am__append_4 = -lz +@HAVE_DWZ_TRUE@@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_4 = btest_dwz_gnudebuglink @HAVE_ZLIB_TRUE@@NATIVE_TRUE@am__append_5 = -lz -@HAVE_PTHREAD_TRUE@@NATIVE_TRUE@am__append_6 = ttest ttest_alloc -@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_7 = btest_gnudebuglink -@HAVE_COMPRESSED_DEBUG_TRUE@@NATIVE_TRUE@am__append_8 = ctestg ctesta \ +@HAVE_ZLIB_TRUE@@NATIVE_TRUE@am__append_6 = -lz +@HAVE_PTHREAD_TRUE@@NATIVE_TRUE@am__append_7 = ttest ttest_alloc +@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_8 = btest_gnudebuglink +@HAVE_COMPRESSED_DEBUG_TRUE@@NATIVE_TRUE@am__append_9 = ctestg ctesta \ @HAVE_COMPRESSED_DEBUG_TRUE@@NATIVE_TRUE@ ctestg_alloc \ @HAVE_COMPRESSED_DEBUG_TRUE@@NATIVE_TRUE@ ctesta_alloc subdir = . @@ -789,7 +790,7 @@ libbacktrace_la_LIBADD = \ libbacktrace_la_DEPENDENCIES = $(libbacktrace_la_LIBADD) TESTS = $(check_PROGRAMS) $(am__append_2) $(am__append_3) \ - $(am__append_7) + $(am__append_4) $(am__append_8) @NATIVE_TRUE@check_LTLIBRARIES = libbacktrace_alloc.la \ @NATIVE_TRUE@ libbacktrace_noformat.la \ @NATIVE_TRUE@ libbacktrace_instrumented_alloc.la @@ -834,9 +835,9 @@ TESTS = $(check_PROGRAMS) $(am__append_2) $(am__append_3) \ @NATIVE_TRUE@stest_alloc_LDADD = libbacktrace_alloc.la @NATIVE_TRUE@ztest_SOURCES = ztest.c testlib.c @NATIVE_TRUE@ztest_CFLAGS = -DSRCDIR=\"$(srcdir)\" -@NATIVE_TRUE@ztest_LDADD = libbacktrace.la $(am__append_4) \ +@NATIVE_TRUE@ztest_LDADD = libbacktrace.la $(am__append_5) \ @NATIVE_TRUE@ $(CLOCK_GETTIME_LINK)
Re: [backtrace] Avoid segfault
On 25-01-19 01:51, Ian Lance Taylor wrote: > On Thu, Jan 24, 2019 at 4:11 PM Nathan Sidwell wrote: >> >> I just tripped over a segfault in libbacktrace. We apply strrchr to a >> possibly NULL filename, with predictable results when it is. >> >> elf.c:3044 passes NULL as the filename parm: >> ret = elf_add (state, NULL, d, base_address, error_callback, data, >> fileline_fn, found_sym, found_dwarf, NULL, 0, 1, >> NULL, >> 0); >> >> This gets to elf_open_debugfile_by_debuglink which passes it on through: >>ddescriptor = elf_find_debugfile_by_debuglink (state, filename, >> debuglink_name, >> error_callback, data); >> >> this patch avoids the strrchr when filename is null. I reordered the >> way prefix & prefix len got set, finding it prefereable to: >>slash = filename ? NULL : strrchr (filename, '/'); >> but if you prefer to avoid the assignment in the conditional I'm fine >> with that too. > > Yeah, please don't do an assignment in a conditional. > > Why don't we just pass "" instead of NULL in the call to elf_add? If > that works, that is OK. > With this refactoring preamble ... [libbacktrace] Rename dtest to btest_gnudebuglink Create a pattern rule for copying an existing test-case, separating out the debug information into a .debug file, and referencing the .debug file from the copied test-case using a .gnu_debuglink. 2019-01-25 Tom de Vries * Makefile.am: Rewrite dtest rule into "%_gnudebuglink" pattern rule. (TESTS): Rename dtest to btest_gnudebuglink. * Makefile.in: Regenerate. --- libbacktrace/Makefile.am | 8 libbacktrace/Makefile.in | 14 +++--- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/libbacktrace/Makefile.am b/libbacktrace/Makefile.am index bf90ebdb2d5..2f1d9af89c3 100644 --- a/libbacktrace/Makefile.am +++ b/libbacktrace/Makefile.am @@ -255,11 +255,11 @@ endif HAVE_PTHREAD if HAVE_OBJCOPY_DEBUGLINK -TESTS += dtest +TESTS += btest_gnudebuglink -dtest: btest - $(OBJCOPY) --only-keep-debug btest btest.debug - $(OBJCOPY) --strip-debug --add-gnu-debuglink=btest.debug btest dtest +%_gnudebuglink: % + $(OBJCOPY) --only-keep-debug $< $@.debug + $(OBJCOPY) --strip-debug --add-gnu-debuglink=$@.debug $< $@ endif HAVE_OBJCOPY_DEBUGLINK diff --git a/libbacktrace/Makefile.in b/libbacktrace/Makefile.in index d55e0501171..0b73e3d6981 100644 --- a/libbacktrace/Makefile.in +++ b/libbacktrace/Makefile.in @@ -130,7 +130,7 @@ check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) @HAVE_ZLIB_TRUE@@NATIVE_TRUE@am__append_4 = -lz @HAVE_ZLIB_TRUE@@NATIVE_TRUE@am__append_5 = -lz @HAVE_PTHREAD_TRUE@@NATIVE_TRUE@am__append_6 = ttest ttest_alloc -@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_7 = dtest +@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_7 = btest_gnudebuglink @HAVE_COMPRESSED_DEBUG_TRUE@@NATIVE_TRUE@am__append_8 = ctestg ctesta \ @HAVE_COMPRESSED_DEBUG_TRUE@@NATIVE_TRUE@ ctestg_alloc \ @HAVE_COMPRESSED_DEBUG_TRUE@@NATIVE_TRUE@ ctesta_alloc @@ -1585,9 +1585,9 @@ btest_dwz.log: btest_dwz --log-file $$b.log --trs-file $$b.trs \ $(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \ "$$tst" $(AM_TESTS_FD_REDIRECT) -dtest.log: dtest - @p='dtest'; \ - b='dtest'; \ +btest_gnudebuglink.log: btest_gnudebuglink + @p='btest_gnudebuglink'; \ + b='btest_gnudebuglink'; \ $(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \ --log-file $$b.log --trs-file $$b.trs \ $(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \ @@ -1767,9 +1767,9 @@ uninstall-am: @NATIVE_TRUE@ $(SHELL) $(srcdir)/../move-if-change tmp-edtest2_build.c edtest2_build.c @NATIVE_TRUE@ echo timestamp > $@ -@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@dtest: btest -@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@ $(OBJCOPY) --only-keep-debug btest btest.debug -@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@ $(OBJCOPY) --strip-debug --add-gnu-debuglink=btest.debug btest dtest +@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@%_gnudebuglink: % +@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@ $(OBJCOPY) --only-keep-debug $< $@.debug +@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@ $(OBJCOPY) --strip-debug --add-gnu-debuglink=$@.debug $< $@ alloc.lo: config.h backtrace.h internal.h backtrace.lo: config.h backtrace.h internal.h btest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h
Re: [backtrace] Avoid segfault
On Thu, Jan 24, 2019 at 4:11 PM Nathan Sidwell wrote: > > I just tripped over a segfault in libbacktrace. We apply strrchr to a > possibly NULL filename, with predictable results when it is. > > elf.c:3044 passes NULL as the filename parm: > ret = elf_add (state, NULL, d, base_address, error_callback, data, > fileline_fn, found_sym, found_dwarf, NULL, 0, 1, > NULL, > 0); > > This gets to elf_open_debugfile_by_debuglink which passes it on through: >ddescriptor = elf_find_debugfile_by_debuglink (state, filename, > debuglink_name, > error_callback, data); > > this patch avoids the strrchr when filename is null. I reordered the > way prefix & prefix len got set, finding it prefereable to: >slash = filename ? NULL : strrchr (filename, '/'); > but if you prefer to avoid the assignment in the conditional I'm fine > with that too. Yeah, please don't do an assignment in a conditional. Why don't we just pass "" instead of NULL in the call to elf_add? If that works, that is OK. Thanks. Ian
[backtrace] Avoid segfault
I just tripped over a segfault in libbacktrace. We apply strrchr to a
possibly NULL filename, with predictable results when it is.
elf.c:3044 passes NULL as the filename parm:
ret = elf_add (state, NULL, d, base_address, error_callback, data,
fileline_fn, found_sym, found_dwarf, NULL, 0, 1, NULL,
0);
This gets to elf_open_debugfile_by_debuglink which passes it on through:
ddescriptor = elf_find_debugfile_by_debuglink (state, filename,
debuglink_name,
error_callback, data);
this patch avoids the strrchr when filename is null. I reordered the
way prefix & prefix len got set, finding it prefereable to:
slash = filename ? NULL : strrchr (filename, '/');
but if you prefer to avoid the assignment in the conditional I'm fine
with that too.
ok?
nathan
--
Nathan Sidwell
2019-01-24 Nathan Sidwell
* elf.c (elf_find_debugfile_by_debuglink): Protect against
FILENAME being NULL.
Index: libbacktrace/elf.c
===
--- libbacktrace/elf.c (revision 268252)
+++ libbacktrace/elf.c (working copy)
@@ -970,13 +970,9 @@ elf_find_debugfile_by_debuglink (struct
/* Look for DEBUGLINK_NAME in the same directory as FILENAME. */
- slash = strrchr (filename, '/');
- if (slash == NULL)
-{
- prefix = "";
- prefix_len = 0;
-}
- else
+ prefix = "";
+ prefix_len = 0;
+ if (filename && (slash = strrchr (filename, '/') != NULL)
{
slash++;
prefix = filename;
