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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Concurrent calls to `setenv()` cause undefined behavior and Use-After-Free crashes.
- [High] Concurrent modifications to process-global terminal settings and signal handlers cause permanent corruption.
- [Medium] Data races on static variables (`debuginfod_fetch_cancelled`, `debuginfod_signal`, `debuginfod_progress_started`) corrupt concurrent fetches.
- [Medium] Intercepted signals (e.g., SIGINT) are swallowed on the success path, preventing graceful termination.
- [Low] Missing explicit inclusion of `<limits.h>` for `PATH_MAX`, causing musl libc compilation failures.
--

commit 93a135dd9f8083fd8291b38e49e5952c517a78da
Author: Arnaldo Carvalho de Melo [off-list ref]

perf debuginfo: Fetch debuginfo keyed by build ID using debuginfod

This commit adds support for fetching debuginfo keyed by build ID from
debuginfod servers. It integrates this into the DSO debuginfo resolution
path as a fallback when local debuginfo is missing.

[ ... ]

[ ... ]

+static void debuginfod__setup_urls_env(void)
+{
+	char *urls = NULL;
+	DIR *dir;
+	struct dirent *dent;
+
+	if (getenv("DEBUGINFOD_URLS") != NULL)
+		return;
+
+	dir = opendir("/etc/debuginfod");
+	if (dir == NULL)
+		return;
+
+	while ((dent = readdir(dir)) != NULL) {
+		char *content = NULL;
+		char *new_urls;
+		char path[PATH_MAX];

[Severity: Low]
Does the use of PATH_MAX without an explicit inclusion of <limits.h> cause
build failures on systems using musl libc?

+		size_t len = strlen(dent->d_name), i, size;
+		int n;

[ ... ]

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

[Severity: High]
Since dso__debuginfo() explicitly drops the dso__lock before falling back to
fetch debuginfo, we can have a call chain like this executed concurrently:

Thread 1 & 2:
dso__debuginfo() -> drops lock
  debuginfo__new_build_id()
    debuginfo__find_build_id()
      debuginfod__setup_urls_env()
        setenv()

Can concurrent threads resolving DSOs call setenv() simultaneously here? Because
setenv() is thread-unsafe, this concurrent access could corrupt the process-global
environment array and cause a crash.

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

[ ... ]

+int debuginfo__find_build_id(const struct build_id *bid, char **path)
+{
+	char sbuild_id[SBUILD_ID_SIZE];
+	struct termios orig_termios;
+	struct sigaction sa, orig_sigint, orig_sigterm;
+	bool term_set = false, sigint_set = false, sigterm_set = false;
+	debuginfod_client *c;
+	int fd;

[ ... ]

+	debuginfod_fetch_cancelled = false;
+	debuginfod_signal = 0;

[Severity: Medium]
These static global variables are shared across all threads but modified here
without synchronization. If a second thread starts a fetch, could it
unconditionally reset debuginfod_fetch_cancelled to false and cause a concurrent
thread attempting to cancel its blocked fetch to hang indefinitely?

+
+	/*
+	 * Make stdin deliver keypresses without waiting for a newline,
+	 * the progress callback above polls it for the 's'/'d' keys,

[ ... ]

+	if (isatty(STDIN_FILENO) && isatty(STDERR_FILENO) && !use_browser) {
+		set_term_quiet_input(&orig_termios);
+		term_set = true;
+
+		memset(&sa, 0, sizeof(sa));
+		sa.sa_handler = debuginfod_signal_handler;
+		sigemptyset(&sa.sa_mask);
+		if (sigaction(SIGINT, &sa, &orig_sigint) == 0)
+			sigint_set = true;
+		if (sigaction(SIGTERM, &sa, &orig_sigterm) == 0)
+			sigterm_set = true;
+	}

[Severity: High]
Following the same concurrent execution path from dso__debuginfo(), can multiple
threads concurrently modify the process-wide terminal attributes and signal
handlers here? 

If Thread B captures Thread A's temporary raw state into orig_termios, could it
eventually restore that state permanently and leave the process terminal broken
after the fetches complete?

+
+	fd = debuginfod_find_debuginfo(c, bid->data, bid->size, path);
+
+	if (term_set)
+		tcsetattr(STDIN_FILENO, TCSANOW, &orig_termios);
+	if (sigint_set)
+		sigaction(SIGINT, &orig_sigint, NULL);
+	if (sigterm_set)
+		sigaction(SIGTERM, &orig_sigterm, NULL);
+
+	debuginfod_end(c);
+	if (debuginfod_progress_started) {
+		fputc('\n', stderr);
+		debuginfod_progress_started = false;
+	}
+	if (fd < 0) {
+		build_id__snprintf(bid, sbuild_id, sizeof(sbuild_id));
+		if (debuginfod_fetch_cancelled || debuginfod_signal) {
+			pr_debug("debuginfod search for build ID %s cancelled by the user\n",
+				 sbuild_id);
+			/*
+			 * The terminal is restored, die as the user asked;
+			 * the original dispositions are back in place.
+			 */
+			if (debuginfod_signal)
+				raise(debuginfod_signal);
+			return -1;
+		}
+		pr_debug("No debuginfo found for build ID %s in debuginfod\n",
+			 sbuild_id);
+		debuginfod__miss_add(bid);
+		return -1;
+	}
+
+	close(fd);
+	return 0;
+}

[Severity: Medium]
On the success path when fd is valid, the code returns 0 and skips evaluating
and re-raising debuginfod_signal. Does this mean user interrupt requests like
SIGINT are silently swallowed if the user presses Ctrl-C right as the fetch is
concluding successfully?

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