From 2103a3c7f08934c7b8fc109fcb376fcb75ffb767 Mon Sep 17 00:00:00 2001 From: Stuart Clark Date: Mon, 7 Sep 2026 00:00:01 +0000 Subject: [PATCH] fix(devtools): refuse a webserver port the dev server does not own --- .cspell-project-words.txt | 9 +-- .github/workflows/ci.yml | 83 +++++++++++++++++++++++++++ drupal/.devtools/helpers.php | 105 ++++++++++++++++++++++++++++++++++- drupal/.devtools/start | 51 +++++++++++++++-- 4 files changed, 239 insertions(+), 9 deletions(-) diff --git a/.cspell-project-words.txt b/.cspell-project-words.txt index c1a11134..5e14e3c4 100644 --- a/.cspell-project-words.txt +++ b/.cspell-project-words.txt @@ -1,3 +1,7 @@ +Landofile +Olivero +PKCE +Stanislav appserver behaviour bmewburn @@ -25,7 +29,6 @@ intelephense interruptible knip lando -Landofile libasound libfreetype libgbm @@ -46,21 +49,19 @@ nohup nuxt nuxtjs nvmrc -Olivero opcache openssl openvscode phpsab phpstan phpunit -PKCE +procps recognise recognised recognises repository setuptools shivammathur -Stanislav starterkit startswith tokenless diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5d710254..5c3d32e6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -334,6 +334,89 @@ jobs: working-directory: /tmp/site/nuxt run: npm run build + # `.devtools/start` must never report a server it did not start. Needs + # PHP and the scripts, nothing else: both cases fail before Drupal is + # touched, so this job skips assemble and provision entirely. + test_start_guardrails: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + with: + persist-credentials: false + + - name: Setup PHP + uses: shivammathur/setup-php@v2 + with: + php-version: '8.4' + + # The readiness probe used to connect to the port and accept any + # answer, so a stranger holding it read as a healthy start: BASE_URL + # was written and a login link printed for a site never started. + - name: A port held by another process is refused + working-directory: drupal + run: | + python3 -m http.server 8899 --bind 127.0.0.1 >/dev/null 2>&1 & + squatter=$! + trap 'kill "$squatter" 2>/dev/null || true' EXIT + until curl -sf -o /dev/null http://127.0.0.1:8899/; do sleep 1; done + if WEBSERVER_PORT=8899 .devtools/start > /tmp/squatted.log 2>&1; then + echo 'start reported success against a port it never bound' >&2 + cat /tmp/squatted.log >&2 + exit 1 + fi + grep -q 'already in use by another process' /tmp/squatted.log + test ! -f ../.env + # A guard that clears the port by killing whatever holds it is + # not a guard. stop_webserver only targets `php -S`, and this + # is what pins that. + kill -0 "$squatter" 2>/dev/null || { + echo 'start killed a process it did not start' >&2 + exit 1 + } + + # The gitlab CI image has no procps, and the first version of this + # guard shelled out to `ps` for both liveness and identity: it reported + # a healthy server as exited and could not recognise its own process to + # stop it. This runner does have `ps`, so the guard passed here while + # failing there. Hiding `ps` is what makes that case reachable. + - name: The guard works on an image without procps + working-directory: drupal + run: | + shim=$(mktemp -d) + printf '#!/bin/sh\nexit 127\n' > "$shim/ps" + chmod +x "$shim/ps" + python3 -m http.server 8908 --bind 127.0.0.1 >/dev/null 2>&1 & + squatter=$! + trap 'kill "$squatter" 2>/dev/null || true' EXIT + until curl -sf -o /dev/null http://127.0.0.1:8908/; do sleep 1; done + if PATH="$shim:$PATH" WEBSERVER_PORT=8908 .devtools/start > /tmp/no-procps.log 2>&1; then + echo 'start reported success against a squatted port with no ps' >&2 + cat /tmp/no-procps.log >&2 + exit 1 + fi + grep -q 'already in use by another process' /tmp/no-procps.log + # /proc gives the command line, so the holder is still named. + grep -q 'http.server 8908' /tmp/no-procps.log + kill -0 "$squatter" 2>/dev/null || { + echo 'start killed a process it did not start' >&2 + exit 1 + } + + # Second net, for a bind that fails with the port free: the wait + # watches the process it launched, not the port, so it reports the + # reason instead of timing out on a server that is already gone. + - name: A failed bind is reported, not waited out + working-directory: drupal + run: | + if WEBSERVER_HOST=203.0.113.1 WEBSERVER_PORT=8901 WEBSERVER_WAIT_TIMEOUT=20 \ + .devtools/start > /tmp/bind-failure.log 2>&1; then + echo 'start reported success for a server that never bound' >&2 + cat /tmp/bind-failure.log >&2 + exit 1 + fi + grep -q 'exited before it accepted a connection' /tmp/bind-failure.log + grep -q 'Cannot assign requested address' /tmp/bind-failure.log + # What the root install promises on machines without a working PHP: a # consumer's `npm install` must never fail, `npm run setup` must # fail loudly, and a too-old PHP must be rejected by the preflight. diff --git a/drupal/.devtools/helpers.php b/drupal/.devtools/helpers.php index 992ceda6..cdb81901 100644 --- a/drupal/.devtools/helpers.php +++ b/drupal/.devtools/helpers.php @@ -244,6 +244,107 @@ function find_free_port(int $start = 8888, int $max_attempts = 100): int { // @codeCoverageIgnoreEnd } +/** + * Whether something is already accepting connections on a host and port. + * + * Used as a pre-flight guard, not a readiness probe: a readiness probe + * that only asks "is the port answering" cannot tell our own server from + * a stranger's, which is how a failed bind used to read as a good start. + */ +function port_is_open(string $host, string $port, float $timeout = 0.5): bool { + $conn = @stream_socket_client(sprintf('tcp://%s:%s', $host, $port), $errno, $errstr, $timeout); + if ($conn === FALSE) { + return FALSE; + } + fclose($conn); + + return TRUE; +} + +/** + * Describe whatever holds a port, for an error message. '' when unknown. + */ +function port_holder(string $port): string { + $pids = []; + @exec(sprintf('lsof -ti:%s 2>/dev/null', escapeshellarg($port)), $pids); + foreach ($pids as $pid) { + $pid = trim((string) $pid); + if ($pid === '' || !ctype_digit($pid)) { + continue; + } + $command = process_command((int) $pid); + if ($command !== '') { + return sprintf('pid %d: %s', (int) $pid, $command); + } + } + + return ''; +} + +/** + * The command line of a process, or '' when it cannot be read. + * + * `/proc` first so this works on images without procps. Note the asymmetry + * with pid_is_running(): an unknown liveness must not be read as "dead", but + * an unknown command line must not be read as "ours", because the caller uses + * it to decide what to kill. Uncertainty is safe in opposite directions. + */ +function process_command(int $pid): string { + if ($pid < 1) { + return ''; + } + + $cmdline = @file_get_contents(sprintf('/proc/%d/cmdline', $pid)); + if ($cmdline !== FALSE && $cmdline !== '') { + return trim(str_replace("\0", ' ', $cmdline)); + } + + return trim((string) @shell_exec(sprintf('ps -p %d -o command= 2>/dev/null', $pid))); +} + +/** + * Whether a process id is still running. + * + * Returns NULL when it cannot be determined, which callers must not read as + * "dead". The first version shelled out to `ps` and treated its absence as a + * dead process, so on an image without procps (the `php:8.3` CI image, for + * one) a perfectly healthy server was reported as having exited. A liveness + * check that cannot answer has to say so rather than guess the alarming + * answer. + * + * @return bool|null + * TRUE if running, FALSE if definitely not, NULL if undeterminable. + */ +function pid_is_running(int $pid): ?bool { + if ($pid < 1) { + return FALSE; + } + + // Signal 0 performs the permission and existence checks without sending + // anything. Native, no subprocess, and the usual answer for this. + if (function_exists('posix_kill')) { + if (posix_kill($pid, 0)) { + return TRUE; + } + // ESRCH means no such process; EPERM means it exists but is not ours. + return function_exists('posix_get_last_error') + && posix_get_last_error() === (defined('PCNTL_ESRCH') ? PCNTL_ESRCH : 3) + ? FALSE + : NULL; + } + + // Linux without the posix extension. + if (is_dir('/proc')) { + return is_dir('/proc/' . $pid); + } + + // Last resort. An empty result here is genuinely ambiguous, because it is + // also what a missing `ps` produces, so it reports unknown rather than dead. + $out = trim((string) @shell_exec(sprintf('ps -p %d -o pid= 2>/dev/null', $pid))); + + return $out !== '' ? TRUE : NULL; +} + /** * Stop the dev webserver started by .devtools/start. * @@ -277,7 +378,9 @@ function stop_webserver(string $port): void { } $targets = array_filter(array_unique($candidates), function (int $pid): bool { - $command = trim((string) @shell_exec(sprintf('ps -p %d -o command= 2>/dev/null', $pid))); + $command = process_command($pid); + // An unreadable command line means "do not touch": better to leave our + // own server running than to signal a process we could not identify. return $command !== '' && str_contains($command, 'php') && str_contains($command, '-S'); }); if ($targets === []) { diff --git a/drupal/.devtools/start b/drupal/.devtools/start index c151b052..611d2fa1 100755 --- a/drupal/.devtools/start +++ b/drupal/.devtools/start @@ -55,6 +55,32 @@ if ($xdebug_enabled) { TASK('Stopping previously started services, if any.'); stop_webserver($webserver_port); +// stop_webserver only kills `php -S` processes, which is right: it must +// not shoot at whatever else happens to hold the port. So check here, +// after it has cleared our own server away, and refuse rather than start +// a server that cannot bind. A killed server can take a moment to release +// the port, so the port gets a short grace period before it counts as +// somebody else's. +TASK('Checking that the webserver port is free.'); +$free = FALSE; +for ($i = 0; $i < 6; $i++) { + if (!port_is_open($webserver_host, $webserver_port)) { + $free = TRUE; + break; + } + usleep(500000); +} +if (!$free) { + $holder = port_holder($webserver_port); + FAIL( + 'Port %s is already in use by another process%s.' . "\n" + . 'Stop it, or set WEBSERVER_PORT to a free port.', + $webserver_port, + $holder !== '' ? ' (' . $holder . ')' : '' + ); +} +PASS('Port %s is free.', $webserver_port); + TASK('Starting the PHP webserver (docroot: web/).'); NOTE('Waiting up to %s seconds for the server to be ready.', (string) $webserver_wait_timeout); $cwd = (string) getcwd(); @@ -75,14 +101,25 @@ passthru(sprintf( )); $ready = FALSE; +$died = FALSE; for ($i = 0; $i < $webserver_wait_timeout; $i++) { sleep(1); // Liveness: a cold first start (fresh site, no opcache) can take a // while - dots show the script is waiting, not hung. echo '.'; - $fp = @fsockopen($webserver_host, (int) $webserver_port, $errno, $errstr, 1); - if ($fp !== FALSE) { - fclose($fp); + + // The process we launched, not the port, is the thing being waited on. + // A dead pid means the bind failed (or PHP exited), and there is nothing + // left to wait for: stop now and print why. Only an explicit FALSE counts; + // NULL means the check could not tell, and an undeterminable liveness must + // fall through to the port probe rather than abort a healthy start. + $pid = (int) trim((string) @file_get_contents(server_pid_file())); + if ($pid > 0 && pid_is_running($pid) === FALSE) { + $died = TRUE; + break; + } + + if (port_is_open($webserver_host, $webserver_port, 1)) { $ready = TRUE; break; } @@ -90,7 +127,13 @@ for ($i = 0; $i < $webserver_wait_timeout; $i++) { echo PHP_EOL; if (!$ready) { $log = @file_get_contents('/tmp/quickstart-drupal-php-server.log') ?: ''; - FAIL('Unable to start inbuilt PHP server' . ($log !== '' ? "\n" . $log : '')); + // '%s' as the format, not the message: a server log can contain a + // literal % and sprintf would choke on it. + FAIL( + '%s', + ($died ? 'The PHP server exited before it accepted a connection.' : 'Unable to start inbuilt PHP server') + . ($log !== '' ? "\n" . $log : '') + ); } PASS('Server started successfully.');