From d6b60ac2cd5402ca45d042136c85418ce3d4d9f8 Mon Sep 17 00:00:00 2001 From: Evan Hahn <69474926+EvanHahn-Signal@users.noreply.github.com> Date: Fri, 21 Apr 2023 16:10:35 -0500 Subject: [PATCH] Add `F_BARRIERFSYNC` remote config kill switch --- .../Database/GRDBDatabaseStorageAdapter.swift | 4 +-- .../src/Util/RemoteConfigManager.swift | 35 +++++++++++++++++-- SignalServiceKit/src/Util/SqliteUtil.swift | 22 ++++++++++++ .../tests/Util/SqliteUtilTest.swift | 17 +++++++++ 4 files changed, 73 insertions(+), 5 deletions(-) diff --git a/SignalServiceKit/src/Storage/Database/GRDBDatabaseStorageAdapter.swift b/SignalServiceKit/src/Storage/Database/GRDBDatabaseStorageAdapter.swift index 91ff7325b1..c309286b90 100644 --- a/SignalServiceKit/src/Storage/Database/GRDBDatabaseStorageAdapter.swift +++ b/SignalServiceKit/src/Storage/Database/GRDBDatabaseStorageAdapter.swift @@ -380,9 +380,7 @@ public class GRDBDatabaseStorageAdapter: NSObject { try db.execute(sql: "PRAGMA key = \"\(keyspec)\"") try db.execute(sql: "PRAGMA cipher_plaintext_header_size = 32") try db.execute(sql: "PRAGMA checkpoint_fullfsync = ON") - - // This will use F_BARRIERFSYNC because of our fork of SQLCipher. - try db.execute(sql: "PRAGMA fullfsync = ON") + try SqliteUtil.setBarrierFsync(db: db, enabled: true) if !CurrentAppContext().isMainApp { let perConnectionCacheSizeInKibibytes = 2000 / (GRDBStorage.maximumReaderCountInExtensions + 1) diff --git a/SignalServiceKit/src/Util/RemoteConfigManager.swift b/SignalServiceKit/src/Util/RemoteConfigManager.swift index aa791e3349..ad6f6ba82f 100644 --- a/SignalServiceKit/src/Util/RemoteConfigManager.swift +++ b/SignalServiceKit/src/Util/RemoteConfigManager.swift @@ -448,8 +448,7 @@ private struct Flags { // as soon as we fetch an update to the remote config. They will not // wait for an app restart. enum HotSwappableIsEnabledFlags: String, FlagType { - // This can't be empty, so we define a bogus case. Remove this if you add a flag here. - case __noHotSwappableIsEnabledFlags + case barrierFsyncKillSwitch } // We filter the received config down to just the supported flags. @@ -458,6 +457,7 @@ private struct Flags { // a sticky flag to 100% in beta then turn it back to 0% before going // to production. enum SupportedIsEnabledFlags: String, FlagType { + case barrierFsyncKillSwitch case uuidSafetyNumbers case automaticSessionResetKillSwitch case paymentsResetKillSwitch @@ -661,9 +661,14 @@ public class ServiceRemoteConfigManager: RemoteConfigManager { private func cacheCurrent() { var isEnabledFlags = [String: Bool]() var valueFlags = [String: AnyObject]() + var isUsingBarrierFsync = false + db.read { transaction in isEnabledFlags = self.keyValueStore.getRemoteConfigIsEnabledFlags(transaction: transaction) ?? [:] valueFlags = self.keyValueStore.getRemoteConfigValueFlags(transaction: transaction) ?? [:] + isUsingBarrierFsync = SqliteUtil.isUsingBarrierFsync( + db: SDSDB.shimOnlyBridge(transaction).unwrapGrdbRead.database + ) } if !isEnabledFlags.isEmpty || !valueFlags.isEmpty { @@ -673,6 +678,22 @@ public class ServiceRemoteConfigManager: RemoteConfigManager { Logger.info("no stored remote config") } + // This will be tripped in the unlikely event that the kill switch is enabled, + // but typically won't result in a write. + let shouldUseBarrierFsync: Bool = { + let rawFlag = Flags.HotSwappableIsEnabledFlags.barrierFsyncKillSwitch.rawValue + let isKilled = isEnabledFlags[rawFlag] ?? false + return !isKilled + }() + if shouldUseBarrierFsync != isUsingBarrierFsync { + self.db.write { tx in + try? SqliteUtil.setBarrierFsync( + db: SDSDB.shimOnlyBridge(tx).unwrapGrdbRead.database, + enabled: shouldUseBarrierFsync + ) + } + } + checkClientExpiration(valueFlags: valueFlags) hasWarmedCache.set(true) @@ -793,9 +814,19 @@ public class ServiceRemoteConfigManager: RemoteConfigManager { } } + try? SqliteUtil.setBarrierFsync( + db: SDSDB.shimOnlyBridge(transaction).unwrapGrdbWrite.database, + enabled: { + let rawFlag = Flags.HotSwappableIsEnabledFlags.barrierFsyncKillSwitch.rawValue + let isKilled = isEnabledFlags[rawFlag] ?? false + return !isKilled + }() + ) + self.keyValueStore.setRemoteConfigIsEnabledFlags(isEnabledFlags, transaction: transaction) self.keyValueStore.setRemoteConfigValueFlags(valueFlags, transaction: transaction) self.keyValueStore.setLastFetched(Date(), transaction: transaction) + self.checkClientExpiration(valueFlags: valueFlags) } diff --git a/SignalServiceKit/src/Util/SqliteUtil.swift b/SignalServiceKit/src/Util/SqliteUtil.swift index 5aaaec510a..26173adf4f 100644 --- a/SignalServiceKit/src/Util/SqliteUtil.swift +++ b/SignalServiceKit/src/Util/SqliteUtil.swift @@ -40,6 +40,28 @@ public enum SqliteUtil { sqlName.range(of: "^[a-zA-Z][a-zA-Z0-9_]*$", options: .regularExpression) != nil } + /// Are we using `F_BARRIERFSYNC`? + /// + /// Under the hood, this calls [`PRAGMA fullfsync`][0]. You'd think that this would affect + /// `FULLFSYNC` instead, but we modify SQLCipher to replace `FULLFSYNC` with `FULLFSYNC` with + /// `BARRIERFSYNC`. This helps us balance reliability and performance. + /// + /// [0]: https://www.sqlite.org/pragma.html#pragma_fullfsync + public static func isUsingBarrierFsync(db: Database) -> Bool { + return (try? Bool.fetchOne(db, sql: "PRAGMA fullfsync")) ?? false + } + + /// Enable or disable `F_BARRIERFSYNC`. + /// + /// Under the hood, this calls [`PRAGMA fullfsync`][0]. You'd think that this would affect + /// `FULLFSYNC` instead, but we modify SQLCipher to replace `FULLFSYNC` with `FULLFSYNC` with + /// `BARRIERFSYNC`. This helps us balance reliability and performance. + /// + /// [0]: https://www.sqlite.org/pragma.html#pragma_fullfsync + public static func setBarrierFsync(db: Database, enabled: Bool) throws { + try db.execute(sql: "PRAGMA fullfsync = \(enabled ? "ON" : "OFF")") + } + public enum IntegrityCheckResult { case ok case notOk diff --git a/SignalServiceKit/tests/Util/SqliteUtilTest.swift b/SignalServiceKit/tests/Util/SqliteUtilTest.swift index d402eb04e5..0b3683f5ba 100644 --- a/SignalServiceKit/tests/Util/SqliteUtilTest.swift +++ b/SignalServiceKit/tests/Util/SqliteUtilTest.swift @@ -31,6 +31,23 @@ final class SqliteUtilTest: XCTestCase { } } + func testBarrierFsync() throws { + let databasePath = OWSFileSystem.temporaryFilePath(fileExtension: "sqlite") + + let databaseQueue = try DatabaseQueue(path: databasePath) + defer { try? databaseQueue.close() } + + try databaseQueue.write { db in + XCTAssertFalse(SqliteUtil.isUsingBarrierFsync(db: db)) + + try SqliteUtil.setBarrierFsync(db: db, enabled: true) + XCTAssertTrue(SqliteUtil.isUsingBarrierFsync(db: db)) + + try SqliteUtil.setBarrierFsync(db: db, enabled: false) + XCTAssertFalse(SqliteUtil.isUsingBarrierFsync(db: db)) + } + } + func testIntegrityCheckResult() { let ok = SqliteUtil.IntegrityCheckResult.ok let notOk = SqliteUtil.IntegrityCheckResult.notOk