From 1d764e6138dbe3af97b4ae8fc5ebac4c8b2f8a9e Mon Sep 17 00:00:00 2001 From: Victor Costan Date: Mon, 21 Mar 2022 21:37:49 +0000 Subject: [PATCH] sql: Switch recovery module to use SQLITE_ERROR on bad queries. //sql/recovery_module used SQLITE_MISUSE to signal receiving incorrect SQL statements. However, SQLITE_MISUSE is intended for incorrect use of the SQLite C API, and is mostly used to signal buggy lifecycle management. This CL switches //sql/recovery_module away from SQLITE_MISUSE. SQLITE_ERROR is used instead, matching SQLite's behavior on SQL statement issues such as incorrect row names and bad syntax. Change-Id: I9b879c3351801d475898576bf7f6b3d6c5755676 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3536962 Auto-Submit: Victor Costan Reviewed-by: Austin Sullivan Commit-Queue: Austin Sullivan Cr-Commit-Position: refs/heads/main@{#983500} --- sql/recover_module/module.cc | 10 +++++----- sql/recover_module/module_unittest.cc | 18 +++++++++--------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/sql/recover_module/module.cc b/sql/recover_module/module.cc index d05fb3bb6eeef1..5e85b7916019c4 100644 --- a/sql/recover_module/module.cc +++ b/sql/recover_module/module.cc @@ -60,7 +60,7 @@ int ModuleCreate(sqlite3* sqlite_db, DCHECK(sqlite_db != nullptr); if (argc <= kFirstColumnArgument) { // The recovery module needs at least one column specification. - return SQLITE_MISUSE; + return SQLITE_ERROR; } DCHECK(argv != nullptr); DCHECK(result_sqlite_table != nullptr); @@ -76,7 +76,7 @@ int ModuleCreate(sqlite3* sqlite_db, // temporary database (ATTACH '' AS other_temp). However, there is no easy // way to determine if an attachment point corresponds to a temporary // database, and "temp" is sufficient for Chrome's purposes. - return SQLITE_MISUSE; + return SQLITE_ERROR; } base::StringPiece table_name(argv[kVirtualTableNameArgument]); @@ -84,7 +84,7 @@ int ModuleCreate(sqlite3* sqlite_db, // In the future, we may deploy UMA metrics that use the virtual table name // to attribute recovery events to Chrome features. In preparation for that // future, require all recovery table names to start with "recover_". - return SQLITE_MISUSE; + return SQLITE_ERROR; } TargetTableSpec backing_table_spec = @@ -92,13 +92,13 @@ int ModuleCreate(sqlite3* sqlite_db, if (!backing_table_spec.IsValid()) { // The parser concluded that the string specifying the backing table is // invalid. This is definitely an error in the SQL using the virtual table. - return SQLITE_MISUSE; + return SQLITE_ERROR; } std::vector column_specs = ParseColumnSpecs(argc, argv); if (column_specs.empty()) { // The column specifications were invalid. - return SQLITE_MISUSE; + return SQLITE_ERROR; } auto [sqlite_status, table] = VirtualTable::Create( diff --git a/sql/recover_module/module_unittest.cc b/sql/recover_module/module_unittest.cc index 2124295c567e0b..049c58632fc745 100644 --- a/sql/recover_module/module_unittest.cc +++ b/sql/recover_module/module_unittest.cc @@ -63,7 +63,7 @@ TEST_F(RecoverModuleTest, CreateVtableFailsOnNonTempTable) { ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)")); { sql::test::ScopedErrorExpecter error_expecter; - error_expecter.ExpectError(SQLITE_MISUSE); + error_expecter.ExpectError(SQLITE_ERROR); EXPECT_FALSE(db_.Execute( "CREATE VIRTUAL TABLE recover_backing USING recover(backing, t TEXT)")); EXPECT_TRUE(error_expecter.SawExpectedErrors()); @@ -106,7 +106,7 @@ TEST_F(RecoverModuleTest, CreateVtableFailsOnMissingTableName) { ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)")); { sql::test::ScopedErrorExpecter error_expecter; - error_expecter.ExpectError(SQLITE_MISUSE); + error_expecter.ExpectError(SQLITE_ERROR); EXPECT_FALSE( db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing " "USING recover(main., t TEXT)")); @@ -117,7 +117,7 @@ TEST_F(RecoverModuleTest, CreateVtableFailsOnMissingSchemaSpec) { ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)")); { sql::test::ScopedErrorExpecter error_expecter; - error_expecter.ExpectError(SQLITE_MISUSE); + error_expecter.ExpectError(SQLITE_ERROR); EXPECT_FALSE( db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing " "USING recover(backing)")); @@ -128,7 +128,7 @@ TEST_F(RecoverModuleTest, CreateVtableFailsOnMissingDbName) { ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)")); { sql::test::ScopedErrorExpecter error_expecter; - error_expecter.ExpectError(SQLITE_MISUSE); + error_expecter.ExpectError(SQLITE_ERROR); EXPECT_FALSE( db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing " "USING recover(.backing)")); @@ -168,7 +168,7 @@ TEST_F(RecoverModuleTest, ColumnTypeMappingAnyStrict) { ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)")); { sql::test::ScopedErrorExpecter error_expecter; - error_expecter.ExpectError(SQLITE_MISUSE); + error_expecter.ExpectError(SQLITE_ERROR); EXPECT_FALSE( db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing " "USING recover(backing, t ANY STRICT)")); @@ -180,7 +180,7 @@ TEST_F(RecoverModuleTest, ColumnTypeExtraKeyword) { ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)")); { sql::test::ScopedErrorExpecter error_expecter; - error_expecter.ExpectError(SQLITE_MISUSE); + error_expecter.ExpectError(SQLITE_ERROR); EXPECT_FALSE( db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing " "USING recover(backing, t INTEGER SOMETHING)")); @@ -191,7 +191,7 @@ TEST_F(RecoverModuleTest, ColumnTypeNotNullExtraKeyword) { ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)")); { sql::test::ScopedErrorExpecter error_expecter; - error_expecter.ExpectError(SQLITE_MISUSE); + error_expecter.ExpectError(SQLITE_ERROR); EXPECT_FALSE( db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing " "USING recover(backing, t INTEGER NOT NULL SOMETHING)")); @@ -202,7 +202,7 @@ TEST_F(RecoverModuleTest, ColumnTypeDoubleTypes) { ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)")); { sql::test::ScopedErrorExpecter error_expecter; - error_expecter.ExpectError(SQLITE_MISUSE); + error_expecter.ExpectError(SQLITE_ERROR); EXPECT_FALSE( db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing " "USING recover(backing, t INTEGER FLOAT)")); @@ -213,7 +213,7 @@ TEST_F(RecoverModuleTest, ColumnTypeNotNullDoubleTypes) { ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)")); { sql::test::ScopedErrorExpecter error_expecter; - error_expecter.ExpectError(SQLITE_MISUSE); + error_expecter.ExpectError(SQLITE_ERROR); EXPECT_FALSE( db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing USING recover(" "backing, t INTEGER NOT NULL TEXT)"));