diff --git a/CHANGELOG.md b/CHANGELOG.md index de0b34c..5e97058 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -145,6 +145,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added `Installer::verifyChecksum()` public method - Added `tests/test_installer_checksum.phpt` — 5 test scenarios covering valid checksum, mismatch, empty file, binary content, and recomputed hash +- **SEC-009: Add advisory file locking (flock) to prevent TOCTOU race condition in concurrent Installer::install()** (#77) + - Acquires `LOCK_EX` on `lib/install.lock` before the `file_exists($libPath)` check (lock-check pattern) + - Double-check inside the lock: re-verifies the library isn't already installed by another concurrent process + - Lock file persists as a sentinel (never deleted) — ensures all concurrent processes share the same inode for `flock()` serialization + - Lock is released in `finally` block — crash-safe (kernel auto-releases `flock` on process termination) + - Added `tests/test_installer_flock.phpt` — 4 test scenarios covering basic flock, concurrent serialization, source code verification, and behavioral double-check test with `Installer::install()` + ### Changed - **SMELL-004: Triplicated Insert/Upsert/Update Code** (#85) diff --git a/src/Installer.php b/src/Installer.php index cd2ea1c..dd106a0 100644 --- a/src/Installer.php +++ b/src/Installer.php @@ -21,8 +21,14 @@ class Installer * SHA-256 checksum verification is performed before extraction to ensure integrity * of the downloaded archive (see verifyChecksum()). * + * Concurrent installations are serialized via advisory file locking (flock) to prevent + * TOCTOU race conditions between the file-exists check and the download+extract window. + * The lock is acquired first, then the existence of the library is re-checked inside the + * lock (lock-check pattern). The lock file persists as a sentinel to ensure all concurrent + * processes share the same inode for flock() serialization. + * * @param string|null $version Release version tag (e.g., "v0.4.10"). Auto-detected from composer if null. - * @throws RuntimeException On download failure, checksum mismatch, extraction failure, or missing lib in archive. + * @throws RuntimeException On download failure, checksum mismatch, extraction failure, lock failure, or missing lib in archive. */ public static function install(?string $version = null): void { @@ -46,38 +52,58 @@ public static function install(?string $version = null): void $libName = self::libName(); $libPath = $libDir . '/' . $libName; - if (file_exists($libPath)) { - echo "zvec FFI library already installed at {$libPath}\n"; - return; - } - echo "Downloading zvec FFI library {$version} for " . self::platformLabel() . "...\n"; - - $tmpDir = sys_get_temp_dir() . '/zvec_ffi_' . bin2hex(random_bytes(8)); - if (!mkdir($tmpDir, 0700)) { - throw new RuntimeException("Failed to create temporary directory"); + // Acquire exclusive lock to serialize concurrent installations (TOCTOU mitigation) + $lockFile = $libDir . '/install.lock'; + $lockFh = fopen($lockFile, 'w+'); + if (!$lockFh) { + throw new RuntimeException("Could not create lock file: {$lockFile}"); + } + if (!flock($lockFh, LOCK_EX)) { + fclose($lockFh); + throw new RuntimeException("Could not acquire installation lock"); } - $tmpFile = $tmpDir . '/download.tar.gz'; try { - self::download($url, $tmpFile); + // Double-check after acquiring lock — another process may have installed it + if (file_exists($libPath)) { + echo "zvec FFI library already installed at {$libPath}\n"; + return; + } - $expectedHash = self::getExpectedHash($version, $assetName); - self::verifyChecksum($tmpFile, $expectedHash); + echo "Downloading zvec FFI library {$version} for " . self::platformLabel() . "...\n"; - self::extract($tmpFile, $libDir); - } finally { - if (file_exists($tmpFile)) { - unlink($tmpFile); + $tmpDir = sys_get_temp_dir() . '/zvec_ffi_' . bin2hex(random_bytes(8)); + if (!mkdir($tmpDir, 0700)) { + throw new RuntimeException("Failed to create temporary directory"); } - exec("rm -rf " . escapeshellarg($tmpDir)); - } + $tmpFile = $tmpDir . '/download.tar.gz'; - if (!file_exists($libPath)) { - throw new RuntimeException("Download succeeded but {$libName} not found in archive."); - } + try { + self::download($url, $tmpFile); - echo "zvec FFI library installed at {$libPath}\n"; + $expectedHash = self::getExpectedHash($version, $assetName); + self::verifyChecksum($tmpFile, $expectedHash); + + self::extract($tmpFile, $libDir); + } finally { + if (file_exists($tmpFile)) { + unlink($tmpFile); + } + exec("rm -rf " . escapeshellarg($tmpDir)); + } + + if (!file_exists($libPath)) { + throw new RuntimeException("Download succeeded but {$libName} not found in archive."); + } + + echo "zvec FFI library installed at {$libPath}\n"; + } finally { + flock($lockFh, LOCK_UN); + fclose($lockFh); + // Lock file persists as a sentinel. Never delete — removing it would let + // a new process create a different inode and bypass flock() serialization. + } } public static function verifyChecksum(string $filePath, string $expectedHash): void diff --git a/tests/test_installer_flock.phpt b/tests/test_installer_flock.phpt new file mode 100644 index 0000000..0739b6c --- /dev/null +++ b/tests/test_installer_flock.phpt @@ -0,0 +1,175 @@ +--TEST-- +SEC-009: flock() prevents TOCTOU race condition in Installer::install() +--SKIPIF-- + +--FILE-- + 1) { + $overlapped = true; + } + $maxActive = max($maxActive, $active); + } elseif (str_starts_with($line, 'END')) { + $active--; + } + } + + echo "Max concurrent locks: $maxActive\n"; + echo "Overlapped: " . ($overlapped ? 'yes' : 'no') . "\n"; + + // --- Test 3: Verify flock() code exists in Installer.php --- + $source = file_get_contents(__DIR__ . '/../src/Installer.php'); + $hasFlock = str_contains($source, 'flock($lockFh, LOCK_EX)'); + $hasUnlock = str_contains($source, 'flock($lockFh, LOCK_UN)'); + $hasLockFile = str_contains($source, "install.lock"); + $hasDoubleCheck = substr_count($source, 'file_exists($libPath)') >= 2; + + echo "flock LOCK_EX in source: " . ($hasFlock ? 'yes' : 'no') . "\n"; + echo "flock LOCK_UN in source: " . ($hasUnlock ? 'yes' : 'no') . "\n"; + echo "install.lock in source: " . ($hasLockFile ? 'yes' : 'no') . "\n"; + echo "double-check pattern: " . ($hasDoubleCheck ? 'yes' : 'no') . "\n"; + + exec("rm -rf " . escapeshellarg($lockDir)); + + // --- Test 4: Behavioral — double-check pattern skips download when lib already exists --- + echo "---\n"; + $realLibPath = __DIR__ . '/../lib/' . (PHP_OS_FAMILY === 'Darwin' ? 'libzvec_ffi.dylib' : 'libzvec_ffi.so'); + $dummyCreated = false; + try { + // Create dummy library file so Installer::install() sees it as already installed + file_put_contents($realLibPath, 'dummy'); + $dummyCreated = true; + + // This should detect the file and return early (no download, no exception) + ob_start(); + Installer::install('v0.4.0'); + $output = ob_get_clean(); + + if (str_contains($output, 'already installed')) { + echo "Test 4: Double-check detected existing library\n"; + } else { + echo "FAIL: Expected 'already installed' message\n"; + exit(1); + } + + // Verify lock file persists as sentinel (never deleted — ensures same inode for flock) + $libDir = __DIR__ . '/../lib'; + $lockFilePath = $libDir . '/install.lock'; + if (file_exists($lockFilePath)) { + echo "Test 4: Lock file persists as sentinel\n"; + } else { + echo "FAIL: Lock file was deleted (sentinel should persist)\n"; + exit(1); + } + + echo "Test 4 PASS\n"; + } finally { + // Clean up dummy library + if ($dummyCreated && file_exists($realLibPath)) { + unlink($realLibPath); + } + // Clean up lock file left by test (Installer's own finally never deletes it) + $lockFilePath = __DIR__ . '/../lib/install.lock'; + if (file_exists($lockFilePath)) { + unlink($lockFilePath); + } + } + + echo "DONE\n"; +} finally { + exec("rm -rf " . escapeshellarg($path)); +} +?> +--EXPECT-- +Lock acquired: yes +Lock file exists: yes +Lock file after cleanup: no +Log entries: 6 +Max concurrent locks: 1 +Overlapped: no +flock LOCK_EX in source: yes +flock LOCK_UN in source: yes +install.lock in source: yes +double-check pattern: yes +--- +Test 4: Double-check detected existing library +Test 4: Lock file persists as sentinel +Test 4 PASS +DONE