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

[Torbjorn SVENSSON <torbjorn.svensson@foss.st.com>]

On 2023-10-15 21:18, Nick Alcock wrote:
> 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).

Right.

> 
>>> 	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.

Thanks for the pointer. I found that I had replaced some (unsigned 
long)-1 with CTF_ERR that should be exactly that. V8 is reverting those 
chunks to be like they were before.

>>> (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.

I tried to compile an exhaustive list, but it was too big for my taste, 
so I went with your proposal (part of V8).

> 
>>> 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.)

Ah, okay. In any case, I think it would be clearer if you get all the 
lines in one go that fails than just the first one (in case of multiple 
failures...). - But, that's only my 2 cents.