From b5aa20e3d2f8ff83d1cc12ec2a152bd87d38385f Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 3 Sep 2026 01:19:13 +0200 Subject: [PATCH] fix: replace config.xml atomically instead of truncating it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FileHandler::save() did ftruncate(0) and then fwrite(), so the file is empty on disk between the two. config.xml goes through this method — database credentials, password salt, master-password hash — and a process killed in that window leaves an installation that cannot boot and cannot be repaired through the UI, because the DI container is built before Init runs and the install route refuses once is set. There is nothing to fall back on either: ConfigBackupService::backup() exists and is called from nowhere in src/. That is a separate finding, not fixed here. save() now writes a sibling temp file and renames it over the target. rename() within a filesystem is atomic, so a reader sees either the whole old file or the whole new one, and a process that dies at any point leaves the original untouched. The lock stays, but it 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. Two things the rename carries over by hand — the target's permissions, and a cleared stat cache, which matters because ConfigFile::isExpired() compares the config cache against getFileTime(). One deliberate behaviour change: replacing a file needs write permission on its directory, not just on the file. ConfigUtil already holds config/ at 0750 owned by the web user. A read-only handle is still refused, now by checking the mode it was opened with rather than by letting fwrite() fail on the stream. --- CLAUDE.md | 15 +++ src/Infrastructure/File/FileHandler.php | 94 +++++++++++++- .../Infrastructure/File/FileHandlerTest.php | 118 ++++++++++++++++++ 3 files changed, 221 insertions(+), 6 deletions(-) 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 */