You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up from the packaging review in #276, kept out of that PR deliberately.
Problem
packaging/arch/forkd.logrotate rotates /var/log/forkd/audit.log with copytruncate. That is currently the only workable policy, and the choice is correct given today's code — but it has a failure mode worth recording rather than rediscovering.
copytruncate copies the file, then truncates the original. Any line the daemon writes between the copy and the truncate is silently lost. For an ordinary application log a small gap is a nuisance; for an append-only audit log it means a request can be served and its audit record can vanish, with nothing anywhere indicating a record ever existed. That is exactly the property an audit log is supposed to rule out.
Why copytruncate is currently forced
AuditSink::open opens the file once and the process holds the handle for its whole lifetime (crates/forkd-controller/src/audit.rs:36-53):
let file = OpenOptions::new().create(true).append(true).open(&path)
spawn_shutdown_signal (crates/forkd-controller/src/lib.rs:204-228) installs handlers for SIGINT and SIGTERM only. There is no signal that tells the daemon to reopen its log, so a postrotate policy — the normal way to avoid the race — has nothing to call. logrotate would rename the file and the daemon would keep writing to the now-unlinked inode, which is worse than the copytruncate gap.
One thing that is not a problem here: because the handle is O_APPEND, writes after truncation resume at offset 0 rather than at the old offset, so this does not produce the NUL-padded sparse file that copytruncate causes with non-append writers. The lost-lines race is the whole issue.
Proposed fix
Add a SIGHUP handler that reopens the audit sink, then switch the logrotate policy to postrotate + systemctl reload forkd-controller.
The groundwork is already there — AuditInner stores path alongside the writer, so a reopen is roughly:
Holding the same Mutex that write takes means a rotation cannot interleave with a line being written, which closes the race rather than narrowing it. The unit would also need ExecReload=/bin/kill -HUP $MAINPID.
Notes
Worth emitting an audit line on reopen (something like {"event":"log_reopened"}) so a rotation is visible in the log itself and a gap can be distinguished from a truncation.
Until this lands, copytruncate should stay — it is strictly better than the rename-and-keep-writing alternative. No change to packaging: add Arch forkd-bin recipe #276 is implied.
write() already flushes per line, so there is no additional buffered-data exposure beyond the race described above.
Follow-up from the packaging review in #276, kept out of that PR deliberately.
Problem
packaging/arch/forkd.logrotaterotates/var/log/forkd/audit.logwithcopytruncate. That is currently the only workable policy, and the choice is correct given today's code — but it has a failure mode worth recording rather than rediscovering.copytruncatecopies the file, then truncates the original. Any line the daemon writes between the copy and the truncate is silently lost. For an ordinary application log a small gap is a nuisance; for an append-only audit log it means a request can be served and its audit record can vanish, with nothing anywhere indicating a record ever existed. That is exactly the property an audit log is supposed to rule out.Why
copytruncateis currently forcedAuditSink::openopens the file once and the process holds the handle for its whole lifetime (crates/forkd-controller/src/audit.rs:36-53):spawn_shutdown_signal(crates/forkd-controller/src/lib.rs:204-228) installs handlers for SIGINT and SIGTERM only. There is no signal that tells the daemon to reopen its log, so apostrotatepolicy — the normal way to avoid the race — has nothing to call.logrotatewould rename the file and the daemon would keep writing to the now-unlinked inode, which is worse than thecopytruncategap.One thing that is not a problem here: because the handle is
O_APPEND, writes after truncation resume at offset 0 rather than at the old offset, so this does not produce the NUL-padded sparse file thatcopytruncatecauses with non-append writers. The lost-lines race is the whole issue.Proposed fix
Add a SIGHUP handler that reopens the audit sink, then switch the logrotate policy to
postrotate+systemctl reload forkd-controller.The groundwork is already there —
AuditInnerstorespathalongside the writer, so a reopen is roughly:Holding the same
Mutexthatwritetakes means a rotation cannot interleave with a line being written, which closes the race rather than narrowing it. The unit would also needExecReload=/bin/kill -HUP $MAINPID.Notes
{"event":"log_reopened"}) so a rotation is visible in the log itself and a gap can be distinguished from a truncation.copytruncateshould stay — it is strictly better than the rename-and-keep-writing alternative. No change to packaging: add Arch forkd-bin recipe #276 is implied.write()already flushes per line, so there is no additional buffered-data exposure beyond the race described above.