Re: [PATCH 2/8] perf debuginfo: Fetch debuginfo keyed by build ID using debuginfod

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Thread-unsafe modification of process environment via `setenv` races with concurrent `getenv` calls.
- [Medium] Over-scoping of `debuginfod__fetch_lock` neutralizes concurrent lookup logic and makes condition variables dead code.
--

commit 61a7d2bee4ec3dc777a819193cbb18e4e7f04543
Author: Arnaldo Carvalho de Melo [off-list ref]
Subject: perf debuginfo: Fetch debuginfo keyed by build ID using debuginfod

This commit adds debuginfo__find_build_id() to fetch debuginfo files via
debuginfod based on their build ID. It falls back to debuginfod servers if
local debuginfo is missing, supporting cancellation and progress reporting.

[ ... ]

[ ... ]

+static void debuginfod__urls_env_setup(void)
+{
+	char *urls = NULL;

[ ... ]

+	if (urls != NULL) {
+		setenv("DEBUGINFOD_URLS", urls, 1);

[Severity: High]
Does this thread-unsafe modification of the process environment via setenv()
race with concurrent getenv() calls?

This lazy initialization is triggered during active DSO processing in
debuginfod__fetch(). In multi-threaded commands like 'perf top', one thread
might perform a fetch while another thread simultaneously uses libdw to
process a different DSO (which calls getenv() inside elfutils). This data
race on the environ pointer could lead to memory corruption, use-after-free,
or segmentation faults.

+		pr_debug("Set DEBUGINFOD_URLS from /etc/debuginfod: %s\n", urls);
+	}
+	free(urls);
+}

[ ... ]

+static void debuginfod__miss_add(const struct build_id *bid, bool cancelled)
+{
+	struct debuginfod_miss *miss = zalloc(sizeof(*miss));
+
+	if (miss == NULL)
+		return;
+
+	miss->bid = *bid;
+	miss->cancelled = cancelled;
+
+	debuginfod__missed_lock_init();
+	mutex_lock(&debuginfod__missed_lock);

[Severity: Medium]
Is this inner lock redundant?

Since debuginfod__miss_add() is called from debuginfod__fetch() which is in
turn called from debuginfo_lookup__find_build_id(), the execution always
happens while debuginfod__fetch_lock is already held (acquired in
debuginfo__find_build_id()).

+	list_add(&miss->node, &debuginfod__misses);
+	mutex_unlock(&debuginfod__missed_lock);
+}

[ ... ]

+static int debuginfo_lookup__find_build_id(const struct build_id *bid, char **path)
+{

[ ... ]

+	/*
+	 * Somebody else got here first: wait for the fetch that is in
+	 * progress instead of starting another one, which, while that one
+	 * is still running, would download the same file a second time.
+	 */
+	if (lookup->fetching) {
+		lookup->nr_waiters++;
+		waited = true;
+
+		while (lookup->fetching)
+			cond_wait(&lookup->done, &debuginfod__fetch_lock);

[Severity: Medium]
Is this condition wait loop unreachable?

The outer lock (debuginfod__fetch_lock) is acquired in
debuginfo__find_build_id() before initiating any lookup or fetch,
encompassing the entire blocking network operation. This prevents any
concurrent thread from reaching this wait logic while fetching is in
progress, making the intended fine-grained condition variable logic dead
code and forcing fetches for different build IDs to unnecessarily block
each other.

+	}

[ ... ]

+int debuginfo__find_build_id(const struct build_id *bid, char **path)
+{

[ ... ]

+	/*
+	 * The checks below have to be made with the lock held, as they look
+	 * at the state the fetch changes: debuginfod can be turned off while
+	 * a fetch is in progress, by the 'd' key in its progress line, and a
+	 * build ID the fetch in progress just settled, as a miss or as a
+	 * cancellation, is settled for whoever is waiting for the lock as
+	 * well.  Deciding here and fetching there would repeat a fetch that
+	 * was already made, and put the same build ID on the misses list
+	 * twice.
+	 */
+	debuginfod__fetch_lock_init();
+	mutex_lock(&debuginfod__fetch_lock);
+
+	if (symbol_conf.debuginfod) {
+		bool cancelled;
+
+		if (debuginfod__cache_disabled()) {
+			pr_debug("Build-id cache disabled (buildid dir is '%s'), not using debuginfod\n",
+				 buildid_dir);
+		} else if (debuginfod__missed(bid, &cancelled)) {

[ ... ]

+		} else {
+			err = debuginfo_lookup__find_build_id(bid, path);
+		}
+	}
+
+	mutex_unlock(&debuginfod__fetch_lock);
+
+	return err;
+}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913222821.3353-1-acme@kernel.org?part=2