Re: [PATCH v4 2/6] perf dwarf-aux: Fix libdw API contract violations

Namhyung Kim
2026-05-03T23:36:10Z

On Sun, May 03, 2026 at 10:10:28AM -0700, Ian Rogers wrote:
> Check return values of `dwarf_decl_line` (where non-optional),
> `dwarf_getfuncs`, and `dwarf_lineaddr` to prevent using uninitialized
> stack variables or incorrectly reporting success on failure.
>
> For the root DIE in `die_walk_lines()`, `dwarf_decl_line` and
> `die_get_decl_file` are optional and their failures are handled
> gracefully to avoid breaking line walking on valid functions.
>
> Additionally:
> - Add NULL pointer protection for `strcmp()` in `die_walk_lines()`
> when `inf` or `decf` are NULL to prevent crashes on generated
> code.
> - Use `dwarf_attr_integrate` in `die_get_data_member_location` to
> correctly resolve inherited member locations (e.g. via abstract
> origin or specification).
>
> Fixes: 57f95bf5f882 ("perf probe: Show correct statement line number by perf probe -l")
> Fixes: 3f4460a28fb2 ("perf probe: Filter out redundant inline-instances")
> Fixes: 75186a9b09e4 ("perf probe: Fix to show lines of sys_ functions correctly")
> Fixes: e0d153c69040 ("perf-probe: Move dwarf library routines to dwarf-aux.{c, h}")
> Fixes: 6243b9dc4c99 ("perf probe: Move dwarf specific functions to dwarf-aux.c")
> Assisted-by: Gemini-CLI:Google Gemini 3
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> v4:
> - Fix strcmp(NULL) risk and inherited member location fallbacks in dwarf-aux.c.
> ---
> tools/perf/util/dwarf-aux.c | 34 ++++++++++++++++------------------
> tools/perf/util/dwarf-aux.h | 5 +++++
> 2 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 109a166a6d19..a8ab1c30d0ac 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -125,7 +125,8 @@ int cu_find_lineinfo(Dwarf_Die *cu_die, Dwarf_Addr addr,
> && die_entrypc(&die_mem, &faddr) == 0 &&
> faddr == addr) {
> *fname = die_get_decl_file(&die_mem);
> - dwarf_decl_line(&die_mem, lineno);
> + if (dwarf_decl_line(&die_mem, lineno) != 0)
> + return -ENOENT;
> goto out;
> }
>
> @@ -459,7 +460,7 @@ int die_get_data_member_location(Dwarf_Die *mb_die, Dwarf_Word *offs)
> size_t nexpr;
> int ret;
>
> - if (dwarf_attr(mb_die, DW_AT_data_member_location, &attr) == NULL)
> + if (dwarf_attr_integrate(mb_die, DW_AT_data_member_location, &attr) == NULL)
> return -ENOENT;
>
> if (dwarf_formudata(&attr, offs) != 0) {
> @@ -795,8 +796,7 @@ static int __die_walk_instances_cb(Dwarf_Die *inst, void *data)
>
> /* Ignore redundant instances */
> if (dwarf_tag(inst) == DW_TAG_inlined_subroutine) {
> - dwarf_decl_line(origin, &tmp);
> - if (die_get_call_lineno(inst) == tmp) {
> + if (dwarf_decl_line(origin, &tmp) == 0 && die_get_call_lineno(inst) == tmp) {
> tmp = die_get_decl_fileno(origin);
> if (die_get_call_fileno(inst) == tmp)
> return DIE_FIND_CB_CONTINUE;
> @@ -950,11 +950,6 @@ int die_walk_lines(Dwarf_Die *rt_die, line_walk_callback_t callback, void *data)
> cu_die = dwarf_diecu(rt_die, &die_mem, NULL, NULL);
> dwarf_decl_line(rt_die, &decl);
> decf = die_get_decl_file(rt_die);
> - if (!decf) {
> - pr_debug2("Failed to get the declared file name of %s\n",
> - dwarf_diename(rt_die));
> - return -EINVAL;
> - }

Why is this part removed?

> } else
> cu_die = rt_die;
> if (!cu_die) {
> @@ -998,12 +993,11 @@ int die_walk_lines(Dwarf_Die *rt_die, line_walk_callback_t callback, void *data)
> if (die_find_inlinefunc(rt_die, addr, &die_mem)) {
> /* Call-site check */
> inf = die_get_call_file(&die_mem);
> - if ((inf && !strcmp(inf, decf)) &&
> + if ((inf == decf || (inf && decf && !strcmp(inf, decf))) &&
> die_get_call_lineno(&die_mem) == lineno)
> goto found;
>
> - dwarf_decl_line(&die_mem, &inl);
> - if (inl != decl ||
> + if (dwarf_decl_line(&die_mem, &inl) != 0 || inl != decl ||
> decf != die_get_decl_file(&die_mem))
> continue;
> }
> @@ -1034,8 +1028,10 @@ int die_walk_lines(Dwarf_Die *rt_die, line_walk_callback_t callback, void *data)
> .data = data,
> .retval = 0,
> };
> - dwarf_getfuncs(cu_die, __die_walk_culines_cb, &param, 0);
> - ret = param.retval;
> + if (dwarf_getfuncs(cu_die, __die_walk_culines_cb, &param, 0) < 0)
> + ret = -EINVAL;
> + else
> + ret = param.retval;
> }
>
> return ret;
> @@ -1939,10 +1935,12 @@ static bool die_get_postprologue_addr(unsigned long entrypc_idx,
> break;
> }
>
> - dwarf_lineaddr(line, postprologue_addr);
> - if (*postprologue_addr >= highpc)
> - dwarf_lineaddr(dwarf_onesrcline(lines, i - 1),
> - postprologue_addr);
> + if (dwarf_lineaddr(line, postprologue_addr) != 0)
> + return false;
> + if (*postprologue_addr >= highpc) {
> + if (dwarf_lineaddr(dwarf_onesrcline(lines, i - 1), postprologue_addr) != 0)
> + return false;
> + }
>
> return true;
> }
> diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
> index a79968a2e573..161f0bf980b6 100644
> --- a/tools/perf/util/dwarf-aux.h
> +++ b/tools/perf/util/dwarf-aux.h
> @@ -10,6 +10,11 @@
> #include <elfutils/libdwfl.h>
> #include <elfutils/version.h>
>
> +static inline const char *die_name(Dwarf_Die *die)
> +{
> + return dwarf_diename(die) ?: "<unknown>";
> +}
> +

Please move it where it's used.

Thanks,
Namhyung

> struct strbuf;
>
> /* Find the realpath of the target file */
> --
> 2.54.0.545.g6539524ca2-goog
>