Update the SQLite v2 implementation to use safe handles

src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/Result.cs

-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
-
-namespace Microsoft.CodeAnalysis.SQLite.v2.Interop
-{
-    // From https://sqlite.org/c3ref/c_abort.html
-    // Uncomment what you need.  Leave the rest commented out to make it clear
-    // what we are/aren't using.
-    internal enum Result
-    {
-        OK = 0,                /* Successful result */
-        // ERROR = 1,          /* SQL error or missing database */
-        // INTERNAL = 2,       /* Internal logic error in SQLite */
-        // PERM = 3,           /* Access permission denied */
-        // ABORT = 4,          /* Callback routine requested an abort */
-        BUSY = 5,              /* The database file is locked */
-        LOCKED = 6,            /* A table in the database is locked */
-        NOMEM = 7,             /* A malloc() failed */
-        // READONLY = 8,       /* Attempt to write a readonly database */
-        // INTERRUPT = 9,      /* Operation terminated by sqlite3_interrupt()*/
-        IOERR = 10,            /* Some kind of disk I/O error occurred */
-        // CORRUPT = 11,       /* The database disk image is malformed */
-        // NOTFOUND = 12,      /* Unknown opcode in sqlite3_file_control() */
-        FULL = 13,             /* Insertion failed because database is full */
-        // CANTOPEN = 14,      /* Unable to open the database file */
-        // PROTOCOL = 15,      /* Database lock protocol error */
-        // EMPTY = 16,         /* Database is empty */
-        // SCHEMA = 17,        /* The database schema changed */
-        // TOOBIG = 18,        /* String or BLOB exceeds size limit */
-        CONSTRAINT = 19,       /* Abort due to constraint violation */
-        // MISMATCH = 20,      /* Data type mismatch */
-        // MISUSE = 21,        /* Library used incorrectly */
-        // NOLFS = 22,         /* Uses OS features not supported on host */
-        // AUTH = 23,          /* Authorization denied */
-        // FORMAT = 24,        /* Auxiliary database format error */
-        // RANGE = 25,         /* 2nd parameter to sqlite3_bind out of range */
-        // NOTADB = 26,        /* File opened that is not a database file */
-        // NOTICE = 27,        /* Notifications from sqlite3_log() */
-        // WARNING = 28,       /* Warnings from sqlite3_log() */
-        ROW = 100,             /* sqlite3_step() has another row ready */
-        DONE = 101             /* sqlite3_step() has finished executing */
-    }
-}

src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlConnection.cs

@@ -5,10 +5,9 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
-using Microsoft.CodeAnalysis.ErrorReporting;
 using Microsoft.CodeAnalysis.Host;
+using Microsoft.CodeAnalysis.SQLite.Interop;
 using Roslyn.Utilities;
-using SQLitePCL;
 
 namespace Microsoft.CodeAnalysis.SQLite.v2.Interop
 {
@@ -30,7 +29,7 @@ internal class SqlConnection
         /// <summary>
         /// The raw handle to the underlying DB.
         /// </summary>
-        private readonly sqlite3 _handle;
+        private readonly SafeSqliteHandle _handle;
 
         /// <summary>
         /// For testing purposes to simulate failures during testing.
@@ -70,18 +69,17 @@ public static SqlConnection Create(IPersistentStorageFaultInjector faultInjector
                 OpenFlags.SQLITE_OPEN_NOMUTEX |
                 OpenFlags.SQLITE_OPEN_SHAREDCACHE |
                 OpenFlags.SQLITE_OPEN_URI;
-            var result = (Result)raw.sqlite3_open_v2(databasePath, out var handle, (int)flags, vfs: null);
+            var handle = NativeMethods.sqlite3_open_v2(databasePath, (int)flags, vfs: null, out var result);
 
             if (result != Result.OK)
             {
+                handle.Dispose();
                 throw new SqlException(result, $"Could not open database file: {databasePath} ({result})");
             }
 
-            Contract.ThrowIfNull(handle);
-
             try
             {
-                raw.sqlite3_busy_timeout(handle, (int)TimeSpan.FromMinutes(1).TotalMilliseconds);
+                NativeMethods.sqlite3_busy_timeout(handle, (int)TimeSpan.FromMinutes(1).TotalMilliseconds);
                 var connection = new SqlConnection(handle, faultInjector, queryToStatement);
 
                 // Attach (creating if necessary) a singleton in-memory write cache to this connection.
@@ -105,36 +103,22 @@ public static SqlConnection Create(IPersistentStorageFaultInjector faultInjector
             {
                 // If we failed to create connection, ensure that we still release the sqlite
                 // handle.
-                raw.sqlite3_close(handle);
+                handle.Dispose();
                 throw;
             }
         }
 
-        private SqlConnection(sqlite3 handle, IPersistentStorageFaultInjector faultInjector, Dictionary<string, SqlStatement> queryToStatement)
+        private SqlConnection(SafeSqliteHandle handle, IPersistentStorageFaultInjector faultInjector, Dictionary<string, SqlStatement> queryToStatement)
         {
-            // This constructor avoids allocations since failure (e.g. OutOfMemoryException) would
-            // leave the object partially-constructed, and the finalizer would run later triggering
-            // a crash.
             _handle = handle;
             _faultInjector = faultInjector;
             _queryToStatement = queryToStatement;
         }
 
-        ~SqlConnection()
-        {
-            if (!Environment.HasShutdownStarted)
-            {
-                var ex = new InvalidOperationException("SqlConnection was not properly closed");
-                _faultInjector?.OnFatalError(ex);
-                FatalError.Report(new InvalidOperationException("SqlConnection was not properly closed"));
-            }
-        }
-
         internal void Close_OnlyForUseBySqlPersistentStorage()
         {
-            GC.SuppressFinalize(this);
-
-            Contract.ThrowIfNull(_handle);
+            // Dispose of the underlying handle at the end of cleanup
+            using var _ = _handle;
 
             // release all the cached statements we have.
             //
@@ -147,9 +131,6 @@ internal void Close_OnlyForUseBySqlPersistentStorage()
             }
 
             _queryToStatement.Clear();
-
-            // Finally close our handle to the actual DB.
-            ThrowIfNotOk(raw.sqlite3_close(_handle));
         }
 
         public void ExecuteCommand(string command, bool throwOnError = true)
@@ -167,10 +148,19 @@ public ResettableSqlStatement GetResettableStatement(string query)
         {
             if (!_queryToStatement.TryGetValue(query, out var statement))
             {
-                var result = (Result)raw.sqlite3_prepare_v2(_handle, query, out var rawStatement);
-                ThrowIfNotOk(result);
-                statement = new SqlStatement(this, rawStatement);
-                _queryToStatement[query] = statement;
+                var handle = NativeMethods.sqlite3_prepare_v2(_handle, query, out var result);
+                try
+                {
+                    ThrowIfNotOk(result);
+
+                    statement = new SqlStatement(this, handle);
+                    _queryToStatement[query] = statement;
+                }
+                catch
+                {
+                    handle.Dispose();
+                    throw;
+                }
             }
 
             return new ResettableSqlStatement(statement);
@@ -242,7 +232,7 @@ private void Rollback(bool throwOnError)
             => ExecuteCommand("rollback transaction", throwOnError);
 
         public int LastInsertRowId()
-            => (int)raw.sqlite3_last_insert_rowid(_handle);
+            => (int)NativeMethods.sqlite3_last_insert_rowid(_handle);
 
         [PerformanceSensitive("https://github.com/dotnet/roslyn/issues/36114", AllowCaptures = false)]
         public Stream ReadBlob_MustRunInTransaction(Database database, string tableName, string columnName, long rowId)
@@ -262,27 +252,22 @@ public Stream ReadBlob_MustRunInTransaction(Database database, string tableName,
             }
 
             const int ReadOnlyFlags = 0;
-            var result = raw.sqlite3_blob_open(_handle, database.GetName(), tableName, columnName, rowId, ReadOnlyFlags, out var blob);
-            if (result == raw.SQLITE_ERROR)
+
+            using var blob = NativeMethods.sqlite3_blob_open(_handle, database.GetName(), tableName, columnName, rowId, ReadOnlyFlags, out var result);
+
+            if (result == Result.ERROR)
             {
                 // can happen when rowId points to a row that hasn't been written to yet.
                 return null;
             }
 
             ThrowIfNotOk(result);
-            try
-            {
-                return ReadBlob(blob);
-            }
-            finally
-            {
-                ThrowIfNotOk(raw.sqlite3_blob_close(blob));
-            }
+            return ReadBlob(blob);
         }
 
-        private Stream ReadBlob(sqlite3_blob blob)
+        private Stream ReadBlob(SafeSqliteBlobHandle blob)
         {
-            var length = raw.sqlite3_blob_bytes(blob);
+            var length = NativeMethods.sqlite3_blob_bytes(blob);
 
             // If it's a small blob, just read it into one of our pooled arrays, and then
             // create a PooledStream over it. 
@@ -294,17 +279,17 @@ private Stream ReadBlob(sqlite3_blob blob)
             {
                 // Otherwise, it's a large stream.  Just take the hit of allocating.
                 var bytes = new byte[length];
-                ThrowIfNotOk(raw.sqlite3_blob_read(blob, bytes, length, offset: 0));
+                ThrowIfNotOk(NativeMethods.sqlite3_blob_read(blob, bytes, length, offset: 0));
                 return new MemoryStream(bytes);
             }
         }
 
-        private Stream ReadBlobIntoPooledStream(sqlite3_blob blob, int length)
+        private Stream ReadBlobIntoPooledStream(SafeSqliteBlobHandle blob, int length)
         {
             var bytes = SQLitePersistentStorage.GetPooledBytes();
             try
             {
-                ThrowIfNotOk(raw.sqlite3_blob_read(blob, bytes, length, offset: 0));
+                ThrowIfNotOk(NativeMethods.sqlite3_blob_read(blob, bytes, length, offset: 0));
 
                 // Copy those bytes into a pooled stream
                 return SerializableBytes.CreateReadableStream(bytes, length);
@@ -322,7 +307,7 @@ public void ThrowIfNotOk(int result)
         public void ThrowIfNotOk(Result result)
             => ThrowIfNotOk(_handle, result);
 
-        public static void ThrowIfNotOk(sqlite3 handle, Result result)
+        public static void ThrowIfNotOk(SafeSqliteHandle handle, Result result)
         {
             if (result != Result.OK)
             {
@@ -333,9 +318,9 @@ public static void ThrowIfNotOk(sqlite3 handle, Result result)
         public void Throw(Result result)
             => Throw(_handle, result);
 
-        public static void Throw(sqlite3 handle, Result result)
+        public static void Throw(SafeSqliteHandle handle, Result result)
             => throw new SqlException(result,
-                raw.sqlite3_errmsg(handle) + "\r\n" +
-                raw.sqlite3_errstr(raw.sqlite3_extended_errcode(handle)));
+                NativeMethods.sqlite3_errmsg(handle) + "\r\n" +
+                NativeMethods.sqlite3_errstr(NativeMethods.sqlite3_extended_errcode(handle)));
     }
 }

src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlException.cs

@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
+using Microsoft.CodeAnalysis.SQLite.Interop;
 
 namespace Microsoft.CodeAnalysis.SQLite.v2.Interop
 {

src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlStatement.cs

@@ -4,8 +4,8 @@
 
 using System;
 using System.Runtime.InteropServices;
+using Microsoft.CodeAnalysis.SQLite.Interop;
 using Roslyn.Utilities;
-using SQLitePCL;
 
 namespace Microsoft.CodeAnalysis.SQLite.v2.Interop
 {
@@ -30,23 +30,23 @@ namespace Microsoft.CodeAnalysis.SQLite.v2.Interop
     internal struct SqlStatement
     {
         private readonly SqlConnection _connection;
-        private readonly sqlite3_stmt _rawStatement;
+        private readonly SafeSqliteStatementHandle _rawStatement;
 
-        public SqlStatement(SqlConnection connection, sqlite3_stmt statement)
+        public SqlStatement(SqlConnection connection, SafeSqliteStatementHandle statement)
         {
             _connection = connection;
             _rawStatement = statement;
         }
 
         internal void Close_OnlyForUseBySqlConnection()
-            => _connection.ThrowIfNotOk(raw.sqlite3_finalize(_rawStatement));
+            => _rawStatement.Dispose();
 
         public void Reset()
-            => _connection.ThrowIfNotOk(raw.sqlite3_reset(_rawStatement));
+            => _connection.ThrowIfNotOk(NativeMethods.sqlite3_reset(_rawStatement));
 
         public Result Step(bool throwOnError = true)
         {
-            var stepResult = (Result)raw.sqlite3_step(_rawStatement);
+            var stepResult = NativeMethods.sqlite3_step(_rawStatement);
 
             // Anything other than DONE or ROW is an error when stepping.
             // throw if the caller wants that, or just return the value
@@ -64,30 +64,30 @@ public Result Step(bool throwOnError = true)
         }
 
         internal void BindStringParameter(int parameterIndex, string value)
-            => _connection.ThrowIfNotOk(raw.sqlite3_bind_text(_rawStatement, parameterIndex, value));
+            => _connection.ThrowIfNotOk(NativeMethods.sqlite3_bind_text(_rawStatement, parameterIndex, value));
 
         internal void BindInt64Parameter(int parameterIndex, long value)
-            => _connection.ThrowIfNotOk(raw.sqlite3_bind_int64(_rawStatement, parameterIndex, value));
+            => _connection.ThrowIfNotOk(NativeMethods.sqlite3_bind_int64(_rawStatement, parameterIndex, value));
 
         // SQLite PCL does not expose sqlite3_bind_blob function that takes a length.  So we explicitly
         // DLL import it here.  See https://github.com/ericsink/SQLitePCL.raw/issues/135
 
         internal void BindBlobParameter(int parameterIndex, byte[] value, int length)
-            => _connection.ThrowIfNotOk(sqlite3_bind_blob(_rawStatement.ptr, parameterIndex, value, length, new IntPtr(-1)));
+            => _connection.ThrowIfNotOk(sqlite3_bind_blob(_rawStatement, parameterIndex, value, length, new IntPtr(-1)));
 
         [DllImport("e_sqlite3.dll", ExactSpelling = true, CallingConvention = CallingConvention.Cdecl)]
-        public static extern int sqlite3_bind_blob(IntPtr stmt, int index, byte[] val, int nSize, IntPtr nTransient);
+        public static extern int sqlite3_bind_blob(SafeSqliteStatementHandle stmt, int index, byte[] val, int nSize, IntPtr nTransient);
 
         internal byte[] GetBlobAt(int columnIndex)
-            => raw.sqlite3_column_blob(_rawStatement, columnIndex);
+            => NativeMethods.sqlite3_column_blob(_rawStatement, columnIndex);
 
         internal int GetInt32At(int columnIndex)
-            => raw.sqlite3_column_int(_rawStatement, columnIndex);
+            => NativeMethods.sqlite3_column_int(_rawStatement, columnIndex);
 
         internal long GetInt64At(int columnIndex)
-            => raw.sqlite3_column_int64(_rawStatement, columnIndex);
+            => NativeMethods.sqlite3_column_int64(_rawStatement, columnIndex);
 
         internal string GetStringAt(int columnIndex)
-            => raw.sqlite3_column_text(_rawStatement, columnIndex);
+            => NativeMethods.sqlite3_column_text(_rawStatement, columnIndex);
     }
 }

src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage.Accessor.cs

@@ -6,6 +6,7 @@
 using System.IO;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.CodeAnalysis.SQLite.Interop;
 using Microsoft.CodeAnalysis.SQLite.v2.Interop;
 using Microsoft.CodeAnalysis.Storage;
 using Roslyn.Utilities;

src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage_BulkPopulateIds.cs

@@ -6,6 +6,7 @@
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 using Microsoft.CodeAnalysis.PooledObjects;
+using Microsoft.CodeAnalysis.SQLite.Interop;
 using Microsoft.CodeAnalysis.SQLite.v2.Interop;
 using Microsoft.CodeAnalysis.Storage;
 

src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage_StringIds.cs

@@ -4,6 +4,7 @@
 
 using System;
 using System.Collections.Concurrent;
+using Microsoft.CodeAnalysis.SQLite.Interop;
 using Microsoft.CodeAnalysis.SQLite.v2.Interop;
 using Microsoft.CodeAnalysis.Storage;
 using Roslyn.Utilities;