feat(store): open kernel databases safely - #113
Conversation
|
Warning Review limit reachedNext included review available in 16 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 108 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
Comment |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e208eb9db
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let mut conn = open_writer(&db_path).map_err(|_| KernelError::Inconclusive)?; | ||
| apply_preclassification_profile(&conn).map_err(|_| KernelError::Inconclusive)?; |
There was a problem hiding this comment.
When a header appears to be a kernel database but the identity is ultimately inconclusive, opening it read-write allows SQLite to recover a hot journal or checkpoint an uncheckpointed WAL before marker validation fails. Closing this connection can rewrite core.sqlite and delete its -wal/-shm files, so the KernelError::Inconclusive path does not leave the family untouched and may destroy the original forensic state; classification needs a non-mutating inspection path until the identity is conclusively accepted or quarantined.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cd5c3a. classify_existing_family now opens the candidate with SQLITE_OPEN_READ_ONLY | SQLITE_OPEN_NO_MUTEX and sets query_only=ON before any marker validation runs, so SQLite cannot replay a hot journal or checkpoint a WAL while the identity is still unknown. The Foreign and Inconclusive paths now leave the family byte-identical, and foreign_family_is_refused_before_sqlite_can_touch_it asserts the main file and its -wal are unchanged after the refusal.
| fn bootstrap(path: &Path) -> Result<Connection, KernelError> { | ||
| let mut conn = open_writer(path).map_err(|_| KernelError::Io)?; | ||
| apply_preclassification_profile(&conn).map_err(|_| KernelError::Io)?; | ||
| let incarnation: String = conn | ||
| .query_row("SELECT lower(hex(randomblob(16)))", [], |row| row.get(0)) | ||
| .map_err(|_| KernelError::Io)?; | ||
| apply_kernel_schema(&mut conn, &incarnation, current_time_ms()).map_err(|_| KernelError::Io)?; |
There was a problem hiding this comment.
If the process dies during the first bootstrap after SQLite creates core.sqlite-journal but before the schema transaction commits, the main file can remain zero-length with that sidecar present. On every restart, inspect_header classifies exactly that state as inconclusive and refuses to open it for rollback or retry, permanently wedging a database that never contained committed data; publish bootstrap intent before opening or safely recognize and recover this specific pristine transaction residue.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cd5c3a. inspect_header routes an absent or zero-length main file to classify_empty_family, which treats only a surviving -wal or -shm as inconclusive. A rollback -journal beside a zero-length main is pristine transaction residue, so that state now classifies as Pristine and bootstrap retries instead of wedging permanently.
| let durable_epoch: i64 = writer | ||
| .query_row( | ||
| "SELECT writer_epoch FROM writer_fence WHERE id=0", | ||
| [], | ||
| |row| row.get(0), | ||
| ) | ||
| .map_err(|_| KernelError::FenceLost)?; |
There was a problem hiding this comment.
When a stale store overlaps a lease successor, the successor can stamp its new epoch after this autocommit SELECT but before operation begins its write transaction, allowing the stale operation to commit despite losing the fence. The reverse ordering can also let the successor wait for and stamp immediately after an already-authorized stale transaction; the epoch check and mutation therefore need to run in the same BEGIN IMMEDIATE transaction, as the comment itself requires.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cd5c3a. with_writer opens BEGIN IMMEDIATE first and reads writer_fence inside that transaction, so the write lock is already held when the epoch is compared. A lease successor cannot stamp a new epoch between the check and the mutation. stale_writer_fence_blocks_the_operation and failed_writer_operation_leaves_no_partial_write cover both directions.
| fn suffix_path(path: &Path, suffix: &str) -> PathBuf { | ||
| PathBuf::from(format!("{}{suffix}", path.display())) |
There was a problem hiding this comment.
On Unix, when the supplied root contains non-UTF-8 bytes, path.display() replaces those bytes with Unicode replacement characters before constructing the derived path. The resulting -wal, -shm, and -journal names refer to a different or nonexistent directory, so the actual SQLite sidecars are neither hardened nor moved during quarantine; the reset-marker helper uses the same conversion and can make quarantine fail outright. Append suffixes to the path's OsString instead of round-tripping through display text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cd5c3a. Path derivation goes through suffix_path, which pushes the suffix onto the path's OsString instead of round-tripping through Path::display. family_sidecars and reset_marker_path both use it, so non-UTF-8 roots now name the real sidecars.
| let mut file = options.open(&marker_path).map_err(|_| KernelError::Io)?; | ||
| if file | ||
| .write_all(&bytes) | ||
| .and_then(|()| file.sync_all()) | ||
| .is_err() |
There was a problem hiding this comment.
If the process or host dies after create_new exposes the reset-marker filename but before write_all and sync_all complete, startup finds a truncated or empty marker and resume_quarantine returns Inconclusive forever, even though move_family had not yet begun and the original database is intact. Write and sync the marker under a temporary name and atomically rename it into place so an interrupted publication cannot create an unrecoverable reset state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cd5c3a. publish_reset_marker writes and fsyncs the marker under a staging suffix, then renames it into place and syncs the parent directory. A crash mid-publication leaves either no marker or a complete one, never a truncated marker that recovery can only refuse.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| resume_quarantine(&db_path)?; | ||
| } | ||
|
|
||
| let expected = expected_identity()?; |
There was a problem hiding this comment.
SUGGESTION: Cache expected_identity statically instead of rebuilding schema on every open
expected_identity() opens an in-memory SQLite connection and executes all 31 schema DDL statements to calculate the expected digest and object inventory on every KernelStore::open call. Because the kernel schema is static and determined at compile time, computing this once lazily (e.g. using std::sync::LazyLock or OnceLock) eliminates unnecessary initialization overhead and memory allocation.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 2cd5c3a. The expected digest and object inventory now come from a static EXPECTED_IDENTITY: LazyLock<Option<ExpectedIdentity>>, so the schema is built once per process rather than on every KernelStore::open. The claim was accurate against the reviewed head, where expected_identity returned a freshly built ExpectedIdentity.
| &self, | ||
| operation: impl FnOnce(&Connection) -> rusqlite::Result<T>, | ||
| ) -> Result<T, KernelError> { | ||
| let index = self.next_reader.fetch_add(1, Ordering::Relaxed) % self.readers.len(); |
There was a problem hiding this comment.
WARNING: Round-robin reader dispatch causes head-of-line blocking and poison lock propagation
with_reader selects connections via fetch_add(1, Ordering::Relaxed) % self.readers.len() and locks the selected mutex. If a query is executing on that connection, incoming operations assigned to that slot block even if another reader connection in the pool is idle. Furthermore, if an operation panics while holding a reader mutex, that Mutex<Connection> becomes poisoned and mapping the lock error to KernelError::Io causes half of all subsequent read operations across the process to fail permanently. Consider dynamic reader selection (e.g. try_lock) and resilient lock handling.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
The poison half is fixed in 2cd5c3a: both with_reader and with_writer now recover the guard with unwrap_or_else(PoisonError::into_inner), so a panicking closure no longer takes its pool slot out of service for the life of the process. Transaction::drop rolls the aborted read back, so the recovered connection is not left mid-transaction.
Declining the dispatch change. Reads are short single-transaction closures, and try_lock scanning would trade a bounded wait for an unbounded scan with no fairness guarantee, which is the harder property to reason about under load. Worth revisiting with a measurement that shows queueing on a busy slot while another is idle.
| let metadata = match fs::symlink_metadata(path) { | ||
| Ok(metadata) => metadata, | ||
| Err(error) if error.kind() == std::io::ErrorKind::NotFound => { | ||
| if family_sidecars(path).iter().any(|sidecar| sidecar.exists()) { |
There was a problem hiding this comment.
WARNING: inspect_header sidecar check uses exists() which follows symlinks and misses dangling symlinks
family_sidecars(path).iter().any(|sidecar| sidecar.exists()) uses Path::exists(), which calls fs::metadata() and follows symlinks. If a family sidecar (-wal, -shm, or -journal) is a broken or dangling symlink, exists() returns false, causing inspect_header to classify the state as HeaderState::Pristine rather than KernelError::Inconclusive. Use fs::symlink_metadata(sidecar).is_ok() so any existing sidecar filesystem entry is detected without dereferencing symlinks.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 2cd5c3a. Sidecar probing goes through entry_exists, which uses fs::symlink_metadata and treats only NotFound as absent. A dangling symlink is now detected, and a transient I/O error surfaces as KernelError::Io instead of collapsing to false and selecting a destructive branch.
| } | ||
|
|
||
| fn activate_wal(conn: &Connection) -> Result<(), KernelError> { | ||
| conn.pragma_update(None, "journal_mode", "WAL") |
There was a problem hiding this comment.
WARNING: activate_wal does not verify the journal mode return value
conn.pragma_update(None, "journal_mode", "WAL") executes the pragma update but does not inspect the returned journal mode string. If switching to WAL mode fails or is rejected by SQLite (for instance, on certain network or memory-backed filesystems), SQLite silently remains in rollback journal mode. This can lead to unexpected SQLITE_BUSY locking conflicts when read-pool connections concurrently query the database. Query the pragma result to verify that the active journal mode is "wal".
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 2cd5c3a. activate_wal runs PRAGMA journal_mode=WAL through query_row and rejects any result that is not wal before the read pool is built, so a filesystem that silently refuses WAL fails the open instead of running in rollback-journal mode.
| } | ||
|
|
||
| fn prepare_private_dir(path: &Path) -> Result<(), KernelError> { | ||
| if !path.exists() { |
There was a problem hiding this comment.
WARNING: Non-atomic directory permission creation creates a TOCTOU permission window
prepare_private_dir creates directories using fs::create_dir(path) and subsequently applies 0o700 permissions via fs::set_permissions. On Unix systems, fs::create_dir uses the process umask, leaving the new directory world- or group-accessible before set_permissions executes. Use std::os::unix::fs::DirBuilderExt::mode(&mut fs::DirBuilder::new(), 0o700).create(path) on Unix targets so private store directories are created atomically with restrictive permissions.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 2cd5c3a. prepare_private_dir creates directories with fs::DirBuilder::new().mode(0o700) under #[cfg(unix)], so the mode is applied by the syscall rather than after it. The set_permissions call is retained only to repair a pre-existing directory whose mode is not 0o700.
| sync_directory(parent) | ||
| } | ||
|
|
||
| fn sync_directory(path: &Path) -> Result<(), KernelError> { |
There was a problem hiding this comment.
WARNING: sync_directory uses File::open which fails on non-Unix platforms
sync_directory opens directory paths with File::open(path) and calls sync_all(). On Windows, calling File::open on a directory path returns an access error (PermissionDenied / ERROR_ACCESS_DENIED) because opening directory handles requires backup semantics flags. Because sync_directory is called unconditionally during prepare_root, publish_reset_marker, and move_family, operations on Windows will fail with KernelError::Io. Gate directory fsync to #[cfg(unix)] or provide a platform-specific fallback.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Declining. This repository has no Windows target: the CI matrices are ubuntu-latest, macos-latest, and macos-15-intel, and ci.yml contains no Windows reference. Gating the directory fsync behind #[cfg(unix)] would compile on a platform nobody builds while silently dropping the barrier the crash-recovery contract depends on, so a rename could be lost after a power failure. If Windows becomes a target, this needs a real FILE_FLAG_BACKUP_SEMANTICS implementation rather than a no-op.
| "mc_kernel_format_marker", | ||
| r#"CREATE TABLE mc_kernel_format_marker(singleton INTEGER PRIMARY KEY CHECK(singleton=1),format_epoch INTEGER NOT NULL,database_incarnation_id TEXT NOT NULL,schema_digest TEXT NOT NULL,created_at INTEGER NOT NULL) STRICT;"#, | ||
| r#"CREATE TABLE mc_kernel_format_marker(singleton INTEGER PRIMARY KEY CHECK(singleton=1),format_epoch INTEGER NOT NULL,database_incarnation_id TEXT NOT NULL CHECK(length(database_incarnation_id)=32),schema_digest TEXT NOT NULL CHECK(length(schema_digest)=64),created_at INTEGER NOT NULL,marker_digest TEXT NOT NULL CHECK(length(marker_digest)=64)) STRICT; CREATE TRIGGER mc_kernel_format_marker_no_update BEFORE UPDATE ON mc_kernel_format_marker BEGIN SELECT RAISE(ABORT, 'mc_kernel_format_marker is immutable'); END; CREATE TRIGGER mc_kernel_format_marker_no_delete BEFORE DELETE ON mc_kernel_format_marker BEGIN SELECT RAISE(ABORT, 'mc_kernel_format_marker is immutable'); END;"#, | ||
| ), |
There was a problem hiding this comment.
WARNING: mc_kernel_format_marker table is vulnerable to replacement via INSERT OR REPLACE
mc_kernel_format_marker defines BEFORE UPDATE and BEFORE DELETE triggers to enforce immutability of the format marker row. However, in SQLite, INSERT OR REPLACE INTO / REPLACE INTO on conflict does not execute BEFORE DELETE triggers when recursive triggers are disabled (the default), allowing the singleton format marker to be overwritten. Adding a BEFORE INSERT trigger checking WHEN EXISTS (SELECT 1 FROM mc_kernel_format_marker WHERE singleton = NEW.singleton) ensures complete immutability against replacement inserts.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 2cd5c3a. mc_kernel_format_marker gained a mc_kernel_format_marker_no_replace BEFORE INSERT trigger that aborts when a row already exists, which closes the path around the BEFORE DELETE trigger when recursive_triggers is off.
replace_cannot_bypass_the_append_only_guards now supplies every column, including marker_digest, and loops over both INSERT OR REPLACE and bare REPLACE. Supplying the full column list matters: without it the statement fails on the NOT NULL constraint rather than on the guard, so the test would have passed without proving anything.
| .unwrap(); | ||
| conn.execute( | ||
| "UPDATE mc_kernel_format_marker SET marker_digest=?1", | ||
| ["g".repeat(64)], |
There was a problem hiding this comment.
SUGGESTION: Use valid hex characters to test marker checksum mismatch
malformed_marker_is_inconclusive_and_untouched sets marker_digest to ["g".repeat(64)]. Because 'g' is not a hexadecimal character, this triggers the is_lower_hex format check rather than testing cryptographic checksum mismatch in marker.marker_digest != expected_marker_digest. Using a valid 64-character lowercase hex string (such as "0".repeat(64)) will accurately exercise the digest verification branch.
| ["g".repeat(64)], | |
| ["0".repeat(64)], |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 2cd5c3a. malformed_marker_is_inconclusive_and_untouched loops over both digests, "g".repeat(64) and "0".repeat(64), so the is_lower_hex format check and the marker_digest != expected_marker_digest comparison are each exercised.
| conn.execute_batch("CREATE TABLE unexpected(value INTEGER) STRICT;") | ||
| .unwrap(); | ||
| drop(conn); | ||
| let db_path = core_path(dir.path()).canonicalize().unwrap(); |
There was a problem hiding this comment.
WARNING: Canonicalizing db_path causes quarantine resume failures on symlinked filesystem roots
In valid_interrupted_reset_marker_resumes_without_opening_old_family, db_path is canonicalized with .canonicalize().unwrap(), resolving symlinks. However, KernelStore::open uses absolute_path which does not resolve symlinks. On platforms where temporary directories reside on symlinked paths (such as macOS /var/folders or /tmp), the recorded marker.db_path does not match the opener's path, causing resume_quarantine to fail the marker.db_path != path check and return KernelError::Inconclusive. Remove .canonicalize() or normalize both paths consistently.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 2cd5c3a, from the implementation side. prepare_root now returns fs::canonicalize(root), so the marker's db_path and the opener's path are derived from the same canonical root and agree on a symlinked temporary directory. The test's canonicalize therefore matches rather than diverging, and the suite passes.
There was a problem hiding this comment.
Correcting my earlier reply on this thread: I said this was fixed, and it was not. macOS CI then failed on exactly this test.
valid_interrupted_reset_marker_resumes_without_opening_old_family ... FAILED
panicked at crates/mc-store/tests/kernel_open.rs:284:48
Fixed in b34b52e5, and the diagnosis is slightly different from the one in this comment. The break was not canonicalize by itself, it was the asymmetry: line 256 canonicalized db_path while line 257 built quarantine from the raw dir.path(). Two checks then disagreed:
resume_quarantinecomparesmarker.db_pathto the opened path.prepare_rootcanonicalizes, so both sides were/private/var/...and this passed.valid_quarantine_pathcomparesquarantine.parent()topath.parent(). The marker held/var/...while the opener held/private/var/..., so this failed and the marker was refused asInconclusive.
Removing canonicalize would also have worked, but it would leave the fixture depending on the root never being a symlink. Deriving every path from one canonical root is the version that holds either way, which is what landed.
Two things worth recording. First, this is why the earlier reply was wrong: I checked that prepare_root canonicalizes and concluded the two sides agreed, without checking that the fixture canonicalized both paths it writes into the marker. Second, Linux CI cannot catch this class of bug, because /tmp is a real directory and both spellings are identical. I reproduced it on Linux by pointing TMPDIR at a symlink, which fails at the same 284:48 before the fix and passes after, and I ran the whole mc-store suite that way to check nothing else is spelling-sensitive. Nothing else was.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
Previous Review Summaries (2 snapshots, latest commit 5aba72c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5aba72c)Status: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (8 files)
Fix these issues in Kilo Cloud Previous review (commit 0e208eb)Status: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (8 files)
Reviewed by gemini-3.7-flash · Input: 199K · Output: 12.6K · Cached: 1.6M |
0e208eb to
5aba72c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5aba72c054
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fn activate_wal(conn: &Connection) -> Result<(), KernelError> { | ||
| conn.pragma_update(None, "journal_mode", "WAL") | ||
| .map_err(|_| KernelError::Io)?; |
There was a problem hiding this comment.
When the VFS or filesystem cannot enter WAL mode, PRAGMA journal_mode=WAL can return the previous mode without producing an SQLite error. pragma_update discards that returned mode, so this path can report success and construct the read pool while the database remains in rollback-journal mode, violating the concurrency and recovery contract; query the resulting mode and require wal before proceeding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cd5c3a. activate_wal reads the mode back through query_row and returns an error unless it is wal, so the read pool is only constructed once WAL is actually active.
| let marker_path = reset_marker_path(path); | ||
| move_one(&marker_path, &marker.quarantine_dir)?; | ||
| sync_directory(&marker.quarantine_dir)?; |
There was a problem hiding this comment.
If the host loses power after the live marker is renamed here but before either directory is synced, the marker removal can persist without all preceding family-file renames persisting. The next startup then skips resume_quarantine and may encounter a restored main file or orphaned sidecars with no recovery marker, wedging the store or bootstrapping from an incomplete family; sync both directories while the marker is still live, then remove the marker and sync again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cd5c3a. move_family renames the family, syncs the quarantine directory and the family's parent, and only then moves the marker and syncs both directories again. The marker stays live until the renames it describes are durable, so a power loss in that window still leaves recovery a marker to resume from.
| fn absolute_path(path: &Path) -> Result<PathBuf, KernelError> { | ||
| if path.is_absolute() { | ||
| Ok(path.to_path_buf()) | ||
| } else { | ||
| std::env::current_dir() | ||
| .map(|current| current.join(path)) | ||
| .map_err(|_| KernelError::Io) |
There was a problem hiding this comment.
When the same root is reached through different absolute spellings—such as /data/staging/../kernel initially and /data/kernel after restart—this helper preserves the lexical spelling even though both names address the same directory. An interrupted quarantine therefore leaves a marker at the shared location whose stored db_path fails the equality check in resume_quarantine, making recovery permanently Inconclusive; normalize or canonicalize the created root before deriving and persisting database paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cd5c3a. prepare_root canonicalizes the created root and every derived database path comes from that result, so two spellings of the same directory produce one recorded db_path and resume_quarantine's equality check holds across restarts.
| { | ||
| return Err(KernelError::Inconclusive); | ||
| } | ||
| move_family(path, &marker) |
There was a problem hiding this comment.
If quarantine is interrupted after marker publication, a restore, external SQLite process, or other replacement can change the files at the live family names before the next opener. Because the marker records no dev/inode identities or originally present roles, this call unconditionally moves the current occupants before any header classification, so it can quarantine a newly restored current database or a foreign family and bootstrap over it using stale reset intent; record every family's identity and reject replacements or unexpected files during resume.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real gap, and not fixed here. resume_quarantine validates the marker's protocol, db_path, incarnation, digest, and quarantine path, but it records no dev/inode identities, so it moves whatever currently occupies the family names.
What limits the damage is that quarantine moves rather than deletes: a database restored into the family names is preserved under the quarantine directory, not destroyed, and the empty bootstrap that follows is visible rather than silent. So this is a wrong-outcome bug, not a data-loss bug.
Closing it properly means adding per-role identities to the persisted ResetMarker, covering them with the marker digest, and deciding what resume does when an identity does not match. That is a change to the on-disk marker protocol and deserves its own review rather than riding along here, so I am leaving it out of this PR.
| } | ||
|
|
||
| fn resume_quarantine(path: &Path) -> Result<(), KernelError> { | ||
| let bytes = fs::read(reset_marker_path(path)).map_err(|_| KernelError::Inconclusive)?; |
There was a problem hiding this comment.
When a pending reset marker is corrupted or replaced with a very large file, fs::read allocates for the entire file before the JSON shape or digest is validated. A malformed artifact can therefore exhaust the daemon's memory instead of returning the intended Inconclusive error; enforce a small protocol-size limit from metadata and read through a bounded buffer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cd5c3a. resume_quarantine stats the marker with fs::symlink_metadata and refuses anything that is not a regular file or exceeds RESET_MARKER_MAX_BYTES (64 KiB) before fs::read allocates, so an oversized artifact returns Inconclusive instead of consuming memory.
| ) -> Result<T, KernelError> { | ||
| let index = self.next_reader.fetch_add(1, Ordering::Relaxed) % self.readers.len(); | ||
| let reader = self.readers[index].lock().map_err(|_| KernelError::Io)?; | ||
| operation(&reader).map_err(|_| KernelError::Io) |
There was a problem hiding this comment.
When a query closure performs multiple SELECTs while the writer commits concurrently, this pooled connection remains in autocommit mode, so each statement may observe a different WAL snapshot. The returned value can consequently combine state from before and after one canonical commit rather than provide the required complete known_as_of view; begin a read transaction around the closure and return the connection to the pool only after that transaction is closed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cd5c3a. with_reader opens reader.transaction() around the closure and commits after it returns, so every SELECT in one operation observes a single WAL snapshot and a returned value cannot straddle a concurrent commit. owned_read_connections_are_query_only covers the pool's read-only contract.
Acquire the writer lease before database access, classify foreign files from the header, and quarantine only conclusive kernel mismatches. Exact opens verify the frozen schema before enabling WAL and stamping the writer fence.
The writer fence was read in autocommit, so a lease successor could stamp a new epoch between the check and the write. Reading it inside a BEGIN IMMEDIATE transaction makes the check and the mutation atomic. Classification opened the family read-write, which lets SQLite roll back a journal or checkpoint a WAL. That rewrites the main file and unlinks its sidecars before the identity is known, breaking the promise that Foreign and Inconclusive leave the family untouched. A read-only connection keeps that promise. Quarantine removed the reset marker before the renames it describes were durable. Since the marker's absence is what declares the reset complete, a crash in that window could lose the renames and leave nothing to recover from. Both directories are now durable while the marker is still live, and the marker is published under a staging name so an interrupted publication cannot leave a marker recovery can only refuse. Derived paths went through Path::display, which replaces non-UTF-8 bytes and named a different file, so the real sidecars were neither hardened nor moved. Suffixes are now appended to the path's OsStr, and the root is canonicalized so recovery compares one spelling of the directory. Existence checks went through Path::exists, which maps every error to false and let a transient I/O error select a destructive branch. Close remaining recovery hazards: validate journal mode, bound marker reads, give readers a snapshot, and avoid INSERT OR REPLACE, which bypasses the format-marker delete trigger.
The kernel now stamps the shared mc application id, so the foreign-family fixture stamped the very id it meant to reject and the header classified it as a kernel database. The fixture uses an id from outside the family and asserts it differs from the kernel's, which fails the test instead of quietly reducing it to a tautology if the constants ever converge again. A database carrying the shared id but a different schema has no header-level tell, so a separate case covers that path and pins the promise that the refusal leaves the family byte-identical. The pinned digest moves because the format marker gained its marker_digest column and its replacement guard.
5aba72c to
5df9599
Compare
Rebased onto
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5df9599aca
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let mut conn = Connection::open_with_flags( | ||
| path, | ||
| OpenFlags::SQLITE_OPEN_READ_ONLY | OpenFlags::SQLITE_OPEN_NO_MUTEX, | ||
| ) |
There was a problem hiding this comment.
When a kernel-header database has an uncheckpointed WAL but no -shm file and classification later returns Inconclusive (for example, because its marker is malformed), this read-only SQLite open reconstructs core.sqlite-shm before returning. Fresh evidence in this revision is the comment at lines 145–146 explicitly acknowledging that a read-only open recreates -shm. The family therefore is not left byte-identical as promised, so classification needs a genuinely non-mutating WAL inspection path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and the doc comment was overclaiming. Corrected in f398525a.
The behaviour is unchanged: a read-only open of a database with an uncheckpointed WAL and no -shm does recreate core.sqlite-shm. What that file holds is a shared-memory index over the -wal; it carries no durable content and SQLite rebuilds it on demand, so the forensic content of the family, the main database and its -wal, is intact. foreign_family_is_refused_before_sqlite_can_touch_it and a_sibling_mc_family_is_refused_and_left_untouched assert exactly those bytes.
Note the Foreign verdict never reaches SQLite at all: inspect_header decides it from the 100-byte header, so only the Inconclusive path can recreate -shm.
So the defect was the promise, not the behaviour. classify_existing_family now documents that it preserves the database and its -wal and that a missing -shm may be recreated, which makes the contract checkable instead of aspirational. A genuinely non-mutating path means parsing the WAL without SQLite, which is a much larger change and not one I want to smuggle into a rebase.
| .map_err(|_| KernelError::Inconclusive)? | ||
| .collect::<rusqlite::Result<Vec<_>>>() | ||
| .map_err(|_| KernelError::Inconclusive)?; |
There was a problem hiding this comment.
When a kernel-header database contains a lookalike mc_kernel_format_marker table without the expected singleton constraint, this collection materializes every row and every text field before checking that exactly one row exists. A corrupt or foreign database can consequently exhaust the daemon's memory instead of producing KernelError::Inconclusive; fetch at most two rows and bound field lengths before materializing them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 98b03f70. The marker query now carries LIMIT 2 plus length() predicates on database_incarnation_id, schema_digest, and marker_digest, so a lookalike table without the singleton constraint can no longer make the collection materialize arbitrary rows or arbitrarily wide text.
Bounding the widths in SQL rather than after materializing is behaviour-preserving on the outcome: a row rejected by a width predicate produces no row, and the empty result already maps to Inconclusive, which is what the is_lower_hex checks would have returned anyway. malformed_marker_is_inconclusive_and_untouched still reaches the hex and digest branches because both of its fixtures are 64 characters wide.
| fn apply_preclassification_profile(conn: &Connection) -> rusqlite::Result<()> { | ||
| conn.pragma_update(None, "foreign_keys", "ON")?; | ||
| conn.pragma_update(None, "trusted_schema", "OFF")?; | ||
| conn.pragma_update(None, "busy_timeout", BUSY_TIMEOUT_MS)?; | ||
| Ok(()) |
There was a problem hiding this comment.
Whenever a kernel mutation uses INSERT OR REPLACE or REPLACE against an append-only table such as commit_log, these connections retain SQLite's default recursive_triggers=OFF, so the implicit delete bypasses the schema's delete trigger and rewrites canonical history. The separate kernel connection profile explicitly enables this pragma for that reason, but KernelStore uses only this reduced profile; enable recursive triggers before exposing the writer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and it was real. Fixed in 98b03f70.
apply_preclassification_profile set foreign_keys, trusted_schema, and busy_timeout but left recursive_triggers at SQLite's default of off, and both the writer and the read pool are built from it. So a REPLACE through with_writer skipped the BEFORE DELETE triggers on commit_log, change_event, operation_receipts, consumer_abandonments, and decision_events, which is exactly the append-only history the schema declares immutable. verify_kernel_connection_contract already counts recursive_triggers != 1 as a violation, so the store's own connections did not satisfy the contract this crate defines for them.
Worth naming why the existing coverage missed it: replace_cannot_bypass_the_append_only_guards drives a connection from apply_kernel_connection_profile, which does set the pragma, so it could never observe what KernelStore configures. The new store_connections_run_delete_triggers_for_replace asserts the pragma through with_writer and then drives both INSERT OR REPLACE and bare REPLACE against commit_log. I checked it fails without the fix (left: 0, right: 1) rather than passing vacuously.
| fn sync_directory(path: &Path) -> Result<(), KernelError> { | ||
| File::open(path) | ||
| .and_then(|directory| directory.sync_all()) | ||
| .map_err(|_| KernelError::Io) |
There was a problem hiding this comment.
On Windows, where the product documents support, File::open cannot open a directory without directory-specific CreateFile flags, so every conclusive schema mismatch fails while publishing or completing its quarantine. Because publication may already have exposed the live reset marker, subsequent opens repeatedly enter resume and fail at the same sync, leaving the store unavailable; use a platform-specific directory handle or an explicitly supported durability primitive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You are right that I was wrong, and my earlier reply on the sibling thread was based on a bad premise. I claimed there was no Windows target because CI only builds ubuntu and macos. But README.md documents PowerShell installation, a %USERPROFILE%\.config config path, and an "XDG-equivalent on Windows" location for the durable store, and CONFIGURATION.md documents that POSIX modes have no effect on Windows. So Windows is a documented platform and CI simply does not cover it.
Still not fixing it here, for a different reason than I first gave. docs/mc-host-wire-protocol.md already scopes this explicitly: "Initial secure publication support is Unix-like systems. Windows support is deferred until atomic replacement, ACL validation, instance locking, link handling, and ownership-fenced cleanup have a reviewed contract." Directory fsync is a member of precisely that set of primitives, and the crash-recovery protocol here depends on it: the ordering in move_family and publish_reset_marker is only meaningful because the directory entry is durable at the barrier.
The two available shortcuts are both worse than the current failure. A #[cfg(unix)] no-op would compile on Windows and silently drop the barrier, so a rename could be lost on power failure with nothing reporting it, and that is a data-integrity regression traded for a compile. Returning Io on Windows is what the code does today, which at least fails loudly. A real fix is CreateFileW with FILE_FLAG_BACKUP_SEMANTICS behind a #[cfg(windows)] arm, which means a new platform dependency and its own durability review.
So this stays open as a genuine Windows-support gap rather than something I can close in this PR. Flagging it for the deferred-Windows contract that doc already anticipates.
The store built its writer and readers from the pre-classification profile, which set foreign_keys, trusted_schema, and busy_timeout but left recursive_triggers at SQLite's default of off. REPLACE resolves a conflict by deleting the conflicting row, and that delete skips the row's BEFORE DELETE trigger while the pragma is off, so a REPLACE through the writer could rewrite append-only history that the schema declares immutable. The same pragma is already required by verify_kernel_connection_contract, so the store's own connections did not satisfy the contract the crate defines for them. The existing guard test drives a connection from apply_kernel_connection_profile, which sets the pragma, so it could not observe this. The new test asserts the pragma through with_writer and then drives both REPLACE spellings against commit_log. The format marker is read from an untrusted database, where a lookalike table carries no singleton constraint, so the query now bounds both the row count and the identifier and digest widths instead of materializing whatever it finds.
The promise said the family is left byte-identical, but a read-only open recreates a missing -shm. The database and its -wal carry the durable content and are preserved; naming that keeps the contract checkable.
The fixture canonicalized db_path but left quarantine_dir as the raw temporary path. resume_quarantine compares marker.db_path to the opened root and valid_quarantine_path compares the two parents, so a root reached through a symlink presented two spellings and the marker was refused as Inconclusive. The store never resumed, and the open failed. Linux /tmp is a real directory, so both spellings matched and the suite passed; macOS reaches its temporary directory through /var -> /private/var, which is where it failed. Reproduced on Linux by pointing TMPDIR at a symlink.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b34b52e5cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fn publish_reset_marker(path: &Path, marker: &ResetMarker) -> Result<(), KernelError> { | ||
| let marker_path = reset_marker_path(path); | ||
| let staging = suffix_path(&marker_path, RESET_MARKER_STAGING_SUFFIX); | ||
| let bytes = serde_json::to_vec(marker).map_err(|_| KernelError::Io)?; |
There was a problem hiding this comment.
On Unix, a conclusive schema mismatch under a root containing non-UTF-8 bytes always fails here because ResetMarker contains PathBuf fields and Serde's JSON serialization rejects paths that are not valid UTF-8. Consequently, the database cannot be quarantined or reopened even though sidecar derivation now preserves the original OS bytes. Fresh evidence in this revision is that marker publication still passes those paths through serde_json::to_vec; encode path bytes losslessly or store validated relative byte components instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and it is worse than the comment states: the marker is UTF-8-only in two places, not one.
ResetMarker holds db_path and quarantine_dir as PathBuf, and serde's Serialize for PathBuf calls to_str() and fails with "path contains invalid UTF-8 characters" when it returns None, so serde_json::to_vec in publish_reset_marker errors out. Separately, reset_marker_digest builds its canonical string from marker.db_path.display(), and display() substitutes replacement characters, so even a marker that serialized would be digested over a lossy spelling and would not round-trip.
So the earlier OsStr change fixed sidecar derivation and left the marker protocol on the old footing. You are right that these do not compose.
Not fixing it here. Encoding path bytes losslessly changes the on-disk marker format and its digest input, which is the same protocol revision the "bind resumed moves to recorded family identities" thread needs. Those two want to land together in one reviewed change to RESET_MARKER_PROTOCOL rather than as two separate half-migrations of the same artifact, and doing it inside a rebase is how you get a format nobody reviewed.
Worth stating the current blast radius plainly: on a root containing non-UTF-8 bytes, a conclusive mismatch cannot be quarantined at all. The open fails with Io rather than corrupting anything, so it is unavailable-but-intact, not destructive.
| let inventory = kernel_schema_object_inventory(&tx).map_err(|_| KernelError::Inconclusive)?; | ||
| let digest = kernel_schema_digest(&tx).map_err(|_| KernelError::Inconclusive)?; |
There was a problem hiding this comment.
When a crafted or corrupt database has the shared application ID and one structurally valid marker row, classification reaches these calls while the remaining schema is still untrusted. kernel_schema_object_inventory retains every object name and type in a Vec, and kernel_schema_digest materializes each complete SQL definition as a String, so an attacker-controlled number of objects or a very large identifier/definition can exhaust daemon memory instead of returning KernelError::Inconclusive. Check the object count and field lengths against small protocol limits before collecting or hashing the rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Half confirmed. Splitting the two calls, because they behave differently:
kernel_schema_object_inventory does collect without a bound. It runs query_map(...).collect() into a Vec<(String, String)>, so an attacker-controlled object count is retained in full. That part is real.
kernel_schema_digest does not. It iterates while let Some(row) = rows.next()? and folds each field into the running Sha256, so rows are not accumulated. Its exposure is one field at a time: a single very large sql definition materializes as one String before being hashed. Still an exposure, but bounded by the largest single definition rather than by the schema size, which is a different shape from what the comment describes.
Deliberately not fixing this one in this PR, and the distinction from the marker-row bound I did take in 98b03f70 is worth making explicit, since declining the neighbour of something I just fixed otherwise looks arbitrary. That fix was a LIMIT 2 plus width predicates on a query whose expected result is exactly one row, so it could not affect the trusted bootstrap path. Bounding these two is not local in the same way: kernel_schema_digest and kernel_schema_object_inventory are the same functions bootstrap runs against the schema this crate ships, and inventory is compared for equality against expected.inventory. A limit therefore has to be expressed relative to the real object count, which makes it a protocol constant that both paths must agree on, not a guard I can drop into one query.
The shape I would want is LIMIT expected_object_count + 1, so "more objects than the kernel schema has" is detected without materializing them, plus a width bound on sql. That is a small change but it is a contract decision about the schema's declared size, and it deserves to be reviewed as one.
1 participant
Summary
Stack
Part 3 of 8. Depends on #112; followed by #114.