Skip to content

config: stop trusting that config writes always succeed - #10646

Open
cclements wants to merge 1 commit into
opnsense:masterfrom
cclements:config-save-robustness
Open

config: stop trusting that config writes always succeed#10646
cclements wants to merge 1 commit into
opnsense:masterfrom
cclements:config-save-robustness

Conversation

@cclements

Copy link
Copy Markdown

While digging into how config.xml is written I noticed that Config::save() never
looks at what ftruncate(), fwrite() and fflush() return. The file has already
been truncated at that point, so if the write comes up short — disk full is the easy
way to hit this — we end up with a truncated config.xml and no error anywhere.

It gets worse from there. backup() then copies that damaged file into the backup
directory (the File::file_put_contents() result is ignored too, and it even
@touches the target first, so a completely failed write still leaves a zero-byte
"backup" behind), and cleanupBackups() prunes old backups purely by count. With
backupcount set to 1 that sequence deletes the last good backup, and the next boot
walks through the recovery loop in init(), finds nothing parseable, and lands on
the factory config. With the default of 100 you "only" lose the revision silently
and the system quietly restores an older backup on the next boot.

The both-files-broken scenario isn't hypothetical — 680846d already dealt with
the kernel-crash variant of it.

What this does:

save() builds the XML string once, checks the truncate, compares the byte count
from fwrite() against strlen(), and adds an fsync() so the data isn't still
sitting in the page cache while we start copying backups around. On any failure it
releases the lock and throws ConfigExceptionwrite_config() on the legacy side
already catches exactly that, so callers get a real failure instead of a thumbs up.

backup() now returns null when the source read or the copy fails, and unlinks the
partial file so it can't shadow the intact older backups during boot-time recovery.
Backup pruning is skipped whenever the new backup didn't make it to disk, so a
failed save can never eat the history anymore. A failed backup no longer passes in
silence either: the config was written, so save() doesn't throw for it, but it does
log an error now (previously the audit trail would just show a backup that was in
fact broken).

One thing reviewers may care about: backup() is public, so any out-of-tree caller
that assumed it always returns a string needs to cope with null on failure now. In
core the only caller is save() itself.

What it deliberately doesn't do: switch to write-temp-then-rename. That would be the
textbook fix, but the whole locking model here is flock() on a persistent handle,
plus hasChanged() doing fstat() on it, plus loadFromStream() readers waiting
on the writer's lock — rename swaps the inode out from under all of that. Overwriting
in place while keeping the handle open is load-bearing, so I stayed inside that
design.

fsync() needs PHP 8.1, which we are comfortably past.

🤖 Generated with Claude Code

save() ignored the results of ftruncate()/fwrite()/fflush(), so a short
write (disk full, I/O error) would leave a truncated config.xml behind
while reporting success. backup() then archived that damaged file via
File::file_put_contents() -- also unchecked -- and cleanupBackups()
pruned the older intact backups regardless. With a backup count of one
this can delete the last recoverable configuration and send the next
boot to factory defaults; with default settings it still loses the
revision silently.

Verify the written byte count, fsync before touching the backups, throw
ConfigException on failure (write_config() already handles that), drop
partial backup files instead of letting them shadow intact ones, and
skip backup pruning whenever the new backup did not make it to disk.

The in-place write stays: replacing it with rename() would break the
flock()/fstat() coordination on the persistent file handle that every
reader and writer relies on.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AdSchellevis

Copy link
Copy Markdown
Member

Although in theory it might sound nice to guard this code, in reality trying to fix a full disk is rather futile, removing a broken backup file isn't a huge issue, but being able to restore anything useful is probably not possible anyway in that case. If there is a real world scenario where it makes sense to guard this further I'm all open for it, but in practice there haven't been reports on these parts in a very long time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants