Re: [PATCH v2 3/3] perf symbol: Set dso symtab type for libbfd

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf: double-free and state confusion in libbfd fallback path
--

commit 8791c82124897f35132a0fe029d993b2dcd4a6bc
Author: Namhyung Kim <namhyung@kernel.org>

perf symbol: Set dso symtab type for libbfd

It missed to set the dso symtab type when the symbols are loaded by
libbfd.  When it happens both syms_ss and runtime_ss may not be set
and won't call dso__load_sym().

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 5e63c3477570c..debd0a66f9669 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1929,6 +1929,7 @@ int dso__load(struct dso *dso, struct map *map)
>  				symsrc__destroy(syms_ss);
>  				syms_ss = NULL;
>  			}
> +			dso__set_symtab_type(dso, symtab_type);
>  			ret = 1;
>  			break;
>  		}

[Severity: High]
When dso__load_bfd_symbols() succeeds and sets ret = 1, this explicit
destruction of syms_ss does not remove the struct from the ss_ array because
ss_pos remains unmodified.

Does this lead to a double-free later in the cleanup loop?

tools/perf/util/symbol.c:dso__load() {
    ...
	for (; ss_pos > 0; ss_pos--)
		symsrc__destroy(&ss_[ss_pos - 1]);
    ...
}

Additionally, since runtime_ss is not cleared here, can execution incorrectly
fall through to dso__load_sym() and overwrite the successful ret = 1 value
from libbfd?

tools/perf/util/symbol.c:dso__load() {
    ...
	if (runtime_ss && !syms_ss) {
		syms_ss = runtime_ss;
	}

	/* We'll have to hope for the best */
	if (!runtime_ss && syms_ss)
		runtime_ss = syms_ss;

	if (syms_ss)
		ret = dso__load_sym(dso, map, syms_ss, runtime_ss, kmod);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010182439.9732-1-namhyung@kernel.org?part=3