From 88577f41d3b59098cc6bde90e465416724e994cb Mon Sep 17 00:00:00 2001 From: Cole Eason Date: Thu, 12 Dec 2024 12:29:47 -0500 Subject: [PATCH 1/2] Disallow more non-deterministic queries in writes --- sqlitecluster/SQLite.cpp | 13 ++++++++++--- sqlitecluster/SQLite.h | 9 +++++++++ test/tests/WriteTest.cpp | 16 ++++++++++++++-- 3 files changed, 33 insertions(+), 5 deletions(-) diff --git a/sqlitecluster/SQLite.cpp b/sqlitecluster/SQLite.cpp index e4f63acb2..8b9115535 100644 --- a/sqlitecluster/SQLite.cpp +++ b/sqlitecluster/SQLite.cpp @@ -988,14 +988,21 @@ int SQLite::_authorize(int actionCode, const char* detail1, const char* detail2, !strcmp(detail2, "strftime") || !strcmp(detail2, "changes") || !strcmp(detail2, "last_insert_rowid") || - !strcmp(detail2, "sqlite3_version") + !strcmp(detail2, "sqlite_version") ) { _isDeterministicQuery = false; } - if (!strcmp(detail2, "current_timestamp")) { + // Prevent using certain non-deterministic functions in writes which could cause synchronization with followers to + // result in inconsistent data. Some are not included here because they can be used in a deterministic way that is valid. + // i.e. you can do UPDATE x = DATE('2024-01-01') and its deterministic whereas UPDATE x = DATE('now') is not. It's up to + // callers to prevent using these functions inappropriately. + if (!strcmp(detail2, "current_timestamp") || + !strcmp(detail2, "random") || + !strcmp(detail2, "last_insert_rowid") || + !strcmp(detail2, "changes") || + !strcmp(detail2, "sqlite_version")) { if (_currentlyWriting) { - // Prevent using `current_timestamp` in writes which could cause synchronization with followers to result in inconsistent data. return SQLITE_DENY; } } diff --git a/sqlitecluster/SQLite.h b/sqlitecluster/SQLite.h index 842b51430..50fb4b99c 100644 --- a/sqlitecluster/SQLite.h +++ b/sqlitecluster/SQLite.h @@ -468,6 +468,15 @@ class SQLite { void _checkInterruptErrors(const string& error) const; // Called internally by _sqliteAuthorizerCallback to authorize columns for a query. + // + // PRO-TIP: you can play with the authorizer using the `sqlite3` CLI tool, by running `.auth ON` then running + // your query. The columns displayed are the same as what is passed to this function. + // + // The information passed to this function is different based on the first parameter, actionCode. + // You can see what information is passed for each action code here https://www.sqlite.org/c3ref/c_alter_table.html. + // Note that as of writing this comment, the page seems slightly out of date and the parameter numbers are all off + // by one. That is, the first paramter passed to the callback funciton is actually the integer action code, not the + // second. int _authorize(int actionCode, const char* detail1, const char* detail2, const char* detail3, const char* detail4); // It's possible for certain transactions (namely, timing out a write operation, see here: diff --git a/test/tests/WriteTest.cpp b/test/tests/WriteTest.cpp index 4beef02ab..f50d99b88 100644 --- a/test/tests/WriteTest.cpp +++ b/test/tests/WriteTest.cpp @@ -18,7 +18,7 @@ struct WriteTest : tpunit::TestFixture { TEST(WriteTest::updateAndInsertWithHttp), TEST(WriteTest::shortHandSyntax), TEST(WriteTest::keywordsAsValue), - TEST(WriteTest::blockTimeFunctions), + TEST(WriteTest::blockNonDeterministicFunctions), AFTER_CLASS(WriteTest::tearDown)) { } BedrockTester* tester; @@ -181,7 +181,7 @@ struct WriteTest : tpunit::TestFixture { tester->executeWaitVerifyContent(query3); } - void blockTimeFunctions() { + void blockNonDeterministicFunctions() { // Verify writing the string 'CURRENT_TIMESTAMP' is fine. SData query("query: INSERT INTO stuff VALUES ( NULL, 11, 'CURRENT_TIMESTAMP' );"); tester->executeWaitVerifyContent(query); @@ -193,6 +193,18 @@ struct WriteTest : tpunit::TestFixture { // But allow the function to run in reads. query.methodLine = "query: SELECT CURRENT_TIMESTAMP;"; tester->executeWaitVerifyContent(query); + + // Verify writing the string 'RANDOM' is fine. + query.methodLine = "query: INSERT INTO stuff VALUES ( NULL, 11, 'RANDOM' );"; + tester->executeWaitVerifyContent(query); + + // But verify calling the function RANDOM is blocked when writing. + query.methodLine = "query: INSERT INTO stuff VALUES ( NULL, 11, RANDOM() );"; + tester->executeWaitVerifyContent(query, "502 Query failed"); + + // But allow the function to run in reads. + query.methodLine = "query: SELECT random();"; + tester->executeWaitVerifyContent(query); } } __WriteTest; From 5e18907d94d2159e1a0f76d1bcb7518d3d40299d Mon Sep 17 00:00:00 2001 From: Cole Eason Date: Thu, 12 Dec 2024 12:44:23 -0500 Subject: [PATCH 2/2] Update test that uses non-dertministic function --- test/tests/WriteTest.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/tests/WriteTest.cpp b/test/tests/WriteTest.cpp index f50d99b88..46105532c 100644 --- a/test/tests/WriteTest.cpp +++ b/test/tests/WriteTest.cpp @@ -1,4 +1,5 @@ #include +#include #include struct WriteTest : tpunit::TestFixture { @@ -38,7 +39,8 @@ struct WriteTest : tpunit::TestFixture { for (int i = 0; i < 50; i++) { SData query("Query"); query["writeConsistency"] = "ASYNC"; - query["query"] = "INSERT INTO foo VALUES ( RANDOM() );"; + uint64_t rand = SRandom::rand64(); + query["query"] = "INSERT INTO foo VALUES (" + to_string(rand) + ");"; tester->executeWaitVerifyContent(query); }