From 7c234825d7e901178f2039113f9b3f294165ca52 Mon Sep 17 00:00:00 2001 From: Victor Costan Date: Tue, 13 Jul 2021 03:03:02 +0000 Subject: [PATCH] sql: Disable trigger execution for sql::Database users. The use of SQLite triggers is discouraged in Chrome feature code. This CL disables trigger execution from databases that use sql::Database infrastructure -- this applies to all Chrome feature code, but not to WebSQL, which uses its own SQLite abstraction. CREATE TRIGGER and DROP TRIGGER will still work, but the triggers will not be executed. Disabling trigger execution in Chrome features is the best we can do for now, because WebSQL still needs SQLite support for triggers. After we remove WebSQL, we can remove all trigger support from SQLite. Bug: 910955 Change-Id: I1f994c67341e44253f1f78188a1eaa7c4d966f88 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3020167 Commit-Queue: Marijn Kruisselbrink Auto-Submit: Victor Costan Reviewed-by: Marijn Kruisselbrink Cr-Commit-Position: refs/heads/master@{#900805} --- sql/README.md | 4 ++++ sql/database.cc | 6 ++++++ sql/database_unittest.cc | 21 +++++++++++++++++++++ sql/recovery_unittest.cc | 15 +-------------- 4 files changed, 32 insertions(+), 14 deletions(-) diff --git a/sql/README.md b/sql/README.md index eb73d8cddae751..25ecda4aa684bd 100644 --- a/sql/README.md +++ b/sql/README.md @@ -393,6 +393,10 @@ to disable SQLite's CHECK constraint support using Triggers significantly increase the difficulty of reviewing and maintaining Chrome features that use them. +Triggers are not executed on SQLite databases opened with Chrome's +`sql::Database` infrastructure. This is intended to steer feature developers +away from the discouraged feature. + After [WebSQL](https://www.w3.org/TR/webdatabase/) is removed from Chrome, we plan to disable SQLite's trigger support using [SQLITE_OMIT_TRIGGER](https://sqlite.org/compile.html#omit_trigger). diff --git a/sql/database.cc b/sql/database.cc index 1137116afedb2b..f0f315617e5b94 100644 --- a/sql/database.cc +++ b/sql/database.cc @@ -1497,6 +1497,12 @@ bool Database::OpenInternal(const std::string& file_name, return false; } + // The use of triggers is discouraged for Chrome code. Thanks to this + // configuration change, triggers are not executed. CREATE TRIGGER and DROP + // TRIGGER still succeed. + err = sqlite3_db_config(db_, SQLITE_DBCONFIG_ENABLE_TRIGGER, 0, nullptr); + DCHECK_EQ(err, SQLITE_OK) << "sqlite3_db_config() should not fail"; + // Enable extended result codes to provide more color on I/O errors. // Not having extended result codes is not a fatal problem, as // Chromium code does not attempt to handle I/O errors anyhow. The diff --git a/sql/database_unittest.cc b/sql/database_unittest.cc index c7d85020c21b55..fa785bb88d5cb1 100644 --- a/sql/database_unittest.cc +++ b/sql/database_unittest.cc @@ -1387,6 +1387,27 @@ TEST_P(SQLDatabaseTest, GetMemoryUsage) { << "Page cache usage should go down after calling TrimMemory()"; } +TEST_P(SQLDatabaseTest, TriggersDisabledByDefault) { + ASSERT_TRUE(db_->Execute("CREATE TABLE data(id INTEGER)")); + + // sqlite3_db_config() currently only disables running triggers. Schema + // operations on triggers are still allowed. + EXPECT_TRUE( + db_->Execute("CREATE TRIGGER trigger AFTER INSERT ON data " + "BEGIN DELETE FROM data; END")); + + ASSERT_TRUE(db_->Execute("INSERT INTO data(id) VALUES(42)")); + + Statement select(db_->GetUniqueStatement("SELECT id FROM data")); + EXPECT_TRUE(select.Step()) + << "If the trigger did not run, the table should not be empty."; + EXPECT_EQ(42, select.ColumnInt64(0)); + + // sqlite3_db_config() currently only disables running triggers. Schema + // operations on triggers are still allowed. + EXPECT_TRUE(db_->Execute("DROP TRIGGER IF EXISTS trigger")); +} + class SQLDatabaseTestExclusiveMode : public testing::Test, public testing::WithParamInterface { public: diff --git a/sql/recovery_unittest.cc b/sql/recovery_unittest.cc index d5c541b91947cf..3f37a27482dfc6 100644 --- a/sql/recovery_unittest.cc +++ b/sql/recovery_unittest.cc @@ -780,16 +780,10 @@ TEST_F(SQLRecoveryTest, RecoverDatabase) { ASSERT_TRUE( db_.Execute("CREATE VIEW v AS SELECT x.v FROM x, y WHERE x.v = y.v")); - // When an element is deleted from [x], trigger a delete on [y]. Between the - // BEGIN and END, [old] stands for the deleted rows from [x]. - ASSERT_TRUE( - db_.Execute("CREATE TRIGGER t AFTER DELETE ON x " - "BEGIN DELETE FROM y WHERE y.v = old.v; END")); - // Save aside a copy of the original schema, verifying that it has the created // items plus the sqlite_sequence table. const std::string orig_schema(GetSchema(&db_)); - ASSERT_EQ(6, std::count(orig_schema.begin(), orig_schema.end(), '\n')); + ASSERT_EQ(5, std::count(orig_schema.begin(), orig_schema.end(), '\n')); static const char kXSql[] = "SELECT * FROM x ORDER BY 1"; static const char kYSql[] = "SELECT * FROM y ORDER BY 1"; @@ -815,13 +809,6 @@ TEST_F(SQLRecoveryTest, RecoverDatabase) { EXPECT_EQ("bob|truck\ndean|trailer\njim|telephone", ExecuteWithResults(&db_, kYSql, "|", "\n")); EXPECT_EQ("trailer\ntruck", ExecuteWithResults(&db_, kVSql, "|", "\n")); - - // Test that the trigger works. - ASSERT_TRUE(db_.Execute("DELETE FROM x WHERE v = 'truck'")); - EXPECT_EQ("1|turtle\n3|trailer", ExecuteWithResults(&db_, kXSql, "|", "\n")); - EXPECT_EQ("dean|trailer\njim|telephone", - ExecuteWithResults(&db_, kYSql, "|", "\n")); - EXPECT_EQ("trailer", ExecuteWithResults(&db_, kVSql, "|", "\n")); } // When RecoverDatabase() encounters SQLITE_NOTADB, the database is deleted.