From 5059119151cc51e33bc4e7deeaf33a0ee17bb26c Mon Sep 17 00:00:00 2001 From: lucannez64 <58551020+lucannez64@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:05:26 +0100 Subject: [PATCH] fix: redact Riot tokens and make session cleanup preserve process identity Signed-off-by: lucannez64 <58551020+lucannez64@users.noreply.github.com> --- .github/workflows/source-check.yml | 4 ++ docs/TROUBLESHOOTING.md | 2 + tests/test_crash_report.py | 20 +++++++ tests/test_session_supervisor.py | 78 ++++++++++++++++++++++++++++ tools/crash-report.py | 18 +++---- tools/riot-launcher-offline-check.py | 13 +++++ tools/run-riot-client.sh | 4 +- tools/session-supervisor.py | 76 ++++++++++++++++++++++----- 8 files changed, 192 insertions(+), 23 deletions(-) diff --git a/.github/workflows/source-check.yml b/.github/workflows/source-check.yml index 1f68e08..e004619 100644 --- a/.github/workflows/source-check.yml +++ b/.github/workflows/source-check.yml @@ -45,3 +45,7 @@ jobs: - run: python -m unittest discover -s tests -p 'test_client_in_game_tricks.py' - run: python -m unittest discover -s tests -p 'test_setup_portability.py' - run: python -m unittest discover -s tests -p 'test_preflight.py' + - run: python -m unittest discover -s tests -p 'test_crash_report.py' + - run: python -m unittest discover -s tests -p 'test_doctor.py' + - run: python -m unittest discover -s tests -p 'test_perf_baseline.py' + - run: python -m unittest discover -s tests -p 'test_session_supervisor.py' diff --git a/docs/TROUBLESHOOTING.md b/docs/TROUBLESHOOTING.md index fe753de..7ad29a7 100644 --- a/docs/TROUBLESHOOTING.md +++ b/docs/TROUBLESHOOTING.md @@ -257,6 +257,8 @@ tree running. The supervisor: - leaves everything as it was with `RCLOL_CLEAN_ON_EXIT=0`, which is the old `exec` behaviour. +Runtime ownership is captured before startup helpers run, so a certificate refresh that starts Darling still belongs to this launcher. Leftover process cleanup checks the original start time, UID and PID namespace and sends signals through pidfds; if a process cannot be pinned, it is left alone and a surviving process is reported as a cleanup failure. + Closing the client window is not the same as quitting the client. The window is gone but `RiotClientServices`, `LeagueClientUx` and their CEF helpers stay resident, and `xwininfo` can still list their windows. If closing the window diff --git a/tests/test_crash_report.py b/tests/test_crash_report.py index dea1b7e..e3c9dc3 100644 --- a/tests/test_crash_report.py +++ b/tests/test_crash_report.py @@ -102,6 +102,15 @@ class HelperTests(unittest.TestCase): self.assertNotIn('xyz', masked) self.assertIn('--region=EUW', masked) + def test_qualified_credential_flags_in_both_formats(self): + for flag in ('--remoting-auth-token', '--riotclient-auth-token', + '--access_token', '--client-password', '--api-key', '--authToken'): + for arguments in ([flag + '=synthetic-value'], [flag, 'synthetic-value']): + with self.subTest(arguments=arguments): + masked = crash_report.mask_argv(['LeagueClientUx', *arguments, '--region=EUW']) + self.assertNotIn('synthetic-value', masked) + self.assertIn('--region=EUW', masked) + def test_scrub_uses_whole_tokens(self): self.assertEqual( crash_report.scrub('al and alps at /mnt/al on al-host', home=Path('/mnt/al'), @@ -197,6 +206,17 @@ class BuildTests(unittest.TestCase): self.assertNotIn('do-not-publish', combined) self.assertIn('~/.darling running', combined) + def test_bundle_omits_riot_command_line_tokens(self): + make_process(self.proc, 202, 200, 'S', 'LeagueClientUx', + ['LeagueClientUx', '--remoting-auth-token=synthetic-remote', + '--riotclient-auth-token', 'synthetic-riot']) + archive, _ = self.build() + with tarfile.open(archive) as tar: + text = tar.extractfile('lol-report/processes.txt').read().decode() + self.assertIn('LeagueClientUx', text) + self.assertNotIn('synthetic-remote', text) + self.assertNotIn('synthetic-riot', text) + def test_workspace_paths_are_redacted(self): archive, _ = self.build(launch_log=None, env=dict(self.env, DARLING_WORKSPACE=str(self.root / 'work'))) diff --git a/tests/test_session_supervisor.py b/tests/test_session_supervisor.py index a2f5d8f..e6ebc16 100644 --- a/tests/test_session_supervisor.py +++ b/tests/test_session_supervisor.py @@ -9,6 +9,9 @@ import tempfile import threading import time import unittest +from unittest import mock +import subprocess +import shutil ROOT = Path(__file__).resolve().parents[1] spec = importlib.util.spec_from_file_location( @@ -116,6 +119,63 @@ class CleanupTests(unittest.TestCase): (10, supervisor.signal.SIGTERM)]) self.assertIn('guest session stopped', self.output.getvalue()) + def test_pid_reused_during_shutdown_is_never_signalled(self): + proc = self.process(10) + def shutdown(*_args, **_kwargs): + shutil.rmtree(proc) + fake_process(self.root, 10, ns=self.own, comm='host-editor') + killed = [] + remaining = supervisor.cleanup( + self.prefix, 'darling', self.ns, proc_root=self.root, + run=shutdown, kill=lambda *args: killed.append(args), + sleep=lambda _: None, out=self.output, own_namespace=self.own, + term_grace=0, kill_grace=0) + self.assertEqual(killed, []) + self.assertEqual(remaining, []) + + def test_pid_reused_after_term_is_not_killed(self): + proc = self.process(10) + killed = [] + def kill(pid, signum): + killed.append(signum) + fields = (proc / 'stat').read_text().split() + fields[-1] = '200' + (proc / 'stat').write_text(' '.join(fields)) + remaining = supervisor.stop_processes( + [(10, proc)], kill=kill, proc_root=self.root, + term_grace=0, kill_grace=0) + self.assertEqual(killed, [signal.SIGTERM]) + self.assertEqual(remaining, []) + + def test_pidfd_is_checked_after_open_and_closed_on_identity_change(self): + proc = self.process(10) + identity = supervisor.process_identity(proc) + def opened(_pid): + (proc / 'ns/pid').unlink() + os.symlink(self.own, proc / 'ns/pid') + return 123 + with mock.patch.object(supervisor.os, 'pidfd_open', side_effect=opened), \ + mock.patch.object(supervisor.signal, 'pidfd_send_signal') as send, \ + mock.patch.object(supervisor.os, 'close') as close: + supervisor.signal_process(10, proc, identity, signal.SIGTERM) + send.assert_not_called() + close.assert_called_once_with(123) + + def test_unavailable_pidfd_never_falls_back_to_numeric_pid(self): + proc = self.process(10) + with mock.patch.object(supervisor.os, 'pidfd_open', side_effect=OSError), \ + mock.patch.object(supervisor.signal, 'pidfd_send_signal') as send: + supervisor.signal_process(10, proc, supervisor.process_identity(proc), signal.SIGTERM) + send.assert_not_called() + + def test_real_pidfd_stops_only_the_selected_child(self): + child = subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(30)']) + self.addCleanup(child.wait) + self.addCleanup(lambda: child.poll() is None and child.kill()) + proc = Path('/proc') / str(child.pid) + supervisor.signal_process(child.pid, proc, supervisor.process_identity(proc), signal.SIGTERM) + self.assertEqual(child.wait(timeout=5), -signal.SIGTERM) + def test_cleanup_does_nothing_without_attribution(self): output = io.StringIO() remaining = supervisor.cleanup( @@ -210,6 +270,24 @@ class SupervisorModeTests(unittest.TestCase): self.assertIn('another session owns this runtime', output) self.assertNotIn('guest session stopped', output) + def test_runtime_started_by_startup_helper_is_owned(self): + proc = fake_process(self.proc, 10, comm='darlingserver') + (proc / 'cmdline').write_bytes(b'darlingserver\0' + os.fsencode(self.prefix) + b'\0') + (self.prefix / '.init.pid').write_text('10') + with mock.patch.object(supervisor, 'cleanup', return_value=[]) as cleanup: + code = supervisor.run_supervised( + ['true'], self.prefix, None, proc_root=self.proc, poll=0, + sleep=lambda _: None, runtime_at_start='stopped') + self.assertEqual(code, 0) + cleanup.assert_called_once() + + def test_runtime_existing_before_helpers_is_left_running(self): + with mock.patch.object(supervisor, 'cleanup') as cleanup: + supervisor.run_supervised( + ['true'], self.prefix, None, proc_root=self.proc, poll=0, + sleep=lambda _: None, out=io.StringIO(), runtime_at_start='running') + cleanup.assert_not_called() + class ExitStatusTests(unittest.TestCase): def setUp(self): diff --git a/tools/crash-report.py b/tools/crash-report.py index 7d1565d..6dac376 100644 --- a/tools/crash-report.py +++ b/tools/crash-report.py @@ -44,12 +44,10 @@ PROCESS_NAMES = ( 'LeagueClient', 'League of Legends', 'CrBrowserMain', 'CrGpuMain', 'CrRendererMain', 'CrUtilityMain', 'wine', 'gamescope', 'Xwayland', ) +# Credential names may be qualified, e.g. --riotclient-auth-token. SENSITIVE = re.compile( - r'(?i)^(?:--?)?(?:pass(?:word|wd)?|token|secret|api[-_]?key|auth(?:orization)?|' - r'credential|cookie|session|bearer)$') -SENSITIVE_ASSIGNMENT = re.compile( - r'(?i)^((?:--?)?(?:pass(?:word|wd)?|token|secret|api[-_]?key|auth(?:orization)?|' - r'credential|cookie|session)[^=]*)=(.*)$') + r'(?i)(?:^|[-_])(?:pass(?:word|wd)?|token|secret|api[-_]?key|' + r'auth(?:orization)?|credential|cookie|session|bearer)') URL_USERINFO = re.compile(r'(://[^/:@\s]+:)[^/@\s]+@') @@ -72,14 +70,14 @@ def mask_argv(arguments): masked.append('') hide_next = False continue - if SENSITIVE.match(argument): + name, separator, _value = argument.partition('=') + if SENSITIVE.search(name): + if separator: + masked.append(name + '=') + continue masked.append(argument) hide_next = True continue - assignment = SENSITIVE_ASSIGNMENT.match(argument) - if assignment: - masked.append(assignment.group(1) + '=') - continue masked.append(URL_USERINFO.sub(r'\1@', argument)) return ' '.join(masked) diff --git a/tools/riot-launcher-offline-check.py b/tools/riot-launcher-offline-check.py index 0702183..7f45f19 100644 --- a/tools/riot-launcher-offline-check.py +++ b/tools/riot-launcher-offline-check.py @@ -28,7 +28,13 @@ record = {{"command": os.path.basename(sys.argv[0]), "args": sys.argv[1:]}} # existing assertions still inspect the guest launch that follows. if record["command"] == "python3" and sys.argv[1:] and os.path.basename(sys.argv[1]) == "session-supervisor.py": remainder = sys.argv[2:] + if "--probe-runtime" in remainder: + with open({str(log)!r}, "a") as output: + output.write(json.dumps({{"command": "runtime-probe"}}) + "\\n") + print("stopped") + sys.exit(0) if "--" in remainder: + assert remainder[remainder.index("--runtime-at-start") + 1] == "stopped" command = remainder[remainder.index("--") + 1:] if command and command[0] == "python3": os.execv(sys.executable, [sys.executable] + command[1:]) @@ -95,6 +101,11 @@ with open({str(log)!r}, "a") as output: for fd in leaked: os.close(fd) records = [json.loads(line) for line in log.read_text().splitlines()] + probe = next(i for i, record in enumerate(records) if record["command"] == "runtime-probe") + roots = next(i for i, record in enumerate(records) + if record["command"] == "python3" and + Path(record["args"][0]).name == "ensure-darling-root-certificates.py") + assert probe < roots, "runtime ownership was captured after certificate startup" maintenance = [r for r in records if r["command"] == "python3"] assert [Path(r["args"][0]).name for r in maintenance] == [ "runtime_paths.py", "ensure-darling-runtime.py", "ensure-darling-service-accounts.py", "ensure-league-runtime-layout.py", @@ -103,6 +114,8 @@ with open({str(log)!r}, "a") as output: launches = [r for r in records if r["command"] == "darling"] assert len(launches) == 1, "unexpected launch count" for record in records: + if record["command"] == "runtime-probe": + continue assert record["hud"] == {"MANGOHUD": None, "DISABLE_MANGOHUD": "1"}, "MangoHud survived before a runtime helper" launch = launches[0] assert launch["fds"] == [0, 1, 2], f"inherited descriptors reached the guest: {launch['fds']}" diff --git a/tools/run-riot-client.sh b/tools/run-riot-client.sh index ede4d81..c942db6 100755 --- a/tools/run-riot-client.sh +++ b/tools/run-riot-client.sh @@ -33,6 +33,8 @@ fi # The bundled downloader rejects IPv6 CONNECT_TO addresses. Only Riot CDN # AAAA lookups are suppressed; set this to 0 to retain the upstream resolver. export RCLOL_CDN_IPV4_ONLY="${RCLOL_CDN_IPV4_ONLY:-1}" +# Capture ownership before the certificate helper can start this session's runtime. +runtime_at_start=$(python3 "$runner_dir/session-supervisor.py" --prefix "$prefix_dir" --probe-runtime) python3 "$runner_dir/ensure-darling-runtime.py" python3 "$runner_dir/ensure-darling-service-accounts.py" python3 "$runner_dir/ensure-league-runtime-layout.py" @@ -178,7 +180,7 @@ fi # RCLOL_STOP_AFTER_CLIENT_SECONDS=N to stop once the client window has been # gone for N seconds with no game running. exec python3 "$runner_dir/session-supervisor.py" --prefix "$prefix" \ - --darling "${darling_bin:-darling}" -- \ + --darling "${darling_bin:-darling}" --runtime-at-start "$runtime_at_start" -- \ python3 "$runner_dir/exec-guest.py" env -u ELECTRON_RUN_AS_NODE -u DYLD_INSERT_LIBRARIES EGL_PLATFORM=x11 \ MAC_CHROMIUM_TMPDIR="${MAC_CHROMIUM_TMPDIR:-/private/tmp}" \ LANG="${RCLOL_GUEST_LOCALE:-en_US.UTF-8}" LC_ALL="${RCLOL_GUEST_LOCALE:-en_US.UTF-8}" \ diff --git a/tools/session-supervisor.py b/tools/session-supervisor.py index aeb3f64..54ca6aa 100755 --- a/tools/session-supervisor.py +++ b/tools/session-supervisor.py @@ -191,18 +191,59 @@ def wrapper(proc, name): return None +def process_identity(proc): + """Identity captured before shutdown, not just a reusable PID.""" + info = process_info(proc) + if info is None: + return None + try: + return info[2], os.readlink(proc / 'ns/pid'), proc.stat().st_uid + except OSError: + return None + + +def signal_process(pid, proc, identity, signum, kill=os.kill): + """Pin the process before the final identity check to close the PID reuse race.""" + if identity is None or process_identity(proc) != identity: + return + if kill is not os.kill: + # Injected signal transport for offline process fixtures. + kill(pid, signum) + return + try: + descriptor = os.pidfd_open(pid) + except (AttributeError, OSError): + # Never fall back to signalling an unpinned PID. + return + try: + if process_identity(proc) == identity: + signal.pidfd_send_signal(descriptor, signum) + except (AttributeError, OSError): + pass + finally: + os.close(descriptor) + + def stop_processes(members, kill=os.kill, sleep=time.sleep, proc_root=Path('/proc'), out=sys.stdout, term_grace=TERM_GRACE, kill_grace=KILL_GRACE, - clock=time.monotonic): + clock=time.monotonic, identities=None): """SIGTERM then SIGKILL a set of processes; return what is still present.""" + if identities is None: + identities = {pid: process_identity(proc) for pid, proc in members} + + def present(pid, proc): + return (identities.get(pid) is not None + and process_identity(proc) == identities[pid]) + def alive(pid, proc): info = process_info(proc) - return info is not None and info[0] != 'Z' + return present(pid, proc) and info is not None and info[0] != 'Z' + processes = dict(members) remaining = [(pid, proc) for pid, proc in members if alive(pid, proc)] for pid in deepest_first(remaining, proc_root): try: - kill(pid, signal.SIGTERM) + signal_process(pid, processes[pid], identities[pid], signal.SIGTERM, kill) except OSError: pass deadline = clock() + term_grace @@ -214,18 +255,18 @@ def stop_processes(members, kill=os.kill, sleep=time.sleep, proc_root=Path('/pro remaining = [(pid, proc) for pid, proc in remaining if alive(pid, proc)] for pid in deepest_first(remaining, proc_root): try: - kill(pid, signal.SIGKILL) + signal_process(pid, processes[pid], identities[pid], signal.SIGKILL, kill) except OSError: pass deadline = clock() + kill_grace while remaining and clock() < deadline: sleep(POLL_SECONDS) remaining = [(pid, proc) for pid, proc in remaining - if process_info(proc) is not None] + if present(pid, proc)] # Re-read once even when a grace period is zero, so the caller never gets a # stale "still running" answer for a process that SIGKILL already removed. return [(pid, proc) for pid, proc in remaining - if process_info(proc) is not None] + if present(pid, proc)] def cleanup(prefix, darling, namespace, proc_root=Path('/proc'), run=subprocess.run, @@ -236,6 +277,7 @@ def cleanup(prefix, darling, namespace, proc_root=Path('/proc'), run=subprocess. namespace = prefix_namespace(prefix, proc_root) own_namespace = own_namespace or os.readlink('/proc/self/ns/pid') members = namespace_members(namespace, proc_root, own_namespace) + identities = {pid: process_identity(proc) for pid, proc in members} if members and darling: try: run([darling, 'shutdown'], stdout=subprocess.DEVNULL, @@ -246,16 +288,18 @@ def cleanup(prefix, darling, namespace, proc_root=Path('/proc'), run=subprocess. while members and clock() < deadline: sleep(POLL_SECONDS) members = [(pid, proc) for pid, proc in members - if process_info(proc) is not None] + if identities.get(pid) is not None + and process_identity(proc) == identities[pid]] if not members: members = prefix_only(prefix, proc_root) + identities = {pid: process_identity(proc) for pid, proc in members} if not members: log('runtime and guest processes are gone', out) return [] log('stopping %d leftover guest process(es)' % len(members), out) remaining = stop_processes(members, kill=kill, sleep=sleep, proc_root=proc_root, out=out, term_grace=term_grace, - kill_grace=kill_grace, clock=clock) + kill_grace=kill_grace, clock=clock, identities=identities) if remaining: details = [] for pid, proc in remaining: @@ -347,7 +391,7 @@ def idle_decision(seen_client, idle_since, now, windows, game, seconds): def run_supervised(command, prefix, darling, stop_after=0, clean=True, proc_root=Path('/proc'), run=subprocess.run, kill=os.kill, sleep=time.sleep, out=sys.stdout, poll=POLL_SECONDS, - own_namespace=None): + own_namespace=None, runtime_at_start=None): """Run command, forward termination signals, then clean the session. Cleanup only runs when this launcher started the runtime. A second launch @@ -356,7 +400,8 @@ def run_supervised(command, prefix, darling, stop_after=0, clean=True, """ if not clean: os.execvp(command[0], command) - owns_runtime = not runtime_alive(prefix, proc_root) + owns_runtime = (runtime_at_start == 'stopped' if runtime_at_start is not None + else not runtime_alive(prefix, proc_root)) namespace = None child = subprocess.Popen(command, start_new_session=True) stop = [] @@ -435,6 +480,10 @@ def env_seconds(name, out=sys.stderr): def main(argv=None): parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) parser.add_argument('--prefix', type=Path, required=True) + parser.add_argument('--probe-runtime', action='store_true', + help='print runtime state before launcher startup helpers') + parser.add_argument('--runtime-at-start', choices=('running', 'stopped'), + help='runtime state captured before launcher startup helpers') parser.add_argument('--darling', help='loader used for a graceful shutdown') parser.add_argument('--stop-after', type=int, default=env_seconds('RCLOL_STOP_AFTER_CLIENT_SECONDS'), @@ -443,13 +492,16 @@ def main(argv=None): help='leave the runtime alone after the child exits') parser.add_argument('command', nargs=argparse.REMAINDER) args = parser.parse_args(argv) + prefix = args.prefix.expanduser().resolve() + if args.probe_runtime: + print('running' if runtime_alive(prefix) else 'stopped') + return 0 command = args.command[1:] if args.command[:1] == ['--'] else args.command if not command: parser.error('a command is required after --') clean = not args.no_clean and os.environ.get('RCLOL_CLEAN_ON_EXIT', '1') != '0' - prefix = args.prefix.expanduser().resolve() return run_supervised(command, prefix, args.darling, stop_after=max(0, args.stop_after), - clean=clean) + clean=clean, runtime_at_start=args.runtime_at_start) if __name__ == '__main__':