Re: [PATCH] libctf: check for problems with error returns

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

On 13 Oct 2023, Torbjorn SVENSSON told this:

> Hi Nick,
>
> Thanks for validating this patch!
>
> On 2023-10-13 16:01, Nick Alcock wrote:
>> We do this as a writable test because the only known-affected platforms
>> (with ssize_t longer than unsigned long) use PE, and we do not have support
>> for CTF linkage in the PE linker yet.
>
> Is it visible in PE too or only PE32+? Maybe not important, but the
> Arm built variant does not trigger the fault (same source tree as I
> found the issue in, but they build 32-bit and I build 64-bit) when
> I've executed the GCC testsuite.

The underlying fault is client-side and is visible in anything that does
a ctf_member_next on a platform with sizeof(ssize_t) > sizeof(unsigned
long). The specific instance you saw (a hanging linker) requires CTF in
the input and a linker that deduplicates it, and right now that is ELF
only (because it was painful enough to add that I didn't have the
stamina to add anything else -- if anything else *is* added, it will
probably be PE next, but right now linking PE just blindly concatenates
CTF sections, which isn't very useful).

>> 	PR libctf/30836
>> 	* libctf/testsuite/libctf-writable/libctf-errors.*: New test.
>> ---
>>   .../testsuite/libctf-writable/libctf-errors.c | 74 +++++++++++++++++++
>>   .../libctf-writable/libctf-errors.lk          |  1 +
>>   2 files changed, 75 insertions(+)
>>   create mode 100644 libctf/testsuite/libctf-writable/libctf-errors.c
>>   create mode 100644 libctf/testsuite/libctf-writable/libctf-errors.lk
>> Your patch looks good, and passes every test I can throw at it. I think
>> it can go in.  You cleaned up a bunch of outright errors in this area,
>> too, especially in ctf-dedup: thanks!
>
> There is still some potential for cleanup as some functions are
> returning "unsigned long", but think it should perhaps be ctf_id_t
> instead.

The only one of those i can see is ctf_lookup_symbol_idx. That's
returning a symbol index, not a type ID, so should definitely not be
returning a ctf_id_t. Perhaps it should be an int, but I'm not sure how
big symbol tables in the set of all executable formats can actually
be...

Hm, actually, there is one bit you might want to adjust in ctf-lookup.c.
You fixed one call to ctf_lookup_symbol_idx () in
ctf_lookup_by_sym_or_name ():

    if ((symidx = ctf_lookup_symbol_idx (fp, symname)) == (unsigned long) -1)
      goto try_parent;

but the other one, in ctf_lookup_symbol_idx () itself (doing a recursive
call to check parent dicts), still says

      if ((psym = ctf_lookup_symbol_idx (fp->ctf_parent, symname))
          != CTF_ERR)

Both symidx and psym are unsigned long variables, so probably both
should be checking against (unsigned long) -1.

>> (You probably want to adjust the commit log so that the version history
>> is at the bottom rather than the top, or drop it entirely.)
>
> My plan was to drop that part.

Aha right.

> Do you think I should have a changelog entry for the commit and if so,
> what should I write in it? Should I list every function that is
> touched (more or less half of the ctf_* functions defined...) or is
> there some better way to document this change?

You can just say

	Affected functions adjusted.

or something. A giant list adds no value, I think.

>> Here's a testcase that fails on mingw64 in the absence of your patch,
>> without requiring a cross-build to an ELF arch.  (It also checks at
>> least one instance of the other classes of error return in libctf.)
>> I'll push this after your commit goes in.  (I can push it, with an
>> adjusted commit log, if you want.
>
> Fine either way. Either reply to my question about changelog or just merge it with the correct answer :)

... I think there's one more tiny change :/

>> +  /* Third error class: ssize_t return.  Create a type to iterate over first.  */
>> +
>> +  if ((itype = ctf_add_integer (fp, CTF_ADD_ROOT, "int", &encoding)) == CTF_ERR)
>> +    fprintf (stderr, "cannot add int: %s\n", ctf_errmsg (ctf_errno (fp)));
>> +  else if ((stype = ctf_add_struct (fp, CTF_ADD_ROOT, "foo")) == CTF_ERR)
>> +    fprintf (stderr, "cannot add struct: %s\n", ctf_errmsg (ctf_errno (fp)));
>> +  else if (ctf_add_member (fp, stype, "bar", itype) < 0)
>> +    fprintf (stderr, "cannot add member: %s\n", ctf_errmsg (ctf_errno (fp)));
>
> Should these be if-else-if-else-if-statments like above or just 3 "free-standing" if-statements? I.e. should the 3 potential issue
> be visible in the same run or should they require 3 consecutive runs if all of them are failing?

I was wondering if this was too clever (or unclear!) but the intent is
to only do the later things if the earlier ones didn't fail, i.e. what
would be foo || bar || baz in the shell. (Later operations will also
fail if any of them fail, but the fprintf's from the first failure will
suffice to make the whole test fail -- so maybe it's safe to just do
them in sequence even if the earlier ones failed. Excessive paranoia on
my part perhaps, given that we don't *expect* any of these to fail: it's
just setting things up for the *actual* tests, of ctf_member_info and
ctf_member_next.)

>> +  if (ctf_member_info (fp, stype, "bar", &mi) < 0)
>> +    fprintf (stderr, "cannot get member info: %s\n", ctf_errmsg (ctf_errno (fp)));
>> +
>> +  /* Iteration should never produce an offset bigger than the offset just returned,
>> +     and should quickly terminate.  */
>> +
>> +  while ((ret = ctf_member_next (fp, stype, &i, NULL, NULL, 0)) >= 0) {
>> +    if (ret > mi.ctm_offset)
>> +      fprintf (stderr, "ssize_t return: unexpected offset: %zi\n", ret);

(here.)

-- 
NULL && (void)