Improve configuration resistance to crashes

[welp-ops]

When qBittorrent saves its configuration, it:

1. Uses QSettings to write to qBittorrent_new.conf.
2. Renames qBittorrent_new.conf to qBittorrent.conf.
3. Deletes qBittorrent_new.conf

See TransactionalSettings::write in settingsstorage.cpp

This method successfully ensures that qBittorrent.conf is never overwritten by an invalid config, if for some reason qBit crashes while writing the config. However, I take issue with what happens when qBittorrent recovers from a crash:

1. Checks if qBittorrent_new.conf exists.
2. If so, copies qBittorrent_new.conf to qBittorrent.conf

See TransactionalSettings::read.

I.e., if qBittorrent crashes and leaves a corrupt configuration file in qBittorrent_new.conf, that corrupt configuration file will overwrite the valid configuration file upon startup. The comment in the file indicates that the original author was worried about the case when qBittorrent_new.conf was written fully, then qBit crashed before/during the rename. However, in my opinion, the correct behavior is to ignore qBittorrent_new.conf and just use the old conf after a crash, since it's much more likely that qBit crashed during writing the config than during renaming. Additionally, ignoring qBittorrent_new.conf is the conservative choice, in that it cannot corrupt the configuration, while renaming an unknown _new file can corrupt the configuration.

Am I correct in identifying the behavior in TransactionalSettings::read as incorrect?

Are the other configuration files handled atomically?

[NotTsunami]

No, this is not correct. Because the write is serialized, it won't leave half-written files (see the comment at the top of settingsstorage.cpp).

The only other bad fallthrough I could see would be crashing between Utils::Fs::forceRemove(finalPath); and return QFile::rename(newPath, finalPath); in TransactionalSettings::write(const QVariantHash &data), which would leave both the old and new configuration files, but even this pitfall is captured by Utils::Fs::forceRemove(finalPath) in QVariantHash TransactionalSettings::read() because forceRemove checks if the file exists first, so we wouldn't be trying to remove a non-existant file.

tl;dr the scenario you presented is impossible with the current code

See following comments

[welp-ops]

Because the write is serialized, it won't leave half-written files (see the comment at the top of settingsstorage.cpp).

I think you are misunderstanding the comment. You are interpreting it as "write() does not leave any half-written files", but I am interpreting it as "write() does not leave the main qBittorrent.conf half-written." If you look at the implementation of TransactionalSettings::serialize, it just calls settings->setValue in a loop. Qt writes the configuration to disk every time setValue is called (though it's free to buffer these calls as it sees fit). If there's a crash after the loop is partway complete, the _new file will exist but only contain part of the user's config. So I maintain that an interruption during TransactionalSettings::write() could cause the qBittorrent_new.conf file to be corrupted, and a subsequent call to TransactionalSettings::read() will overwrite the valid config with the corrupted one.

[FranciscoPombal]

Correct. The only robust way to ensure corruption resistance is to use a proper DB engine for settings storage, such as sqlite (this is actually quite an old idea, but its implementation was never finished in qBittorrent EDIT: but in commits that include #14726 and later as an experimental option, at least for fastresumes).

https://www.sqlite.org/howtocorrupt.html is very a very educational read. Ultimately, no mechanism can provide 100% immunity against corruption (for example, no software can protect against data corruption due to faulty RAM flipping bits). But the "usual" cases that every user expects to be covered, i.e. application/OS crash or power failure, are all handled by SQLite.

[welp-ops]

True, without a db engine it will be hard to get the level of crash resistance that SQLite provides. But that doesn't change the claim of my original post, which is that the specific type of corruption that the code is designed to mitigate is in fact not mitigated, and could be easily fixed.