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

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

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.

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

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

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?


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

> 
> diff --git a/libctf/testsuite/libctf-writable/libctf-errors.c b/libctf/testsuite/libctf-writable/libctf-errors.c
> new file mode 100644
> index 00000000000..71f8268cfad
> --- /dev/null
> +++ b/libctf/testsuite/libctf-writable/libctf-errors.c
> @@ -0,0 +1,74 @@
> +/* Make sure that error returns are correct.  Usually this is trivially
> +   true, but on platforms with unusual type sizes all the casting might
> +   cause problems with unexpected sign-extension and truncation.  */
> +
> +#include <ctf-api.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  ctf_dict_t *fp;
> +  ctf_next_t *i = NULL;
> +  size_t boom = 0;
> +  ctf_id_t itype, stype;
> +  ctf_encoding_t encoding = {0};
> +  ctf_membinfo_t mi;
> +  ssize_t ret;
> +  int err;
> +
> +  if ((fp = ctf_create (&err)) == NULL)
> +    {
> +      fprintf (stderr, "%s: cannot create: %s\n", argv[0], ctf_errmsg (err));
> +      return 1;
> +    }
> +
> +  /* First error class: int return.  */
> +
> +  if (ctf_member_count (fp, 1024) >= 0)
> +    fprintf (stderr, "int return: non-error return: %i\n",
> +             ctf_member_count(fp, 1024));
> +
> +  /* Second error class: type ID return.  */
> +
> +  if (ctf_type_reference (fp, 1024) != CTF_ERR)
> +    fprintf (stderr, "ctf_id_t return: non-error return: %li\n",
> +             ctf_type_reference (fp, 1024));
> +
> +  /* 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?

> +
> +  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);
> +    if (boom++ > 1000)
> +      {
> +        fprintf (stderr, "member iteration went on way too long\n");
> +        exit (1);
> +      }
> +  }
> +
> +  /* Fourth error class (trivial): pointer return.  */
> +  if (ctf_type_aname (fp, 1024) != NULL)
> +    fprintf (stderr, "pointer return: non-error return: %p\n",
> +             ctf_type_aname (fp, 1024));
> +
> +  ctf_file_close (fp);
> +
> +  printf("All done.\n");
> +
> +  return 0;
> +}
> diff --git a/libctf/testsuite/libctf-writable/libctf-errors.lk b/libctf/testsuite/libctf-writable/libctf-errors.lk
> new file mode 100644
> index 00000000000..b944f73d013
> --- /dev/null
> +++ b/libctf/testsuite/libctf-writable/libctf-errors.lk
> @@ -0,0 +1 @@
> +All done.
>