Re: [PATCH 2.40 v2 3/3] libctf: ctf-link outdated input check faulty

[Nick Alcock <nick.alcock@oracle.com>]

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)