Re: [PATCH v4 11/23] perf annotate: Move struct annotation to a hashmap and remove symbol_conf.priv_size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] perf annotate: stale empty_annotation pointer in
  symbol__annotate2()
--

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

perf annotate: Move struct annotation to a hashmap and remove symbol_conf.priv_size

This commit moves struct annotation instances from negative-offset space
before struct symbol into an on-demand hashmap keyed by the symbol pointer,
and removes symbol_conf.priv_size.

> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 529d27e2ef636..e907a83e1f27a 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
[ ... ]
> @@ -157,9 +162,130 @@ annotated_source__hist_entry(struct annotated_source *src, const struct evsel *e
[ ... ]
> +struct annotation *symbol__annotation(struct symbol *sym)
> +{
> +	struct annotation *notes = NULL;
> +	long val = 0;
> +
> +	if (!sym)
> +		return NULL;
> +
> +	/* Fast path: look up existing annotation under read lock if flagged. */
> +	if (symbol__annotated(sym)) {
> +		down_read(annotations_lock());
> +		if (annotations && hashmap__find(annotations, (long)sym, &val))
> +			notes = (struct annotation *)val;
> +		up_read(annotations_lock());
> +		if (notes)
> +			return notes;
> +	}
> +
> +	down_write(annotations_lock());
> +	if (!annotations) {
> +		annotations = hashmap__new(sym_ptr_hash, sym_ptr_equal, NULL);
> +		if (IS_ERR(annotations)) {
> +			annotations = NULL;
> +			up_write(annotations_lock());
> +			return &empty_annotation;
> +		}
> +	} else if (hashmap__find(annotations, (long)sym, &val)) {
> +		notes = (struct annotation *)val;
> +		symbol__set_annotated(sym, true);
> +		up_write(annotations_lock());
> +		return notes;
> +	}
> +
> +	notes = zalloc(sizeof(*notes));
> +	if (!notes)
> +		goto err_free_hashmap;
> +
> +	annotation__init(notes);
> +	if (hashmap__add(annotations, (long)sym, notes) < 0) {
> +		annotation__exit(notes);
> +		free(notes);
> +		goto err_free_hashmap;
> +	}
> +	symbol__set_annotated(sym, true);
> +	up_write(annotations_lock());
> +	return notes;
> +
> +err_free_hashmap:
> +	if (hashmap__size(annotations) == 0) {
> +		hashmap__free(annotations);
> +		annotations = NULL;
> +	}
> +	up_write(annotations_lock());
> +	return &empty_annotation;
> +}

[Severity: Low]
Can returning &empty_annotation without inserting sym into annotations cause a
NULL pointer dereference in symbol__annotate2()?

tools/perf/util/annotate.c:symbol__annotate2() {
	struct symbol *sym = ms->sym;
	struct annotation *notes = symbol__annotation(sym);
	size_t size = symbol__size(sym);
	int err;

	err = symbol__annotate(ms, evsel, parch);
	if (err)
		return err;

	symbol__calc_percent(sym, evsel);

	annotation__set_index(notes);

If the first symbol__annotation(sym) call in symbol__annotate2() fails to
allocate and returns &empty_annotation, sym is not added to the hashmap. If
the second symbol__annotation(sym) call inside symbol__annotate() then
succeeds, symbol__annotate() populates the newly allocated annotation and
returns 0, while symbol__annotate2() passes its cached &empty_annotation
pointer (with notes->src == NULL) to annotation__set_index(notes).

For the same reason, callers such as perf_top__parse_source() that lock the
sharded mutex for notes = symbol__annotation(sym) before calling
symbol__hists(sym, ...) will lock &empty_annotation while symbol__hists()
allocates and mutates a different struct annotation instance unlocked.

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