fix: redact Riot tokens and make session cleanup preserve process identity

Signed-off-by: lucannez64 <[email protected]>

.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'

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

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')))

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):

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('<redacted>')
             hide_next = False
             continue
-        if SENSITIVE.match(argument):
+        name, separator, _value = argument.partition('=')
+        if SENSITIVE.search(name):
+            if separator:
+                masked.append(name + '=<redacted>')
+                continue
             masked.append(argument)
             hide_next = True
             continue
-        assignment = SENSITIVE_ASSIGNMENT.match(argument)
-        if assignment:
-            masked.append(assignment.group(1) + '=<redacted>')
-            continue
         masked.append(URL_USERINFO.sub(r'\1<redacted>@', argument))
     return ' '.join(masked)
 

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']}"

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}" \

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__':