libdwfl: Remove asserts from library code

  From 33d436aefb6b63a159943bd24eb432b8cfb8b369 Mon Sep 17 00:00:00 2001
From: Di Chen <dichen@redhat.com>
Date: Sun, 24 Dec 2023 11:44:48 +0800
Subject: [PATCH] libdwfl: Remove asserts from library code

It would be better for elfutils library functions to return an error
code instead of aborting because of a failed assert.

This commit works on removing the asserts under libdwfl. There are two
ways handling the removing:

  1) Replace the asserts with if statements.

  2) Replace the asserts with eu_static_assert.

Asserts are heavily used across all elfutils libraries, and it's
impossibe to implement the removing in one commit. So let's gradually
remove the asserts in the later coming commits.

https://sourceware.org/bugzilla/show_bug.cgi?id=31027

Signed-off-by: Di Chen <dichen@redhat.com>
---
 libdwfl/dwfl_frame.c                 | 14 +++++++++-----
 libdwfl/dwfl_frame_pc.c              |  3 ++-
 libdwfl/dwfl_frame_regs.c            |  7 +++++--
 libdwfl/dwfl_module_build_id.c       |  3 ++-
 libdwfl/dwfl_module_getdwarf.c       |  3 ++-
 libdwfl/dwfl_module_getsrc_file.c    |  3 ++-
 libdwfl/dwfl_module_register_names.c |  3 ++-
 libdwfl/dwfl_segment_report_module.c |  2 +-
 8 files changed, 25 insertions(+), 13 deletions(-)

               void *notes;
               if (ei_data == MY_ELFDATA
  

Hi Di,

Thanks for the patch.

On Sun, Dec 24, 2023 at 2:35 AM Di Chen <dichen@redhat.com> wrote:
>
> From 33d436aefb6b63a159943bd24eb432b8cfb8b369 Mon Sep 17 00:00:00 2001
> From: Di Chen <dichen@redhat.com>
> Date: Sun, 24 Dec 2023 11:44:48 +0800
> Subject: [PATCH] libdwfl: Remove asserts from library code
>
> It would be better for elfutils library functions to return an error
> code instead of aborting because of a failed assert.
>
> This commit works on removing the asserts under libdwfl. There are two
> ways handling the removing:
>
>   1) Replace the asserts with if statements.
>
>   2) Replace the asserts with eu_static_assert.
>
> Asserts are heavily used across all elfutils libraries, and it's
> impossibe to implement the removing in one commit. So let's gradually
> remove the asserts in the later coming commits.

Agreed.

>
> https://sourceware.org/bugzilla/show_bug.cgi?id=31027
>
> Signed-off-by: Di Chen <dichen@redhat.com>
> ---
>  libdwfl/dwfl_frame.c                 | 14 +++++++++-----
>  libdwfl/dwfl_frame_pc.c              |  3 ++-
>  libdwfl/dwfl_frame_regs.c            |  7 +++++--
>  libdwfl/dwfl_module_build_id.c       |  3 ++-
>  libdwfl/dwfl_module_getdwarf.c       |  3 ++-
>  libdwfl/dwfl_module_getsrc_file.c    |  3 ++-
>  libdwfl/dwfl_module_register_names.c |  3 ++-
>  libdwfl/dwfl_segment_report_module.c |  2 +-
>  8 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
> index 5ee71dd4..18150e2a 100644
> --- a/libdwfl/dwfl_frame.c
> +++ b/libdwfl/dwfl_frame.c
> @@ -85,12 +85,14 @@ free_states (Dwfl_Frame *state)
>  static Dwfl_Frame *
>  state_alloc (Dwfl_Thread *thread)
>  {
> -  assert (thread->unwound == NULL);
> +  if (thread->unwound != NULL)
> +    return NULL;
>    Ebl *ebl = thread->process->ebl;
>    size_t nregs = ebl_frame_nregs (ebl);
>    if (nregs == 0)
>      return NULL;
> -  assert (nregs < sizeof (((Dwfl_Frame *) NULL)->regs_set) * 8);
> +  if (nregs >= sizeof (((Dwfl_Frame *) NULL)->regs_set) * 8)
> +    return NULL;

All of these assert removals look good, but it may be helpful if we also
set an error code with __libdwfl_seterrno, at least in the dwfl_frame*.c
functions being modified in this patch.

I think the INVALID_REGISTER error can be used when nregs is too big.
However for 'thread->unwound != NULL' it's not clear to me exactly
which DWFL_ERROR we should use.  The NO_UNWIND error seems applicable
but its description reads "Unwinding not supported for this architecture".
Arch support isn't necessarily the problem here.

Maybe we should add another DWFL_ERROR for these cases, something like
"BAD_FRAME"?  Then we can use this error code when a frame is missing
or its pc_state is corrupt.

>    Dwfl_Frame *state = malloc (sizeof (*state) + sizeof (*state->regs) * nregs);
>    if (state == NULL)
>      return NULL;
> @@ -283,8 +285,9 @@ dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread *thread, void *arg),
>   }
>        int err = callback (&thread, arg);
>        if (err != DWARF_CB_OK)
> - return err;
> -      assert (thread.unwound == NULL);
> +        return err;
> +      if (thread.unwound != NULL)
> +        return -1;
>      }
>    /* NOTREACHED */
>  }
> @@ -450,7 +453,8 @@ dwfl_thread_getframes (Dwfl_Thread *thread,
>        __libdwfl_seterrno (err);
>        return -1;
>      }
> -  assert (state->pc_state == DWFL_FRAME_STATE_PC_UNDEFINED);
> +  if (state->pc_state != DWFL_FRAME_STATE_PC_UNDEFINED)
> +    return -1;
>    free_states (state);
>    return 0;
>  }
> diff --git a/libdwfl/dwfl_frame_pc.c b/libdwfl/dwfl_frame_pc.c
> index 296c815b..b9991e9a 100644
> --- a/libdwfl/dwfl_frame_pc.c
> +++ b/libdwfl/dwfl_frame_pc.c
> @@ -35,7 +35,8 @@
>  bool
>  dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool *isactivation)
>  {
> -  assert (state->pc_state == DWFL_FRAME_STATE_PC_SET);
> +  if (state->pc_state != DWFL_FRAME_STATE_PC_SET)
> +    return false;
>    *pc = state->pc;
>    ebl_normalize_pc (state->thread->process->ebl, pc);
>    if (isactivation)
> diff --git a/libdwfl/dwfl_frame_regs.c b/libdwfl/dwfl_frame_regs.c
> index a4bd3884..064f5e8a 100644
> --- a/libdwfl/dwfl_frame_regs.c
> +++ b/libdwfl/dwfl_frame_regs.c
> @@ -37,8 +37,11 @@ dwfl_thread_state_registers (Dwfl_Thread *thread, int firstreg,
>       unsigned nregs, const Dwarf_Word *regs)
>  {
>    Dwfl_Frame *state = thread->unwound;
> -  assert (state && state->unwound == NULL);
> -  assert (state->initial_frame);
> +  if (state == NULL || state->unwound != NULL || !state->initial_frame)
> +    {
> +    __libdwfl_seterrno (DWFL_E_INVALID_REGISTER);
> +    return false;

Need two more spaces of indentation here.

> +    }
>    for (unsigned regno = firstreg; regno < firstreg + nregs; regno++)
>      if (! __libdwfl_frame_reg_set (state, regno, regs[regno - firstreg]))
>        {
> diff --git a/libdwfl/dwfl_module_build_id.c b/libdwfl/dwfl_module_build_id.c
> index 0c198f23..4dcc046e 100644
> --- a/libdwfl/dwfl_module_build_id.c
> +++ b/libdwfl/dwfl_module_build_id.c
> @@ -65,7 +65,8 @@ __libdwfl_find_build_id (Dwfl_Module *mod, bool set, Elf *elf)
>    int build_id_len;
>
>    /* For mod == NULL use dwelf_elf_gnu_build_id directly.  */
> -  assert (mod != NULL);
> +  if (mod == NULL)
> +    return -1;
>
>    int result = __libdwfl_find_elf_build_id (mod, elf, &build_id_bits,
>      &build_id_elfaddr, &build_id_len);
> diff --git a/libdwfl/dwfl_module_getdwarf.c b/libdwfl/dwfl_module_getdwarf.c
> index 6f98c02b..596de51e 100644
> --- a/libdwfl/dwfl_module_getdwarf.c
> +++ b/libdwfl/dwfl_module_getdwarf.c
> @@ -161,7 +161,8 @@ open_elf (Dwfl_Module *mod, struct dwfl_file *file)
>   mod->e_type = ET_DYN;
>      }
>    else
> -    assert (mod->main.elf != NULL);
> +    if (mod->main.elf == NULL)
> +      return DWFL_E (LIBELF, elf_errno ());
>
>    return DWFL_E_NOERROR;
>  }
> diff --git a/libdwfl/dwfl_module_getsrc_file.c b/libdwfl/dwfl_module_getsrc_file.c
> index fc144225..4456dc58 100644
> --- a/libdwfl/dwfl_module_getsrc_file.c
> +++ b/libdwfl/dwfl_module_getsrc_file.c
> @@ -165,7 +165,8 @@ dwfl_module_getsrc_file (Dwfl_Module *mod,
>
>    if (cur_match > 0)
>      {
> -      assert (*nsrcs == 0 || *srcsp == match);
> +      if (*nsrcs != 0 && *srcsp != match)
> +        return -1;
>
>        *nsrcs = cur_match;
>        *srcsp = match;
> diff --git a/libdwfl/dwfl_module_register_names.c b/libdwfl/dwfl_module_register_names.c
> index 9ea09371..6fb4195b 100644
> --- a/libdwfl/dwfl_module_register_names.c
> +++ b/libdwfl/dwfl_module_register_names.c
> @@ -73,7 +73,8 @@ dwfl_module_register_names (Dwfl_Module *mod,
>   }
>        if (likely (len > 0))
>   {
> -  assert (len > 1); /* Backend should never yield "".  */
> +  if (len <= 1) /* Backend should never yield "".  */
> +      return -1;

The return statement needs to be indented more.

>    result = (*func) (arg, regno, setname, prefix, name, bits, type);
>   }
>      }
> diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c
> index dc34e0ae..39e27e22 100644
> --- a/libdwfl/dwfl_segment_report_module.c
> +++ b/libdwfl/dwfl_segment_report_module.c
> @@ -530,7 +530,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
>        if (filesz > SIZE_MAX / sizeof (Elf32_Nhdr))
>   continue;
>
> -              assert (sizeof (Elf32_Nhdr) == sizeof (Elf64_Nhdr));
> +              eu_static_assert (sizeof (Elf32_Nhdr) == sizeof (Elf64_Nhdr));
>
>                void *notes;
>                if (ei_data == MY_ELFDATA
> --
> 2.41.0
>

Aaron