fix: replace config.xml atomically instead of truncating it - #901
Merged
Conversation
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 <installed> 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FileHandler::save()didftruncate(0)and thenfwrite(). Between those two calls the file isempty on disk.
config.xmlgoes through this method. It holds the database credentials, the password salt and themaster-password hash, so a process killed in that window — an OOM kill, a container stopped
mid-save, the host losing power — leaves an installation that cannot boot. It also cannot be
repaired through the UI: the DI container is built before
Initruns, and the install route refusesonce
<installed>has been set. Recovery means editing files on the server.There is nothing to fall back on, either.
ConfigBackupService::backup()exists and is called fromnowhere in
src/— which is a second finding, and not this PR's to fix.The change
save()writes a sibling temp file and renames it over the target.rename()within a filesystemis 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 is worth being clear that it was never what protected readers and could not
have been:
XmlFileStorage::load()hands the path toDOMDocument, andreadToString()readsby path too, so neither has ever taken this handle's lock. What the lock still does is order two
writers holding the same open file — and losing one of two concurrent saves is a much smaller
problem than losing the file.
Two things a rename has to carry over by hand, and does:
rename.
config/config.xmlis 0644 with its directory held at 0750 and that arrangement isdeliberate, so a save must not change either half of it.
SplFileInfostats thepathname rather than the descriptor, so clearing the stat cache is enough — which matters because
ConfigFile::isExpired()compares the config cache againstgetFileTime(), and a stale mtimethere would leave the cache looking current after every save.
One behaviour change, deliberate
Replacing a file needs write permission on its directory, not just on the file. In this
application that is already the case —
ConfigUtilholdsconfig/at 0750 owned by the web user —but it is a real difference and worth stating.
A read-only handle is still refused. That used to fall out of
fwrite()failing on the stream;since the write no longer goes through the handle,
save()checks the mode it was opened with.Tests
Five, of which two fail against the old implementation:
getFileTime()/getFileSize()still describe what is on diskThe failure is forced deterministically by putting a directory where the temp file wants to go,
which blocks root as well — the suite runs as root, so permission-based denial would prove nothing.