* [PATCH v2 1/3] ctf: fix various dreadful typos in the ctf_archive format comments
2023-01-10 13:00 [PATCH v2 0/3] A few backlogged fixes, and a crash bug Nick Alcock
@ 2023-01-10 13:00 ` Nick Alcock
2023-01-10 13:00 ` [PATCH v2 2/3] libctf: skip the testsuite from inside dejagnu Nick Alcock
2023-01-10 13:00 ` [PATCH v2 3/3] libctf: ctf-link outdated input check faulty Nick Alcock
2 siblings, 0 replies; 9+ messages in thread
From: Nick Alcock @ 2023-01-10 13:00 UTC (permalink / raw)
To: binutils
When defining a format it helps to a) get the endianness right when you
explicitly state what it is and b) define things in terms of fields that
exist rather than fields that don't.
(A bunch of changes of names during implementation were not reflected in
these comments...)
Thanks to Jose "Eye of the Eagle" Marchesi for spotting these.
include/
* ctf.h (struct ctf_archive) [ctfa_ctfs]: The size element of
this is in little-endian byte order, not network byte order.
(struct ctf_archive_modent): This is positioned right after the
end fo the struct ctf_archive, not at the offset of a
nonexistent field. The number of elements in the array depends
on ctfa_ndicts, not another nonexistent field.
---
include/ctf.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/ctf.h b/include/ctf.h
index 4414cb0ebed..4263799f82d 100644
--- a/include/ctf.h
+++ b/include/ctf.h
@@ -599,13 +599,13 @@ struct ctf_archive
/* Offset of the name table. */
uint64_t ctfa_names;
- /* Offset of the CTF table. Each element starts with a size (a uint64_t
- in network byte order) then a ctf_dict_t of that size. */
+ /* Offset of the CTF table. Each element starts with a size (a little-
+ endian uint64_t) then a ctf_dict_t of that size. */
uint64_t ctfa_ctfs;
};
-/* An array of ctfa_nnamed of this structure lies at
- ctf_archive[ctf_archive->ctfa_modents] and gives the ctfa_ctfs or
+/* An array of ctfa_ndicts of this structure lies at
+ ctf_archive[sizeof(struct ctf_archive)] and gives the ctfa_ctfs or
ctfa_names-relative offsets of each name or ctf_dict_t. */
typedef struct ctf_archive_modent
--
2.39.0.267.g7648178303
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2 2/3] libctf: skip the testsuite from inside dejagnu
2023-01-10 13:00 [PATCH v2 0/3] A few backlogged fixes, and a crash bug Nick Alcock
2023-01-10 13:00 ` [PATCH v2 1/3] ctf: fix various dreadful typos in the ctf_archive format comments Nick Alcock
@ 2023-01-10 13:00 ` Nick Alcock
2023-01-10 13:00 ` [PATCH v2 3/3] libctf: ctf-link outdated input check faulty Nick Alcock
2 siblings, 0 replies; 9+ messages in thread
From: Nick Alcock @ 2023-01-10 13:00 UTC (permalink / raw)
To: binutils
The libctf testsuite uses Tcl try/catch to trap run_output errors. This
is only supported in reasonably recent Tcls, so we detect the lack of
try/catch and suppress the testsuite via an Automake conditional in its
absence.
But this turns out not to work: Automake produces a check-DEJAGNU target
regardless of the value of this conditional and sticks it in an
unconditionally-executed part of the makefile, so the testsuite gets
executed anyway, and fails with a nasty-looking syntax error. We can't
disable it by taking "dejagnu" out of AUTOMAKE_OPTIONS, because if you
do that Automake stops you using RUNTEST, RUNTESTFLAGS and other
variables users would expect to work.
So move to disabling the testsuite from inside the testsuite itself,
importing the value of the former Automake conditional as a Tcl variable
and exiting very early in default.exp if it's false.
* configure.ac (TCL_TRY): No longer an Automake conditional.
Rename to...
(HAVE_TCL_TRY): ... this.
* Makefile.am: Drop TCL_TRY.
(development.exp): Set have_tcl_try.
* testsuite/config/default.exp: Exit if have_tcl_try is false.
* configure: Regenerated.
* Makefile.in: Likewise.
---
libctf/Makefile.am | 3 +-
libctf/Makefile.in | 95 +++++++++++++----------------
libctf/configure | 21 +++----
libctf/configure.ac | 7 ++-
libctf/testsuite/config/default.exp | 5 ++
5 files changed, 61 insertions(+), 70 deletions(-)
diff --git a/libctf/Makefile.am b/libctf/Makefile.am
index c4809f70bae..c959b09e492 100644
--- a/libctf/Makefile.am
+++ b/libctf/Makefile.am
@@ -72,7 +72,6 @@ EXPECT = expect
RUNTEST = runtest
RUNTESTFLAGS =
-if TCL_TRY
CC_FOR_TARGET = ` \
if [ -f $$r/../gcc/xgcc ] ; then \
if [ -f $$r/../newlib/Makefile ] ; then \
@@ -105,6 +104,7 @@ check-DEJAGNU: site.exp development.exp
development.exp: $(BFDDIR)/development.sh
$(AM_V_GEN)$(EGREP) "(development|experimental)=" $(BFDDIR)/development.sh \
| $(AWK) -F= '{ print "set " $$1 " " $$2 }' > $@
+ echo "set have_tcl_try @HAVE_TCL_TRY@" >> $@
# development.sh is used to determine -Werror default.
CONFIG_STATUS_DEPENDENCIES = $(BFDDIR)/development.sh
@@ -112,6 +112,5 @@ CONFIG_STATUS_DEPENDENCIES = $(BFDDIR)/development.sh
EXTRA_DEJAGNU_SITE_CONFIG = development.exp
DISTCLEANFILES += site.exp development.exp
-endif
include doc/local.mk
diff --git a/libctf/Makefile.in b/libctf/Makefile.in
index f2b852e3fae..c6cb55c9768 100644
--- a/libctf/Makefile.in
+++ b/libctf/Makefile.in
@@ -127,10 +127,9 @@ build_triplet = @build@
host_triplet = @host@
target_triplet = @target@
@NEED_CTF_QSORT_R_TRUE@am__append_1 = ctf-qsort_r.c
-@TCL_TRY_TRUE@am__append_2 = site.exp development.exp
-@BUILD_INFO_TRUE@am__append_3 = doc/ctf-spec.texi
-@BUILD_INFO_TRUE@am__append_4 = texput.log
-@BUILD_INFO_TRUE@am__append_5 = doc/ctf-spec.info
+@BUILD_INFO_TRUE@am__append_2 = doc/ctf-spec.texi
+@BUILD_INFO_TRUE@am__append_3 = texput.log
+@BUILD_INFO_TRUE@am__append_4 = doc/ctf-spec.info
subdir = .
ACLOCAL_M4 = $(top_srcdir)/aclocal.m4
am__aclocal_m4_deps = $(top_srcdir)/../bfd/acinclude.m4 \
@@ -312,7 +311,7 @@ DVIS = doc/ctf-spec.dvi
PDFS = doc/ctf-spec.pdf
PSS = doc/ctf-spec.ps
HTMLS = doc/ctf-spec.html
-TEXINFOS = $(am__append_3)
+TEXINFOS = $(am__append_2)
TEXI2DVI = texi2dvi
TEXI2PDF = $(TEXI2DVI) --pdf --batch
MAKEINFOHTML = $(MAKEINFO) --html
@@ -407,6 +406,7 @@ FGREP = @FGREP@
GENCAT = @GENCAT@
GMSGFMT = @GMSGFMT@
GREP = @GREP@
+HAVE_TCL_TRY = @HAVE_TCL_TRY@
INCINTL = @INCINTL@
INSTALL = @INSTALL@
INSTALL_DATA = @INSTALL_DATA@
@@ -522,9 +522,9 @@ ACLOCAL_AMFLAGS = -I .. -I ../config -I ../bfd
AUTOMAKE_OPTIONS = dejagnu foreign info-in-builddir no-texinfo.tex
# Variables that we might accumulate conditionally or in subdirs.
-info_TEXINFOS = $(am__append_3)
-DISTCLEANFILES = $(am__append_2) $(am__append_4)
-MAINTAINERCLEANFILES = $(am__append_5)
+info_TEXINFOS = $(am__append_2)
+DISTCLEANFILES = site.exp development.exp $(am__append_3)
+MAINTAINERCLEANFILES = $(am__append_4)
# This is where we get zlib from. zlibdir is -L../zlib and zlibinc is
# -I../zlib, unless we were configured with --with-system-zlib, in which
@@ -559,25 +559,25 @@ libctf_la_LDFLAGS = $(libctf_ldflags_nover) @VERSION_FLAGS@
libctf_la_SOURCES = $(libctf_nobfd_la_SOURCES) ctf-open-bfd.c
RUNTEST = runtest
RUNTESTFLAGS =
-@TCL_TRY_TRUE@CC_FOR_TARGET = ` \
-@TCL_TRY_TRUE@ if [ -f $$r/../gcc/xgcc ] ; then \
-@TCL_TRY_TRUE@ if [ -f $$r/../newlib/Makefile ] ; then \
-@TCL_TRY_TRUE@ echo $$r/../gcc/xgcc -B$$r/../gcc/ -idirafter $$r/../newlib/targ-include -idirafter $${srcroot}/../newlib/libc/include -nostdinc; \
-@TCL_TRY_TRUE@ else \
-@TCL_TRY_TRUE@ echo $$r/../gcc/xgcc -B$$r/../gcc/; \
-@TCL_TRY_TRUE@ fi; \
-@TCL_TRY_TRUE@ else \
-@TCL_TRY_TRUE@ if [ "@host@" = "@target@" ] ; then \
-@TCL_TRY_TRUE@ echo $(CC); \
-@TCL_TRY_TRUE@ else \
-@TCL_TRY_TRUE@ echo gcc | sed '$(transform)'; \
-@TCL_TRY_TRUE@ fi; \
-@TCL_TRY_TRUE@ fi`
+CC_FOR_TARGET = ` \
+ if [ -f $$r/../gcc/xgcc ] ; then \
+ if [ -f $$r/../newlib/Makefile ] ; then \
+ echo $$r/../gcc/xgcc -B$$r/../gcc/ -idirafter $$r/../newlib/targ-include -idirafter $${srcroot}/../newlib/libc/include -nostdinc; \
+ else \
+ echo $$r/../gcc/xgcc -B$$r/../gcc/; \
+ fi; \
+ else \
+ if [ "@host@" = "@target@" ] ; then \
+ echo $(CC); \
+ else \
+ echo gcc | sed '$(transform)'; \
+ fi; \
+ fi`
# development.sh is used to determine -Werror default.
-@TCL_TRY_TRUE@CONFIG_STATUS_DEPENDENCIES = $(BFDDIR)/development.sh
-@TCL_TRY_TRUE@EXTRA_DEJAGNU_SITE_CONFIG = development.exp
+CONFIG_STATUS_DEPENDENCIES = $(BFDDIR)/development.sh
+EXTRA_DEJAGNU_SITE_CONFIG = development.exp
@BUILD_INFO_TRUE@AM_MAKEINFOFLAGS = --no-split
all: config.h
$(MAKE) $(AM_MAKEFLAGS) all-am
@@ -1235,18 +1235,6 @@ cscopelist-am: $(am__tagged_files)
distclean-tags:
-rm -f TAGS ID GTAGS GRTAGS GSYMS GPATH tags
-rm -f cscope.out cscope.in.out cscope.po.out cscope.files
-
-@TCL_TRY_FALSE@check-DEJAGNU: site.exp
-@TCL_TRY_FALSE@ srcdir='$(srcdir)'; export srcdir; \
-@TCL_TRY_FALSE@ EXPECT=$(EXPECT); export EXPECT; \
-@TCL_TRY_FALSE@ if $(SHELL) -c "$(RUNTEST) --version" > /dev/null 2>&1; then \
-@TCL_TRY_FALSE@ exit_status=0; l='$(DEJATOOL)'; for tool in $$l; do \
-@TCL_TRY_FALSE@ if $(RUNTEST) $(AM_RUNTESTFLAGS) $(RUNTESTDEFAULTFLAGS) $(RUNTESTFLAGS); \
-@TCL_TRY_FALSE@ then :; else exit_status=1; fi; \
-@TCL_TRY_FALSE@ done; \
-@TCL_TRY_FALSE@ else echo "WARNING: could not find '$(RUNTEST)'" 1>&2; :;\
-@TCL_TRY_FALSE@ fi; \
-@TCL_TRY_FALSE@ exit $$exit_status
site.exp: Makefile $(EXTRA_DEJAGNU_SITE_CONFIG)
@echo 'Making a new site.exp file ...'
@echo '## these variables are automatically generated by make ##' >site.tmp
@@ -1685,23 +1673,24 @@ uninstall-am: uninstall-dvi-am uninstall-html-am \
.PRECIOUS: Makefile
-@TCL_TRY_TRUE@check-DEJAGNU: site.exp development.exp
-@TCL_TRY_TRUE@ srcroot=`cd $(srcdir) && pwd`; export srcroot; \
-@TCL_TRY_TRUE@ r=`pwd`; export r; \
-@TCL_TRY_TRUE@ LC_ALL=C; export LC_ALL; \
-@TCL_TRY_TRUE@ EXPECT=$(EXPECT); export EXPECT; \
-@TCL_TRY_TRUE@ runtest=$(RUNTEST); \
-@TCL_TRY_TRUE@ if $(SHELL) -c "$$runtest --version" > /dev/null 2>&1; then \
-@TCL_TRY_TRUE@ $$runtest --tool $(DEJATOOL) --srcdir $${srcroot}/testsuite \
-@TCL_TRY_TRUE@ CC="$(CC)" CC_FOR_TARGET="$(CC_FOR_TARGET)" \
-@TCL_TRY_TRUE@ CFLAGS="$(CFLAGS) -I$(INCDIR) -I$(srcdir) -I$(builddir) -I$(builddir)/../bfd $(ZLIBINC)" \
-@TCL_TRY_TRUE@ LIBS="$(libctf_nobfd_la_LIBADD) $(LIBS)" $(RUNTESTFLAGS); \
-@TCL_TRY_TRUE@ else echo "WARNING: could not find \`runtest'" 1>&2; :;\
-@TCL_TRY_TRUE@ fi
-
-@TCL_TRY_TRUE@development.exp: $(BFDDIR)/development.sh
-@TCL_TRY_TRUE@ $(AM_V_GEN)$(EGREP) "(development|experimental)=" $(BFDDIR)/development.sh \
-@TCL_TRY_TRUE@ | $(AWK) -F= '{ print "set " $$1 " " $$2 }' > $@
+check-DEJAGNU: site.exp development.exp
+ srcroot=`cd $(srcdir) && pwd`; export srcroot; \
+ r=`pwd`; export r; \
+ LC_ALL=C; export LC_ALL; \
+ EXPECT=$(EXPECT); export EXPECT; \
+ runtest=$(RUNTEST); \
+ if $(SHELL) -c "$$runtest --version" > /dev/null 2>&1; then \
+ $$runtest --tool $(DEJATOOL) --srcdir $${srcroot}/testsuite \
+ CC="$(CC)" CC_FOR_TARGET="$(CC_FOR_TARGET)" \
+ CFLAGS="$(CFLAGS) -I$(INCDIR) -I$(srcdir) -I$(builddir) -I$(builddir)/../bfd $(ZLIBINC)" \
+ LIBS="$(libctf_nobfd_la_LIBADD) $(LIBS)" $(RUNTESTFLAGS); \
+ else echo "WARNING: could not find \`runtest'" 1>&2; :;\
+ fi
+
+development.exp: $(BFDDIR)/development.sh
+ $(AM_V_GEN)$(EGREP) "(development|experimental)=" $(BFDDIR)/development.sh \
+ | $(AWK) -F= '{ print "set " $$1 " " $$2 }' > $@
+ echo "set have_tcl_try @HAVE_TCL_TRY@" >> $@
@BUILD_INFO_TRUE@html-local: doc/ctf-spec/index.html
@BUILD_INFO_TRUE@doc/ctf-spec/index.html: doc/ctf-spec.texi doc/$(am__dirstamp)
diff --git a/libctf/configure b/libctf/configure
index 17463a74cca..c22f7dffd2c 100755
--- a/libctf/configure
+++ b/libctf/configure
@@ -637,8 +637,7 @@ LTLIBOBJS
LIBOBJS
VERSION_FLAGS_NOBFD
VERSION_FLAGS
-TCL_TRY_FALSE
-TCL_TRY_TRUE
+HAVE_TCL_TRY
EXPECT
CTF_LIBADD
SHARED_LDFLAGS
@@ -11632,7 +11631,7 @@ else
lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
lt_status=$lt_dlunknown
cat > conftest.$ac_ext <<_LT_EOF
-#line 11635 "configure"
+#line 11634 "configure"
#include "confdefs.h"
#if HAVE_DLFCN_H
@@ -11738,7 +11737,7 @@ else
lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
lt_status=$lt_dlunknown
cat > conftest.$ac_ext <<_LT_EOF
-#line 11741 "configure"
+#line 11740 "configure"
#include "confdefs.h"
#if HAVE_DLFCN_H
@@ -14975,12 +14974,10 @@ fi`
fi
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_libctf_tcl_try" >&5
$as_echo "$ac_cv_libctf_tcl_try" >&6; }
- if test "${ac_cv_libctf_tcl_try}" = yes; then
- TCL_TRY_TRUE=
- TCL_TRY_FALSE='#'
-else
- TCL_TRY_TRUE='#'
- TCL_TRY_FALSE=
+
+HAVE_TCL_TRY=false
+if test "${ac_cv_libctf_tcl_try}" = "yes"; then
+ HAVE_TCL_TRY=true
fi
@@ -15208,10 +15205,6 @@ if test -z "${BUILD_INFO_TRUE}" && test -z "${BUILD_INFO_FALSE}"; then
as_fn_error $? "conditional \"BUILD_INFO\" was never defined.
Usually this means the macro was only invoked conditionally." "$LINENO" 5
fi
-if test -z "${TCL_TRY_TRUE}" && test -z "${TCL_TRY_FALSE}"; then
- as_fn_error $? "conditional \"TCL_TRY\" was never defined.
-Usually this means the macro was only invoked conditionally." "$LINENO" 5
-fi
: "${CONFIG_STATUS=./config.status}"
ac_write_fail=0
diff --git a/libctf/configure.ac b/libctf/configure.ac
index a0148a4c328..1d0cf4d0fa5 100644
--- a/libctf/configure.ac
+++ b/libctf/configure.ac
@@ -241,7 +241,12 @@ if @<:@llength @<:@info commands try@:>@@:>@ then { puts yes } else { puts no }
EOF
fi`
])
-AM_CONDITIONAL(TCL_TRY, test "${ac_cv_libctf_tcl_try}" = yes)
+
+HAVE_TCL_TRY=false
+if test "${ac_cv_libctf_tcl_try}" = "yes"; then
+ HAVE_TCL_TRY=true
+fi
+AC_SUBST(HAVE_TCL_TRY)
# Use a version script, if possible, or an -export-symbols-regex otherwise.
decommented_version_script=
diff --git a/libctf/testsuite/config/default.exp b/libctf/testsuite/config/default.exp
index 4bac9c4281c..357c4ee57f1 100644
--- a/libctf/testsuite/config/default.exp
+++ b/libctf/testsuite/config/default.exp
@@ -21,6 +21,11 @@
# Written by Jeffrey Wheat (cassidy@cygnus.com)
#
+# Don't run anything if Tcl is too old to have try / catch.
+if { ! $have_tcl_try } {
+ log_and_exit
+}
+
if ![info exists ld] then {
set ld [findfile $base_dir/../ld/ld-new $base_dir/../ld/ld-new [transform ld]]
}
--
2.39.0.267.g7648178303
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2 3/3] libctf: ctf-link outdated input check faulty
2023-01-10 13:00 [PATCH v2 0/3] A few backlogged fixes, and a crash bug Nick Alcock
2023-01-10 13:00 ` [PATCH v2 1/3] ctf: fix various dreadful typos in the ctf_archive format comments Nick Alcock
2023-01-10 13:00 ` [PATCH v2 2/3] libctf: skip the testsuite from inside dejagnu Nick Alcock
@ 2023-01-10 13:00 ` Nick Alcock
2023-01-11 17:21 ` [PATCH 2.40 " Nick Alcock
2 siblings, 1 reply; 9+ messages in thread
From: Nick Alcock @ 2023-01-10 13:00 UTC (permalink / raw)
To: binutils
This check has a pair of faults which, combined, can lead to memory
corruption. Firstly, it assumes that the values of the ctf_link_inputs
hash are ctf_dict_t's: they are not, they are ctf_link_input_t's, a much
shorter structure. So the flags check which is the core of this is
faulty (but happens, by chance, to give the right output on most
architectures, since usually we happen to get a 0 here, so the test that
checks this usually passes). Worse, the warning that is emitted when
the test fails is added to the wrong dict -- it's added to the input
dict, whose warning list is never consumed, rendering the whole check
useless. But the dict it adds to is still the wrong type, so we end up
overwriting something deep in memory (or, much more likely,
dereferencing a garbage pointer and crashing).
Fixing both reveals another problem: the link input is an *archive*
consisting of multiple members, so we have to consider whether to check
all of them for the outdated-func-info thing we are checking here.
However, no compiler exists that emits a mixture of members with this
flag on and members with it off, and the linker always reserializes (and
upgrades) such things when it sees them: so all members in a given
archive must have the same value of the flag, so we only need to check
one member per input archive.
libctf/
PR libctf/29983
* ctf-link.c (ctf_link_warn_outdated_inputs): Get the types of
members of ctf_link_inputs right, fixing a possible spurious
tesst failure / wild pointer deref / overwrite. Emit the
warning message into the right dict.
---
libctf/ctf-link.c | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/libctf/ctf-link.c b/libctf/ctf-link.c
index 2837168b2a6..df8fa3b9d9b 100644
--- a/libctf/ctf-link.c
+++ b/libctf/ctf-link.c
@@ -1848,19 +1848,42 @@ ctf_link_warn_outdated_inputs (ctf_dict_t *fp)
{
ctf_next_t *i = NULL;
void *name_;
- void *ifp_;
+ void *input_;
int err;
- while ((err = ctf_dynhash_next (fp->ctf_link_inputs, &i, &name_, &ifp_)) == 0)
+ while ((err = ctf_dynhash_next (fp->ctf_link_inputs, &i, &name_, &input_)) == 0)
{
const char *name = (const char *) name_;
- ctf_dict_t *ifp = (ctf_dict_t *) ifp_;
+ ctf_link_input_t *input = (ctf_link_input_t *) input_;
+ ctf_next_t *j = NULL;
+ ctf_dict_t *ifp;
+ int err;
+
+ /* We only care about CTF archives by this point: lazy-opened archives
+ have always been opened by this point, and short-circuited entries have
+ a matching corresponding archive member. Entries with NULL clin_arc can
+ exist, and constitute old entries renamed via a name changer: the
+ renamed entries exist elsewhere in the list, so we can just skip
+ those. */
+
+ if (!input->clin_arc)
+ continue;
+
+ /* All entries in the archive will necessarily contain the same
+ CTF_F_NEWFUNCINFO flag, so we only need to check the first. We don't
+ even need to do that if we can't open it for any reason at all: the
+ link will fail later on regardless, since an input can't be opened. */
+
+ ifp = ctf_archive_next (input->clin_arc, &j, NULL, 0, &err);
+ if (!ifp)
+ continue;
+ ctf_next_destroy (j);
if (!(ifp->ctf_header->cth_flags & CTF_F_NEWFUNCINFO)
&& (ifp->ctf_header->cth_varoff - ifp->ctf_header->cth_funcoff) > 0)
- ctf_err_warn (ifp, 1, 0, _("linker input %s has CTF func info but uses "
- "an old, unreleased func info format: "
- "this func info section will be dropped."),
+ ctf_err_warn (fp, 1, 0, _("linker input %s has CTF func info but uses "
+ "an old, unreleased func info format: "
+ "this func info section will be dropped."),
name);
}
if (err != ECTF_NEXT_END)
--
2.39.0.267.g7648178303
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2.40 v2 3/3] libctf: ctf-link outdated input check faulty
2023-01-10 13:00 ` [PATCH v2 3/3] libctf: ctf-link outdated input check faulty Nick Alcock
@ 2023-01-11 17:21 ` Nick Alcock
2023-01-12 13:43 ` Nick Clifton
0 siblings, 1 reply; 9+ messages in thread
From: Nick Alcock @ 2023-01-11 17:21 UTC (permalink / raw)
To: binutils; +Cc: Nick Clifton
Is this patch alone OK for 2.40? (After I push this series to master.
Tomorrow, not today... all tests passed, crash on i686-pc-linux-gnu
fixed, valgrind-clean, asan-clean.)
(I plan to backport it all the way back to 2.36.)
On 10 Jan 2023, Nick Alcock via Binutils spake thusly:
> This check has a pair of faults which, combined, can lead to memory
> corruption. Firstly, it assumes that the values of the ctf_link_inputs
> hash are ctf_dict_t's: they are not, they are ctf_link_input_t's, a much
> shorter structure. So the flags check which is the core of this is
> faulty (but happens, by chance, to give the right output on most
> architectures, since usually we happen to get a 0 here, so the test that
> checks this usually passes). Worse, the warning that is emitted when
> the test fails is added to the wrong dict -- it's added to the input
> dict, whose warning list is never consumed, rendering the whole check
> useless. But the dict it adds to is still the wrong type, so we end up
> overwriting something deep in memory (or, much more likely,
> dereferencing a garbage pointer and crashing).
>
> Fixing both reveals another problem: the link input is an *archive*
> consisting of multiple members, so we have to consider whether to check
> all of them for the outdated-func-info thing we are checking here.
> However, no compiler exists that emits a mixture of members with this
> flag on and members with it off, and the linker always reserializes (and
> upgrades) such things when it sees them: so all members in a given
> archive must have the same value of the flag, so we only need to check
> one member per input archive.
>
> libctf/
> PR libctf/29983
> * ctf-link.c (ctf_link_warn_outdated_inputs): Get the types of
> members of ctf_link_inputs right, fixing a possible spurious
> tesst failure / wild pointer deref / overwrite. Emit the
> warning message into the right dict.
> ---
> libctf/ctf-link.c | 35 +++++++++++++++++++++++++++++------
> 1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/libctf/ctf-link.c b/libctf/ctf-link.c
> index 2837168b2a6..df8fa3b9d9b 100644
> --- a/libctf/ctf-link.c
> +++ b/libctf/ctf-link.c
> @@ -1848,19 +1848,42 @@ ctf_link_warn_outdated_inputs (ctf_dict_t *fp)
> {
> ctf_next_t *i = NULL;
> void *name_;
> - void *ifp_;
> + void *input_;
> int err;
>
> - while ((err = ctf_dynhash_next (fp->ctf_link_inputs, &i, &name_, &ifp_)) == 0)
> + while ((err = ctf_dynhash_next (fp->ctf_link_inputs, &i, &name_, &input_)) == 0)
> {
> const char *name = (const char *) name_;
> - ctf_dict_t *ifp = (ctf_dict_t *) ifp_;
> + ctf_link_input_t *input = (ctf_link_input_t *) input_;
> + ctf_next_t *j = NULL;
> + ctf_dict_t *ifp;
> + int err;
> +
> + /* We only care about CTF archives by this point: lazy-opened archives
> + have always been opened by this point, and short-circuited entries have
> + a matching corresponding archive member. Entries with NULL clin_arc can
> + exist, and constitute old entries renamed via a name changer: the
> + renamed entries exist elsewhere in the list, so we can just skip
> + those. */
> +
> + if (!input->clin_arc)
> + continue;
> +
> + /* All entries in the archive will necessarily contain the same
> + CTF_F_NEWFUNCINFO flag, so we only need to check the first. We don't
> + even need to do that if we can't open it for any reason at all: the
> + link will fail later on regardless, since an input can't be opened. */
> +
> + ifp = ctf_archive_next (input->clin_arc, &j, NULL, 0, &err);
> + if (!ifp)
> + continue;
> + ctf_next_destroy (j);
>
> if (!(ifp->ctf_header->cth_flags & CTF_F_NEWFUNCINFO)
> && (ifp->ctf_header->cth_varoff - ifp->ctf_header->cth_funcoff) > 0)
> - ctf_err_warn (ifp, 1, 0, _("linker input %s has CTF func info but uses "
> - "an old, unreleased func info format: "
> - "this func info section will be dropped."),
> + ctf_err_warn (fp, 1, 0, _("linker input %s has CTF func info but uses "
> + "an old, unreleased func info format: "
> + "this func info section will be dropped."),
> name);
> }
> if (err != ECTF_NEXT_END)
--
NULL && (void)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2.40 v2 3/3] libctf: ctf-link outdated input check faulty
2023-01-11 17:21 ` [PATCH 2.40 " Nick Alcock
@ 2023-01-12 13:43 ` Nick Clifton
2023-01-12 14:46 ` Nick Alcock
0 siblings, 1 reply; 9+ messages in thread
From: Nick Clifton @ 2023-01-12 13:43 UTC (permalink / raw)
To: Nick Alcock, binutils
Hi Nick,
> Is this patch alone OK for 2.40?
It is - but please do this today if you can - I am planning on creating
the release tarballs tomorrow...
> (After I push this series to master.
> Tomorrow, not today... all tests passed, crash on i686-pc-linux-gnu
> fixed, valgrind-clean, asan-clean.)
>
> (I plan to backport it all the way back to 2.36.)
Have fun!
Cheers
Nick
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2.40 v2 3/3] libctf: ctf-link outdated input check faulty
2023-01-12 13:43 ` Nick Clifton
@ 2023-01-12 14:46 ` Nick Alcock
2023-01-12 15:39 ` Nick Alcock
0 siblings, 1 reply; 9+ messages in thread
From: Nick Alcock @ 2023-01-12 14:46 UTC (permalink / raw)
To: Nick Clifton via Binutils; +Cc: Nick Clifton
On 12 Jan 2023, Nick Clifton via Binutils said:
> Hi Nick,
>
>> Is this patch alone OK for 2.40?
>
> It is - but please do this today if you can - I am planning on creating
> the release tarballs tomorrow...
OK! I'll be pushing to master and 2.40 in twenty minutes or so (final
paranoid tests running). A bit longer for the earlier branches, but
they're not holding up a release :)
--
NULL && (void)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2.40 v2 3/3] libctf: ctf-link outdated input check faulty
2023-01-12 14:46 ` Nick Alcock
@ 2023-01-12 15:39 ` Nick Alcock
2023-01-13 11:14 ` Nick Clifton
0 siblings, 1 reply; 9+ messages in thread
From: Nick Alcock @ 2023-01-12 15:39 UTC (permalink / raw)
To: binutils; +Cc: Nick Clifton
On 12 Jan 2023, Nick Alcock via Binutils uttered the following:
> On 12 Jan 2023, Nick Clifton via Binutils said:
>>> Is this patch alone OK for 2.40?
>>
>> It is - but please do this today if you can - I am planning on creating
>> the release tarballs tomorrow...
>
> OK! I'll be pushing to master and 2.40 in twenty minutes or so (final
Hark at the optimism! I reckoned without the traditional last-minute
massive slowdown of every system one is testing on. I'm only lucky the
power didn't go out.
> paranoid tests running). A bit longer for the earlier branches, but
> they're not holding up a release :)
Tests passed, finally: pushed to master and 2.40! Sorry this was so
late.
--
NULL && (void)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2.40 v2 3/3] libctf: ctf-link outdated input check faulty
2023-01-12 15:39 ` Nick Alcock
@ 2023-01-13 11:14 ` Nick Clifton
0 siblings, 0 replies; 9+ messages in thread
From: Nick Clifton @ 2023-01-13 11:14 UTC (permalink / raw)
To: Nick Alcock, binutils
Hi Nick,
>>> It is - but please do this today if you can - I am planning on creating
>>> the release tarballs tomorrow...
>>
>> OK! I'll be pushing to master and 2.40 in twenty minutes or so (final
>
> Hark at the optimism! I reckoned without the traditional last-minute
> massive slowdown of every system one is testing on. I'm only lucky the
> power didn't go out.
Ha- it is always the way. I must apologise too, since I was wrong to
say that I am going to releasing today (Fri 13th). I am actually
planning to make the release tomorrow (Sat 14th), but when I wrote
the original email I had it in my head that it was Friday already.
Wishful thinking I suppose...
> Tests passed, finally: pushed to master and 2.40!
Yay!
Cheers
Nick
^ permalink raw reply [flat|nested] 9+ messages in thread