Re: [PATCH v3 3/6] perf libdw: Fix libdw API contract violations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Incomplete error handling makes the newly added `args.err` field entirely dead code, silently swallowing inline unwinding memory errors.
- [Medium] The new bounds check `call_lineno > 0` incorrectly discards valid DWARF line `0` references, leading to lost caller filenames.
- [Medium] The commit message falsely claims the patch prevents a `strdup(NULL)` crash in `new_inline_sym`, justifying the change on an incorrect premise.
--

commit 5e576062601ce3a0337e76030c663f3ce42bdf61
Author: Ian Rogers <irogers@google.com>

perf libdw: Fix libdw API contract violations

This commit adds return value checks for several libdw API calls. It also
introduces an error field to propagate inline unwinding failures and
filters caller line numbers to avoid negative DWARF error codes.

> Check return values of dwfl_report_end and dwfl_module_addrdie.
> Validate dwarf_diename result before passing to new_inline_sym (avoid
> potential strdup(NULL) crash) and check die_get_call_lineno for
> errors.

This isn't a bug, but is the description of the crash risk accurate?
Looking at new_inline_sym(), it already guards against NULL pointers
by defaulting to a placeholder string:

static struct symbol *new_inline_sym(struct dso *dso,
                                     struct symbol *base_sym,
                                     const char *funcname)
{
        ...
        if (!funcname)
                funcname = "??";

It appears the patch changes the fallback string from "??" to "unknown"
but wouldn't actually prevent a crash since NULL is already handled natively.

> diff --git a/tools/perf/util/libdw.c b/tools/perf/util/libdw.c
> index 216977884103..e37f3b22699d 100644
> --- a/tools/perf/util/libdw.c
> +++ b/tools/perf/util/libdw.c

[ ... ]

> @@ -72,22 +76,27 @@ struct libdw_a2l_cb_args {
>  static int libdw_a2l_cb(Dwarf_Die *die, void *_args)
>  {
>  	struct libdw_a2l_cb_args *args  = _args;
> -	struct symbol *inline_sym = new_inline_sym(args->dso, args->sym, dwarf_diename(die));
> +	const char *name = dwarf_diename(die);
> +	struct symbol *inline_sym = new_inline_sym(args->dso, args->sym, name ?: "unknown");
>  	const char *call_fname = die_get_call_file(die);
> +	int call_lineno = die_get_call_lineno(die);
>  	char *call_srcline = srcline__unknown;
>  	struct inline_list *ilist;
>  
> -	if (!inline_sym)
> -		return -ENOMEM;
> +	if (!inline_sym) {
> +		args->err = -ENOMEM;
> +		return DWARF_CB_ABORT;
> +	}
>  
>  	/* Assign caller information to the parent. */
> -	if (call_fname)
> -		call_srcline = srcline_from_fileline(call_fname, die_get_call_lineno(die));
> +	if (call_fname && call_lineno > 0)

Does this condition inadvertently drop valid DWARF line 0 references?

In the DWARF standard, line 0 is a valid identifier used for
compiler-generated code that lacks a specific source line, but the
associated file name remains valid. By requiring the line number to
be strictly greater than 0, it seems we might discard successfully
retrieved caller filenames for these entries.

> +		call_srcline = srcline_from_fileline(call_fname, call_lineno);
>  
>  	list_for_each_entry(ilist, &args->node->val, list) {
>  		if (args->leaf_srcline == ilist->srcline)

[ ... ]

> @@ -163,7 +172,8 @@ int libdw__addr2line(u64 addr, char **file, unsigned int *line_nr,
>  		};
>  
>  		/* Walk from the parent down to the leaf. */
> -		cu_walk_functions_at(cudie, addr, libdw_a2l_cb, &args);
> +		if (cudie)
> +			cu_walk_functions_at(cudie, addr, libdw_a2l_cb, &args);
>  
>  		if (!args.leaf_srcline_used)
>  			free(args.leaf_srcline);

Is there a missing check for args.err after this walk?

The callback libdw_a2l_cb() was updated to set args.err = -ENOMEM and
return DWARF_CB_ABORT on allocation failure. However,
cu_walk_functions_at() is called here without inspecting its return
value or args.err, and libdw__addr2line() eventually returns 1
(success) regardless.

This appears to leave args.err as dead code and silently swallows the
memory allocation error, returning a truncated inline callchain to
the caller.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260503003552.1063540-1-irogers@google.com?part=3