diff --git a/azure-pipelines.yml b/azure-pipelines.yml index 2ec5faaded..4ac009a116 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -156,7 +156,7 @@ jobs: script: | $(PsExec.secureFilePath) -accepteula -s -i $(buildOutDir)\AppInstallerCLITests\AppInstallerCLITests.exe -logto $(artifactsDir)\AICLI-Unpackaged-System.log -s -r junit -o $(artifactsDir)\TEST-AppInstallerCLI-Unpackaged-System.xml workingDirectory: '$(buildOutDir)\AppInstallerCLITests' - continueOnError: true + condition: succeededOrFailed() - task: PowerShell@2 displayName: Create Package Layout @@ -164,7 +164,7 @@ jobs: filePath: 'src\AppInstallerCLIPackage\Execute-AppxRecipe.ps1' arguments: '-AppxRecipePath AppInstallerCLIPackage\bin\$(buildPlatform)\$(buildConfiguration)\AppInstallerCLIPackage.build.appxrecipe -LayoutPath $(packageLayoutDir) -Force -Verbose' workingDirectory: 'src' - continueOnError: true + condition: succeededOrFailed() - task: PowerShell@2 displayName: Run Unit Tests Packaged @@ -172,7 +172,7 @@ jobs: filePath: 'src\AppInstallerCLITests\Run-TestsInPackage.ps1' arguments: '-Args "~[pips]" -BuildRoot $(buildOutDir) -PackageRoot $(packageLayoutDir) -LogTarget $(artifactsDir)\AICLI-Packaged.log -TestResultsTarget $(artifactsDir)\TEST-AppInstallerCLI-Packaged.xml -ScriptWait' workingDirectory: 'src' - continueOnError: true + condition: succeededOrFailed() - task: PublishTestResults@2 displayName: Publish Unit Test Results @@ -180,6 +180,7 @@ jobs: testResultsFormat: 'JUnit' testResultsFiles: '$(artifactsDir)\TEST-*.xml' failTaskOnFailedTests: true + condition: succeededOrFailed() - task: MSBuild@1 displayName: Build MSIX Test Installer File @@ -191,6 +192,7 @@ jobs: /p:AppxBundle=Never /p:UapAppxPackageBuildMode=SideLoadOnly /p:AppxPackageSigningEnabled=false' + condition: succeededOrFailed() - task: PowerShell@2 displayName: 'Set program files directory' @@ -202,6 +204,7 @@ jobs: } else { Write-Host "##vso[task.setvariable variable=platformProgramFiles;]${env:ProgramFiles}" } + condition: succeededOrFailed() # Resolves resource strings utilized by InProc E2E tests. - task: CopyFiles@2 @@ -210,6 +213,7 @@ jobs: SourceFolder: '$(buildOutDir)\AppInstallerCLI' TargetFolder: '$(platformProgramFiles)\dotnet' Contents: resources.pri + condition: succeededOrFailed() # Winmd accessed by test runner process (dotnet.exe) - task: CopyFiles@2 @@ -218,6 +222,7 @@ jobs: SourceFolder: '$(buildOutDir)\Microsoft.Management.Deployment' TargetFolder: '$(platformProgramFiles)\dotnet' Contents: Microsoft.Management.Deployment.winmd + condition: succeededOrFailed() - template: templates/e2e-setup.yml parameters: diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 041a541a1b..eba36a6e48 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -2804,14 +2804,17 @@ Please specify one of them using the --source option to proceed. The installer technology in use does not support repair. - + The package installed for user scope cannot be repaired when running with administrator privileges. Repair operations involving administrator privileges are not permitted on packages installed within the user scope. - + Repair failed with exit code: {0} {Locked="{0}"} Error message displayed when an attempt to repair an application package fails. {0} is a placeholder replaced by an error code. + + The SQLite connection was terminated to prevent corruption. + \ No newline at end of file diff --git a/src/AppInstallerCLITests/SQLiteWrapper.cpp b/src/AppInstallerCLITests/SQLiteWrapper.cpp index d2ee2c2b76..bc597e19b7 100644 --- a/src/AppInstallerCLITests/SQLiteWrapper.cpp +++ b/src/AppInstallerCLITests/SQLiteWrapper.cpp @@ -312,6 +312,90 @@ TEST_CASE("SQLiteWrapper_PrepareFailure", "[sqlitewrapper]") REQUIRE_THROWS_HR(builder.Prepare(connection), MAKE_HRESULT(SEVERITY_ERROR, FACILITY_SQLITE, SQLITE_ERROR)); } +TEST_CASE("SQLiteWrapper_BusyTimeout_None", "[sqlitewrapper]") +{ + TestCommon::TempFile tempFile{ "repolibtest_tempdb"s, ".db"s }; + INFO("Using temporary file named: " << tempFile.GetPath()); + + wil::unique_event busy, done; + busy.create(); + done.create(); + + std::thread busyThread([&]() + { + Connection threadConnection = Connection::Create(tempFile, Connection::OpenDisposition::Create); + Statement threadStatement = Statement::Create(threadConnection, "BEGIN EXCLUSIVE TRANSACTION"); + threadStatement.Execute(); + busy.SetEvent(); + done.wait(500); + }); + busyThread.detach(); + + busy.wait(500); + + Connection testConnection = Connection::Create(tempFile, Connection::OpenDisposition::ReadWrite); + testConnection.SetBusyTimeout(0ms); + Statement testStatement = Statement::Create(testConnection, "BEGIN EXCLUSIVE TRANSACTION"); + REQUIRE_THROWS_HR(testStatement.Execute(), MAKE_HRESULT(SEVERITY_ERROR, FACILITY_SQLITE, SQLITE_BUSY)); + + done.SetEvent(); +} + +TEST_CASE("SQLiteWrapper_BusyTimeout_Some", "[sqlitewrapper]") +{ + TestCommon::TempFile tempFile{ "repolibtest_tempdb"s, ".db"s }; + INFO("Using temporary file named: " << tempFile.GetPath()); + + wil::unique_event busy, ready, done; + busy.create(); + ready.create(); + done.create(); + + std::thread busyThread([&]() + { + Connection threadConnection = Connection::Create(tempFile, Connection::OpenDisposition::Create); + Statement threadBeginStatement = Statement::Create(threadConnection, "BEGIN EXCLUSIVE TRANSACTION"); + Statement threadCommitStatement = Statement::Create(threadConnection, "COMMIT"); + threadBeginStatement.Execute(); + busy.SetEvent(); + ready.wait(500); + done.wait(100); + threadCommitStatement.Execute(); + }); + busyThread.detach(); + + busy.wait(500); + + Connection testConnection = Connection::Create(tempFile, Connection::OpenDisposition::ReadWrite); + testConnection.SetBusyTimeout(500ms); + Statement testStatement = Statement::Create(testConnection, "BEGIN EXCLUSIVE TRANSACTION"); + ready.SetEvent(); + testStatement.Execute(); + + done.SetEvent(); +} + +TEST_CASE("SQLiteWrapper_CloseConnectionOnError", "[sqlitewrapper]") +{ + Connection connection = Connection::Create(SQLITE_MEMORY_DB_CONNECTION_TARGET, Connection::OpenDisposition::Create); + + Builder::StatementBuilder builder; + builder.CreateTable(s_tableName).Columns({ + Builder::ColumnBuilder(s_firstColumn, Builder::Type::Int), + Builder::ColumnBuilder(s_secondColumn, Builder::Type::Text), + }); + + Statement createTable = builder.Prepare(connection); + REQUIRE_FALSE(createTable.Step()); + REQUIRE(createTable.GetState() == Statement::State::Completed); + + createTable.Reset(); + REQUIRE_THROWS(createTable.Step(true)); + + // Do anything that needs the connection + REQUIRE_THROWS_HR(connection.GetLastInsertRowID(), APPINSTALLER_CLI_ERROR_SQLITE_CONNECTION_TERMINATED); +} + TEST_CASE("SQLBuilder_SimpleSelectBind", "[sqlbuilder]") { Connection connection = Connection::Create(SQLITE_MEMORY_DB_CONNECTION_TARGET, Connection::OpenDisposition::Create); diff --git a/src/AppInstallerCLITests/TestCommon.cpp b/src/AppInstallerCLITests/TestCommon.cpp index fea68d8639..1364f49e4b 100644 --- a/src/AppInstallerCLITests/TestCommon.cpp +++ b/src/AppInstallerCLITests/TestCommon.cpp @@ -85,7 +85,7 @@ namespace TestCommon } } - TempFile::~TempFile() + TempFile::~TempFile() try { switch (s_TempFileDestructorBehavior) { @@ -99,6 +99,7 @@ namespace TestCommon break; } } + CATCH_LOG_RETURN() void TempFile::Rename(const std::filesystem::path& newFilePath) { diff --git a/src/AppInstallerSharedLib/Errors.cpp b/src/AppInstallerSharedLib/Errors.cpp index 70707ede34..1cfa20e909 100644 --- a/src/AppInstallerSharedLib/Errors.cpp +++ b/src/AppInstallerSharedLib/Errors.cpp @@ -213,6 +213,7 @@ namespace AppInstaller WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_EXEC_REPAIR_FAILED, "Repair operation failed."), WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_REPAIR_NOT_SUPPORTED, "The installer technology in use doesn't support repair."), WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_ADMIN_CONTEXT_REPAIR_PROHIBITED, "Repair operations involving administrator privileges are not permitted on packages installed within the user scope."), + WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_SQLITE_CONNECTION_TERMINATED, "The SQLite connection was terminated to prevent corruption."), // Install errors. WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALL_PACKAGE_IN_USE, "Application is currently running. Exit the application then try again."), diff --git a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h index 06ca30cecb..d8fc1c51b8 100644 --- a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h +++ b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h @@ -143,6 +143,7 @@ #define APPINSTALLER_CLI_ERROR_EXEC_REPAIR_FAILED ((HRESULT)0x8A15007B) #define APPINSTALLER_CLI_ERROR_REPAIR_NOT_SUPPORTED ((HRESULT)0x8A15007C) #define APPINSTALLER_CLI_ERROR_ADMIN_CONTEXT_REPAIR_PROHIBITED ((HRESULT)0x8A15007D) +#define APPINSTALLER_CLI_ERROR_SQLITE_CONNECTION_TERMINATED ((HRESULT)0x8A15007E) // Install errors. #define APPINSTALLER_CLI_ERROR_INSTALL_PACKAGE_IN_USE ((HRESULT)0x8A150101) diff --git a/src/AppInstallerSharedLib/Public/winget/SQLiteWrapper.h b/src/AppInstallerSharedLib/Public/winget/SQLiteWrapper.h index 68182182e3..a5f966347d 100644 --- a/src/AppInstallerSharedLib/Public/winget/SQLiteWrapper.h +++ b/src/AppInstallerSharedLib/Public/winget/SQLiteWrapper.h @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -125,6 +126,23 @@ namespace AppInstaller::SQLite template using ParameterSpecifics = ParameterSpecificsImpl>; + + // Allows the connection to be shared so that it can be closed in some circumstances. + struct SharedConnection + { + // Disables the connection, causing an exception to be thrown by `get`. + void Disable(); + + // Gets the connection object if active. + sqlite3* Get() const; + + // Gets the connection object for creation. + sqlite3** GetPtr(); + + private: + std::atomic_bool m_active = true; + wil::unique_any m_dbconn; + }; } // A SQLite exception. @@ -133,9 +151,13 @@ namespace AppInstaller::SQLite SQLiteException(int error) : wil::ResultException(MAKE_HRESULT(SEVERITY_ERROR, FACILITY_SQLITE, error)) {} }; + struct Statement; + // The connection to a database. struct Connection { + friend Statement; + // The disposition for opening a database connection. enum class OpenDisposition : int { @@ -180,13 +202,20 @@ namespace AppInstaller::SQLite //. Gets the (fixed but arbitrary) identifier for this connection. size_t GetID() const; - operator sqlite3* () const { return m_dbconn.get(); } + // Sets the busy timeout for the connection. + void SetBusyTimeout(std::chrono::milliseconds timeout); + + operator sqlite3* () const { return m_dbconn->Get(); } + + protected: + // Gets the shared connection. + std::shared_ptr GetSharedConnection() const; private: Connection(const std::string& target, OpenDisposition disposition, OpenFlags flags); size_t m_id = 0; - wil::unique_any m_dbconn; + std::shared_ptr m_dbconn; }; // A SQL statement. @@ -234,10 +263,10 @@ namespace AppInstaller::SQLite // Evaluate the statement; either retrieving the next row or executing some action. // Returns true if there is a row of data, or false if there is none. // This return value is the equivalent of 'GetState() == State::HasRow' after calling Step. - bool Step(bool failFastOnError = false); + bool Step(bool closeConnectionOnError = false); // Equivalent to Step, but does not ever expect a result, throwing if one is retrieved. - void Execute(bool failFastOnError = false); + void Execute(bool closeConnectionOnError = false); // Gets a boolean value that indicates whether the specified column value is null in the current row. // The index is 0 based. @@ -282,6 +311,7 @@ namespace AppInstaller::SQLite return std::make_tuple(details::ParameterSpecifics::GetColumn(m_stmt.get(), I)...); } + std::shared_ptr m_dbconn; size_t m_connectionId = 0; size_t m_id = 0; wil::unique_any m_stmt; @@ -303,7 +333,7 @@ namespace AppInstaller::SQLite ~Savepoint(); // Rolls back the Savepoint. - void Rollback(); + void Rollback(bool throwOnError = true); // Commits the Savepoint. void Commit(); diff --git a/src/AppInstallerSharedLib/SQLiteWrapper.cpp b/src/AppInstallerSharedLib/SQLiteWrapper.cpp index 5ddc36137f..8af16ca09b 100644 --- a/src/AppInstallerSharedLib/SQLiteWrapper.cpp +++ b/src/AppInstallerSharedLib/SQLiteWrapper.cpp @@ -7,6 +7,7 @@ #include +using namespace std::chrono_literals; using namespace std::string_view_literals; // Enable this to have all Statement constructions output the associated query plan. @@ -16,11 +17,14 @@ using namespace std::string_view_literals; #include #endif +// Connection is used twice +#define SQLITE_ERROR_MSG(_error_,_connection_) (_connection_ ? sqlite3_errmsg(_connection_) : sqlite3_errstr(_error_)) + #define THROW_SQLITE(_error_,_connection_) \ do { \ int _ts_sqliteReturnValue = (_error_); \ sqlite3* _ts_sqliteConnection = (_connection_); \ - THROW_EXCEPTION_MSG(SQLiteException(_ts_sqliteReturnValue), "%hs", _ts_sqliteConnection ? sqlite3_errmsg(_ts_sqliteConnection) : sqlite3_errstr(_ts_sqliteReturnValue)); \ + THROW_EXCEPTION_MSG(SQLiteException(_ts_sqliteReturnValue), "%hs", SQLITE_ERROR_MSG(_ts_sqliteReturnValue, _ts_sqliteConnection)); \ } while (0,0) #define THROW_IF_SQLITE_FAILED(_statement_,_connection_) \ @@ -144,22 +148,40 @@ namespace AppInstaller::SQLite return {}; } } + + void SharedConnection::Disable() + { + m_active = false; + } + + sqlite3* SharedConnection::Get() const + { + THROW_HR_IF(APPINSTALLER_CLI_ERROR_SQLITE_CONNECTION_TERMINATED, !m_active.load()); + return m_dbconn.get(); + } + + sqlite3** SharedConnection::GetPtr() + { + return &m_dbconn; + } } Connection::Connection(const std::string& target, OpenDisposition disposition, OpenFlags flags) { + m_dbconn = std::make_shared(); m_id = GetNextConnectionId(); AICLI_LOG(SQL, Info, << "Opening SQLite connection #" << m_id << ": '" << target << "' [" << std::hex << static_cast(disposition) << ", " << std::hex << static_cast(flags) << "]"); // Always force connection serialization until we determine that there are situations where it is not needed int resultingFlags = static_cast(disposition) | static_cast(flags) | SQLITE_OPEN_FULLMUTEX; - THROW_IF_SQLITE_FAILED(sqlite3_open_v2(target.c_str(), &m_dbconn, resultingFlags, nullptr), nullptr); + THROW_IF_SQLITE_FAILED(sqlite3_open_v2(target.c_str(), m_dbconn->GetPtr(), resultingFlags, nullptr), nullptr); } Connection Connection::Create(const std::string& target, OpenDisposition disposition, OpenFlags flags) { Connection result{ target, disposition, flags }; - - THROW_IF_SQLITE_FAILED(sqlite3_extended_result_codes(result.m_dbconn.get(), 1), result.m_dbconn.get()); + + THROW_IF_SQLITE_FAILED(sqlite3_extended_result_codes(result.m_dbconn->Get(), 1), result.m_dbconn->Get()); + result.SetBusyTimeout(250ms); return result; } @@ -167,17 +189,17 @@ namespace AppInstaller::SQLite void Connection::EnableICU() { AICLI_LOG(SQL, Verbose, << "Enabling ICU"); - THROW_IF_SQLITE_FAILED(sqlite3IcuInit(m_dbconn.get()), m_dbconn.get()); + THROW_IF_SQLITE_FAILED(sqlite3IcuInit(m_dbconn->Get()), m_dbconn->Get()); } rowid_t Connection::GetLastInsertRowID() { - return sqlite3_last_insert_rowid(m_dbconn.get()); + return sqlite3_last_insert_rowid(m_dbconn->Get()); } int Connection::GetChanges() const { - return sqlite3_changes(m_dbconn.get()); + return sqlite3_changes(m_dbconn->Get()); } size_t Connection::GetID() const @@ -185,8 +207,19 @@ namespace AppInstaller::SQLite return m_id; } + void Connection::SetBusyTimeout(std::chrono::milliseconds timeout) + { + THROW_IF_SQLITE_FAILED(sqlite3_busy_timeout(m_dbconn->Get(), static_cast(timeout.count())), m_dbconn->Get()); + } + + std::shared_ptr Connection::GetSharedConnection() const + { + return m_dbconn; + } + Statement::Statement(const Connection& connection, std::string_view sql) { + m_dbconn = connection.GetSharedConnection(); m_connectionId = connection.GetID(); m_id = GetNextStatementId(); AICLI_LOG(SQL, Verbose, << "Preparing statement #" << m_connectionId << '-' << m_id << ": " << sql); @@ -253,7 +286,7 @@ namespace AppInstaller::SQLite return { connection, sql }; } - bool Statement::Step(bool failFastOnError) + bool Statement::Step(bool closeConnectionOnError) { AICLI_LOG(SQL, Verbose, << "Stepping statement #" << m_connectionId << '-' << m_id); int result = sqlite3_step(m_stmt.get()); @@ -273,20 +306,19 @@ namespace AppInstaller::SQLite else { m_state = State::Error; - if (failFastOnError) - { - FAIL_FAST_MSG("Critical SQL statement failed"); - } - else + + if (closeConnectionOnError) { - THROW_SQLITE(result, sqlite3_db_handle(m_stmt.get())); + m_dbconn->Disable(); } + + THROW_SQLITE(result, sqlite3_db_handle(m_stmt.get())); } } - void Statement::Execute(bool failFastOnError) + void Statement::Execute(bool closeConnectionOnError) { - THROW_HR_IF(E_UNEXPECTED, Step(failFastOnError)); + THROW_HR_IF(E_UNEXPECTED, Step(closeConnectionOnError)); } bool Statement::GetColumnIsNull(int column) @@ -323,20 +355,35 @@ namespace AppInstaller::SQLite Savepoint::~Savepoint() { - Rollback(); + // Prevent a termination by not throwing on errors here + Rollback(false); } - void Savepoint::Rollback() + void Savepoint::Rollback(bool throwOnError) { if (m_inProgress) { - AICLI_LOG(SQL, Verbose, << "Roll back savepoint: " << m_name); - m_rollbackTo.Step(true); - // 'ROLLBACK TO' *DOES NOT* remove the savepoint from the transaction stack. - // In order to remove it, we must RELEASE. Since we just invoked a ROLLBACK TO - // this should have the effect of 'committing' nothing. - m_release.Step(true); + // Only try rollback once m_inProgress = false; + + try + { + AICLI_LOG(SQL, Verbose, << "Roll back savepoint: " << m_name); + m_rollbackTo.Step(true); + // 'ROLLBACK TO' *DOES NOT* remove the savepoint from the transaction stack. + // In order to remove it, we must RELEASE. Since we just invoked a ROLLBACK TO + // this should have the effect of 'committing' nothing. + m_release.Step(true); + } + catch (...) + { + if (throwOnError) + { + throw; + } + + LOG_CAUGHT_EXCEPTION(); + } } } @@ -345,7 +392,7 @@ namespace AppInstaller::SQLite if (m_inProgress) { AICLI_LOG(SQL, Verbose, << "Commit savepoint: " << m_name); - m_release.Step(true); + m_release.Step(); m_inProgress = false; } }