public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Nick Alcock <nick.alcock@oracle.com>
To: binutils@sourceware.org
Subject: [PATCH v2 3/3] libctf: ctf-link outdated input check faulty
Date: Tue, 10 Jan 2023 13:00:50 +0000	[thread overview]
Message-ID: <20230110130050.366404-4-nick.alcock@oracle.com> (raw)
In-Reply-To: <20230110130050.366404-1-nick.alcock@oracle.com>

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


  parent reply	other threads:[~2023-01-10 13:01 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2023-01-11 17:21   ` [PATCH 2.40 v2 3/3] libctf: ctf-link outdated input check faulty Nick Alcock
2023-01-12 13:43     ` Nick Clifton
2023-01-12 14:46       ` Nick Alcock
2023-01-12 15:39         ` Nick Alcock
2023-01-13 11:14           ` Nick Clifton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230110130050.366404-4-nick.alcock@oracle.com \
    --to=nick.alcock@oracle.com \
    --cc=binutils@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).