Replace Valgrind check with sanitizers (#188)

Author: simolus3

.github/workflows/tests.yml

@@ -222,30 +222,30 @@ jobs:
         working-directory: dart
         run: dart test
 
-  valgrind:
-    name: Testing with Valgrind on ${{ matrix.os }}
-    runs-on: ${{ matrix.os }}
-    if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository)
-    strategy:
-      matrix:
-        include:
-          - os: ubuntu-latest
+  test_with_sanitizers:
+    runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v6
         with:
           persist-credentials: false
-      - name: Install Rust
+      - uses: dart-lang/setup-dart@v1
+      - name: Install Rust Nightly
         run: rustup install
+      - name: Install LLVM toolchain
+        run: |
+          sudo apt update
+          sudo apt install -y llvm clang lld
 
-      - name: Install valgrind
-        run: sudo apt update && sudo apt install -y valgrind
+      - name: Build with sanitizers
+        run: tool/build_linux_sanitized.sh
 
-      - name: Install Cargo Valgrind
-        # TODO: Use released version. Currently we rely on the git version while we wait for this
-        # to be released: https://github.com/jfrimmel/cargo-valgrind/commit/408c0b4fb56e84eddc2bb09c88a11ba3adc0c188
-        run: cargo install --git https://github.com/jfrimmel/cargo-valgrind cargo-valgrind
-        #run: cargo install cargo-valgrind
+      - run: dart pub get
+        working-directory: dart
 
-      - name: Test Core
-        run: |
-          cargo valgrind test -p powersync_core
+      - name: Test with MemorySanitizer
+        working-directory: dart
+        run: dart tool/run_tests.dart msan
+
+      - name: Test with AddressSanitizer
+        working-directory: dart
+        run: dart tool/run_tests.dart asan

.gitignore

@@ -10,3 +10,4 @@ target/
 *.tar.xz
 *.zip
 .build
+/sanitized/

crates/core/src/pre_close_vtab.rs

@@ -8,6 +8,7 @@ use powersync_sqlite_nostd as sqlite;
 use sqlite::{Connection, ResultCode};
 
 use crate::state::DatabaseState;
+use crate::update_hooks::uninstall_update_hooks;
 use crate::vtab_util::*;
 
 /// A virtual table hack to implement a "pre-close hook" for databases.
@@ -20,6 +21,7 @@ use crate::vtab_util::*;
 struct VirtualTable {
     base: sqlite::vtab,
     state: Rc<DatabaseState>,
+    db: *mut sqlite::sqlite3,
 }
 
 extern "C" fn connect(
@@ -42,6 +44,7 @@ extern "C" fn connect(
                 zErrMsg: core::ptr::null_mut(),
             },
             state: DatabaseState::clone_from(aux),
+            db,
         }));
         *vtab = tab.cast::<sqlite::vtab>();
         let _ = sqlite::vtab_config(db, 0);
@@ -57,6 +60,7 @@ extern "C" fn disconnect(vtab: *mut sqlite::vtab) -> c_int {
     // So we can use this as a "pre-close" hook and ensure we clear prepared statements the core
     // extension might hold.
     vtab.state.release_resources();
+    uninstall_update_hooks(vtab.db, &vtab.state);
 
     ResultCode::OK as c_int
 }

crates/core/src/state.rs

@@ -25,6 +25,8 @@ use crate::{
 #[derive(Default)]
 pub struct DatabaseState {
     pub is_in_sync_local: Cell<bool>,
+    /// Whether the core extension has installed update, commit and rollback hooks.
+    pub core_extension_has_update_hooks: Cell<bool>,
     schema: RefCell<Option<Schema>>,
     pending_updates: RefCell<BTreeSet<String>>,
     commited_updates: RefCell<BTreeSet<String>>,

crates/core/src/update_hooks.rs

@@ -1,5 +1,4 @@
 use core::{
-    cell::Cell,
     ffi::{CStr, c_char, c_int, c_void},
     ptr::null_mut,
 };
@@ -19,13 +18,9 @@ use crate::{constants::SUBTYPE_JSON, error::PowerSyncError, state::DatabaseState
 ///      and comitted since the last `powersync_update_hooks` call.
 ///
 /// The update hooks don't have to be uninstalled manually, that happens when the connection is
-/// closed and the function is unregistered.
+/// closed (`powersync_init()` installs a vtab calling `uninstall_update_hooks()`).
 pub fn register(db: *mut sqlite::sqlite3, state: Rc<DatabaseState>) -> Result<(), ResultCode> {
-    let state = Box::new(HookState {
-        has_registered_hooks: Cell::new(false),
-        db,
-        state,
-    });
+    let state = Box::new(HookState { db, state });
 
     db.create_function_v2(
         "powersync_update_hooks",
@@ -41,31 +36,13 @@ pub fn register(db: *mut sqlite::sqlite3, state: Rc<DatabaseState>) -> Result<()
 }
 
 struct HookState {
-    has_registered_hooks: Cell<bool>,
     db: *mut sqlite::sqlite3,
     state: Rc<DatabaseState>,
 }
 
 extern "C" fn destroy_function(ctx: *mut c_void) {
     let state = unsafe { Box::from_raw(ctx as *mut HookState) };
-
-    if state.has_registered_hooks.get() {
-        check_previous(
-            "update",
-            &state.state,
-            state.db.update_hook(None, null_mut()),
-        );
-        check_previous(
-            "commit",
-            &state.state,
-            state.db.commit_hook(None, null_mut()),
-        );
-        check_previous(
-            "rollback",
-            &state.state,
-            state.db.rollback_hook(None, null_mut()),
-        );
-    }
+    uninstall_update_hooks(state.db, &state.state);
 }
 
 extern "C" fn powersync_update_hooks(
@@ -107,7 +84,7 @@ extern "C" fn powersync_update_hooks(
                     Rc::into_raw(db_state.clone()) as *mut c_void,
                 ),
             );
-            state.has_registered_hooks.set(true);
+            db_state.core_extension_has_update_hooks.set(true);
         }
         "get" => {
             let state = unsafe { user_data.as_ref().unwrap_unchecked() };
@@ -155,6 +132,14 @@ unsafe extern "C" fn rollback_hook_impl(ctx: *mut c_void) {
     state.track_rollback();
 }
 
+pub fn uninstall_update_hooks(db: *mut sqlite::sqlite3, state: &Rc<DatabaseState>) {
+    if state.core_extension_has_update_hooks.take() {
+        check_previous("update", state, db.update_hook(None, null_mut()));
+        check_previous("commit", state, db.commit_hook(None, null_mut()));
+        check_previous("rollback", state, db.rollback_hook(None, null_mut()));
+    }
+}
+
 fn check_previous(desc: &'static str, expected: &Rc<DatabaseState>, previous: *const c_void) {
     let expected = Rc::as_ptr(expected);
 

dart/pubspec.lock

@@ -237,10 +237,10 @@ packages:
     dependency: transitive
     description:
       name: native_toolchain_c
-      sha256: "89e83885ba09da5fdf2cdacc8002a712ca238c28b7f717910b34bcd27b0d03ac"
+      sha256: "8aaead321425bd3f03bd5894aa27c8ea6993eab95531da7e59f5d39c6e5708ec"
       url: "https://pub.dev"
     source: hosted
-    version: "0.17.4"
+    version: "0.18.0"
   node_preamble:
     dependency: transitive
     description:
@@ -364,11 +364,12 @@ packages:
   sqlite3:
     dependency: "direct main"
     description:
-      name: sqlite3
-      sha256: c6cfe9b1cc159c9eb8ba174b533a60b5126f9db8c6e34efb127d2bc04bc45034
-      url: "https://pub.dev"
-    source: hosted
-    version: "3.1.4"
+      path: sqlite3
+      ref: main
+      resolved-ref: "97268a5b32cf6313dd670e6fa57ae88d38a6564b"
+      url: "https://github.com/simolus3/sqlite3.dart.git"
+    source: git
+    version: "3.3.2"
   sqlite3_test:
     dependency: "direct dev"
     description:
@@ -514,4 +515,4 @@ packages:
     source: hosted
     version: "3.1.3"
 sdks:
-  dart: ">=3.9.999 <4.0.0"
+  dart: ">=3.10.0 <4.0.0"

dart/pubspec.yaml

@@ -19,6 +19,15 @@ dev_dependencies:
   meta: ^1.16.0
   path: ^1.9.1
 
+dependency_overrides:
+  # TODO: Remove after https://github.com/simolus3/sqlite3.dart/commit/97268a5b32cf6313dd670e6fa57ae88d38a6564b
+  # gets released.
+  sqlite3:
+    git:
+      url: https://github.com/simolus3/sqlite3.dart.git
+      path: sqlite3
+      ref: main
+
 # See https://pub.dev/documentation/sqlite3/latest/topics/hook-topic.html
 hooks:
   user_defines:

dart/test/sync_test.dart

@@ -978,7 +978,10 @@ void _syncTests<T>({
       expect(db.select('SELECT * FROM ps_buckets'), isEmpty);
     });
 
-    group('recoverable', () {
+    group('recoverable',
+        skip: testingWithSanitizers != null
+            ? 'Unsupported in memory VFS'
+            : null, () {
       late CommonDatabase secondary;
       final checkpoint = {
         'checkpoint': {

dart/test/utils/native_test_utils.dart

@@ -1,5 +1,6 @@
 import 'dart:ffi';
 import 'dart:io';
+import 'dart:math';
 
 import 'package:clock/clock.dart';
 import 'package:fake_async/fake_async.dart';
@@ -14,12 +15,26 @@ const defaultSqlitePath = 'libsqlite3.so.0';
 const cargoDebugPath = '../target/debug';
 var didLoadExtension = false;
 
+String? testingWithSanitizers = null;
+
 CommonDatabase openTestDatabase(
     {VirtualFileSystem? vfs, String fileName = ':memory:'}) {
   if (!didLoadExtension) {
     loadExtension();
   }
 
+  if (testingWithSanitizers != null && vfs == null) {
+    // When testing with sanitizers, always use a Dart VFS since we use an
+    // uninstrumented libc that wouldn't report buffers for read files as
+    // initialized.
+    final inMemory =
+        InMemoryFileSystem(name: 'in-memory-${Random().nextInt(1 << 32)}');
+    sqlite3.registerVirtualFileSystem(inMemory);
+    addTearDown(() => sqlite3.unregisterVirtualFileSystem(inMemory));
+
+    vfs = inMemory;
+  }
+
   final db = sqlite3.open(fileName, vfs: vfs?.name);
   addTearDown(db.close);
   return db;
@@ -64,6 +79,10 @@ String resolvePowerSyncLibrary() {
 }
 
 String _getLibraryForPlatform({String? path = cargoDebugPath}) {
+  if (testingWithSanitizers case final sanitizer?) {
+    return '../sanitized/core_extension/libpowersync_$sanitizer.linux.so';
+  }
+
   return switch (Abi.current()) {
     Abi.androidArm ||
     Abi.androidArm64 ||

dart/tool/all_tests.dart

@@ -0,0 +1,30 @@
+import 'package:test/test.dart';
+
+import '../test/crud_test.dart' as crud_test;
+import '../test/error_test.dart' as error_test;
+import '../test/js_key_encoding_test.dart' as json_key_encoding_test;
+import '../test/migration_test.dart' as migration_test;
+import '../test/schema_test.dart' as schema_test;
+// Skipping sync_local_performance_test because it's slow. It's functionality is
+// covered by other tests.
+import '../test/sync_stream_test.dart' as sync_stream_test;
+import '../test/sync_test.dart' as sync_test;
+import '../test/update_hooks_test.dart' as update_hooks_test;
+
+import '../test/utils/native_test_utils.dart';
+
+/// Runs all native tests.
+///
+/// We aot-compile this file to run tests with different sanitizers.
+void main(List<String> args) {
+  testingWithSanitizers = args.single;
+
+  group('crud_test.dart', crud_test.main);
+  group('error_test.dart', error_test.main);
+  group('js_key_encoding_test.dart', json_key_encoding_test.main);
+  group('migration_test.dart', migration_test.main);
+  group('schema_test.dart', schema_test.main);
+  group('sync_stream_test.dart', sync_stream_test.main);
+  group('sync_test.dart', sync_test.main);
+  group('update_hooks_test.dart', update_hooks_test.main);
+}