diff --git a/CLAUDE.md b/CLAUDE.md index da8956f1d..e4c9e7890 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -562,6 +562,21 @@ named `deleteMultiplePartialMatchIsNotDetected`, pinning the gap as known rather **A test that records a weaker behaviour with a name saying so is a defect somebody chose to describe** — read it as a lead, not as a decision, and check what its siblings do. +**A file rewritten in place, with nothing to fall back on.** `FileHandler::save()` did +`ftruncate(0)` and then `fwrite()`, so the file is empty on disk between the two. `config.xml` +goes through it, and it holds the database credentials, the password salt and the master-password +hash — a process killed in that window leaves an installation that cannot boot *and cannot be +repaired through the UI*, because the container is built before `Init` runs and the install route +refuses once `` is set. There is no backup to fall back on either: +`ConfigBackupService::backup()` exists and is called from nowhere in `src/`. + +It is now a sibling temp file renamed into place, which is atomic within a filesystem. The lock was +never what protected readers and could not have been — `XmlFileStorage::load()` hands the *path* to +`DOMDocument` and `readToString()` reads by path, so neither ever took it. **Ask what a half-written +file would cost before asking who holds the lock**; and note the two things a rename has to carry +over by hand: the target's permissions, and a stat cache that would otherwise still describe the +replaced file. + **A static factory a subclass cannot use.** `SPException` offers `error()`, `info()`, `critical()`, `warning()` and `from()`, each doing `new static($message, …)`. Four subclasses fix their own message and take `int $type` first instead — `AccountPermissionException`, `UnauthorizedActionException`, diff --git a/src/Infrastructure/File/FileHandler.php b/src/Infrastructure/File/FileHandler.php index b0278f88c..dca1de0d0 100644 --- a/src/Infrastructure/File/FileHandler.php +++ b/src/Infrastructure/File/FileHandler.php @@ -44,11 +44,24 @@ final class FileHandler extends SplFileObject implements FileHandlerInterface { public const CHUNK_FACTOR = 3; + /** + * @inheritDoc + */ + /** + * The mode is kept because `save()` no longer writes through this handle — it replaces the + * file by renaming a sibling over it, which the directory's permissions allow whatever this + * handle was opened for. Refusing a read-only handle has to be explicit now that the stream + * will not refuse it for us. + */ + private readonly string $mode; + /** * @inheritDoc */ public function __construct(private readonly string $file, string $mode = 'r') { + $this->mode = $mode; + parent::__construct($this->file, $mode); } @@ -134,19 +147,88 @@ public function read(): iterable */ public function save(string $data): FileHandlerInterface { + // `r` without `+` is the only read-only family; w, a, x and c all open for writing. + if (str_starts_with($this->mode, 'r') && !str_contains($this->mode, '+')) { + throw FileException::error(sprintf(__('Unable to read/write the file (%s)'), $this->file)); + } + $this->lock(); - $this->rewind(); - $this->ftruncate(0); + try { + $this->replaceWith($data); + } finally { + $this->unlock(); + } - if ($this->fwrite($data) === false) { + return $this; + } + + /** + * Put `$data` in place of this file's contents, without the file ever being partly written. + * + * This used to be `ftruncate(0)` followed by `fwrite()`, which has a window in which the file + * is empty on disk. `config.xml` goes through here, and it holds the database credentials, the + * password salt and the master-password hash — so a process killed in that window (an OOM, a + * container stopped mid-save, the host losing power) left an installation that cannot boot and + * cannot be recovered through the UI, because the container is built before `Init` runs and the + * install route refuses once `` was set. Nothing takes a backup first: + * `ConfigBackupService::backup()` exists and is called from nowhere. + * + * A sibling temp file renamed into place cannot show a half-written state to anybody, because + * `rename()` within a filesystem is atomic. That is what readers need, and the lock was never + * going to give it to them: `XmlFileStorage::load()` hands the *path* to `DOMDocument`, and + * `readToString()` reads by path too, so neither has ever taken this handle's lock. The lock + * stays because it still orders two writers that hold the same open file, and because losing + * one of two concurrent saves is a different and much smaller problem than losing the file. + * + * The temp file is created private and given the target's own permissions before the rename, + * so the mode does not change and the contents are never briefly readable at the umask. + * + * @throws FileException + */ + private function replaceWith(string $data): void + { + // Per-process, so two of them cannot write into each other's temp file. + $temp = sprintf('%s.%d.tmp', $this->file, getmypid()); + + $mode = file_exists($this->file) + ? (fileperms($this->file) & 0777) + : (0666 & ~umask()); + + $handle = @fopen($temp, 'wb'); + + if ($handle === false) { throw FileException::error(sprintf(__('Unable to read/write the file (%s)'), $this->file)); } - $this->fflush(); - $this->unlock(); + try { + @chmod($temp, 0600); - return $this; + if (fwrite($handle, $data) === false) { + throw FileException::error(sprintf(__('Unable to read/write the file (%s)'), $this->file)); + } + + fflush($handle); + } finally { + fclose($handle); + } + + try { + @chmod($temp, $mode); + + if (!@rename($temp, $this->file)) { + throw FileException::error(sprintf(__('Unable to read/write the file (%s)'), $this->file)); + } + } catch (FileException $e) { + @unlink($temp); + + throw $e; + } + + // This handle still refers to the file that was just replaced. Nothing reads through it — + // every read in this class goes by path — but clearing the stat cache keeps getFileSize() + // and getFileTime() answering about what is now on disk. + clearstatcache(true, $this->file); } /** diff --git a/tests/Unit/Infrastructure/File/FileHandlerTest.php b/tests/Unit/Infrastructure/File/FileHandlerTest.php index 0e9bc7aa5..a39351572 100644 --- a/tests/Unit/Infrastructure/File/FileHandlerTest.php +++ b/tests/Unit/Infrastructure/File/FileHandlerTest.php @@ -90,6 +90,124 @@ public function testSavePersistsToDisk(): void self::assertSame('saved content', file_get_contents($this->file)); } + /** + * The file is replaced, not truncated and rewritten. + * + * `config.xml` goes through `save()`, and it holds the database credentials, the password salt + * and the master-password hash. `ftruncate(0)` followed by `fwrite()` has a window in which + * that file is empty on disk, and a process killed in it — an OOM, a container stopped + * mid-save, the host losing power — leaves an installation that cannot boot and cannot be + * recovered through the UI. Nothing takes a backup first. + * + * The inode is the observable part of the fix: a rename gives the path a different file, where + * a truncate keeps the same one. It is also exactly the property that makes the replacement + * atomic for a reader, which is what this is for. + * + * @throws FileException + */ + public function testSaveReplacesTheFileRatherThanTruncatingIt(): void + { + file_put_contents($this->file, 'the old contents'); + $before = fileinode($this->file); + + (new FileHandler($this->file, 'c+'))->save('the new contents'); + + clearstatcache(true, $this->file); + + self::assertSame('the new contents', file_get_contents($this->file)); + self::assertNotSame($before, fileinode($this->file), 'the file must be replaced, not truncated'); + } + + /** + * A save that cannot be completed leaves the file exactly as it was. + * + * This is the failure the change is about, and the only way to reach it deterministically here + * is to make the temporary file impossible to create — a directory in its place does that for + * root as well, which is what the suite runs as. Before the change the write went straight into + * the file, so the same conditions replaced its contents instead of preserving them. + * + * @throws FileException + */ + public function testASaveThatCannotBeCompletedLeavesTheFileAsItWas(): void + { + file_put_contents($this->file, 'the old contents'); + + // Where save() wants to put its temporary file. + mkdir(sprintf('%s.%d.tmp', $this->file, getmypid())); + + $handler = new FileHandler($this->file, 'c+'); + + try { + @$handler->save('the new contents'); + self::fail('a save that cannot write its temporary file has to throw'); + } catch (FileException) { + // asserted below: what matters is what is left on disk + } finally { + @rmdir(sprintf('%s.%d.tmp', $this->file, getmypid())); + } + + clearstatcache(true, $this->file); + + self::assertSame('the old contents', file_get_contents($this->file)); + } + + /** + * Replacing the file keeps the permissions it had, rather than giving it the umask's. + * + * `config/config.xml` is 0644 with its *directory* held at 0750, and that arrangement is + * deliberate — a save must not quietly change either half of it. + * + * @throws FileException + */ + public function testSaveKeepsThePermissionsOfTheFileItReplaces(): void + { + file_put_contents($this->file, 'contents'); + chmod($this->file, 0640); + + (new FileHandler($this->file, 'c+'))->save('new contents'); + + clearstatcache(true, $this->file); + + self::assertSame(0640, fileperms($this->file) & 0777); + } + + /** + * And it leaves nothing beside the file it wrote. + * + * @throws FileException + */ + public function testSaveLeavesNoTemporaryFileBehind(): void + { + (new FileHandler($this->file, 'c+'))->save('contents'); + + self::assertSame([$this->file], glob($this->dir . DIRECTORY_SEPARATOR . '*')); + } + + /** + * After a save, this handle still describes what is on disk. + * + * It refers to the file that was just replaced, so this is the thing a rename could plausibly + * have broken: `ConfigFile::isExpired()` compares the config cache against + * `$this->fileStorage->getFileTime()`, and a stale mtime there would leave the cache looking + * current after every save. `SplFileInfo` stats the pathname rather than the open descriptor, + * and `save()` clears the stat cache for it, so both answers follow the rename. + * + * @throws FileException + */ + public function testTheHandleStillDescribesTheFileAfterASave(): void + { + file_put_contents($this->file, 'old'); + touch($this->file, time() - 60); + + $handler = new FileHandler($this->file, 'c+'); + $before = $handler->getFileTime(); + + $handler->save('new and longer contents'); + + self::assertGreaterThan($before, $handler->getFileTime(), 'the mtime must follow the rename'); + self::assertSame(strlen('new and longer contents'), $handler->getFileSize()); + } + /** * @throws FileException */