fix: set iVersion=2 to prevent SEGFAULT from null xFetch#80
Open
russellromney wants to merge 2 commits intoorbitinghail:mainfrom
Open
fix: set iVersion=2 to prevent SEGFAULT from null xFetch#80russellromney wants to merge 2 commits intoorbitinghail:mainfrom
russellromney wants to merge 2 commits intoorbitinghail:mainfrom
Conversation
iVersion was set to 3 in PR orbitinghail#76 when SHM support was added, but xFetch and xUnfetch were left as None. SQLite interprets iVersion>=3 as "xFetch is callable" and does not null-check before calling. Under concurrent WAL load (1 writer + 4 readers), SQLite's pager calls xFetch during checkpoint page reads, jumping to address 0x0. ~60% SEGFAULT rate. Fix: iVersion=2 (declares SHM support, not mmap support). SQLite falls back to xRead for all page reads. Includes regression test with a minimal file-backed VFS that reproduces the crash without any external VFS implementation.
Contributor
|
Thanks for the submission! Please remove the regression test. This is a straightforward API compatibility mistake, and the regression test is more complex than I think it strictly needs to be (afaict no need to have it implement shm) |
Author
|
Got it, def more complex than it should be. Do you want me to re-pull without it or keep it in the commit history? |
Contributor
|
Thanks for updating. I just saw your other PR and I'll probably go with that one as it seems straightforward. But nice to have this one as backup. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PR #76 added SHM support (xShmMap, xShmLock, xShmBarrier, xShmUnmap) and bumped
iVersionfrom 1 to 3. ButiVersion: 3declares thatxFetchandxUnfetchare implemented and callable. They were left asNone.SQLite trusts
iVersionand does not null-check function pointers. Under concurrent WAL load, SQLite's pager callsxFetchduring checkpoint page reads, jumping to address0x0. ~60% SEGFAULT rate with 1 writer + 4 readers.The SQLite
iVersioncontract (fromsqlite3.h):Fix
Set
iVersion: 2(SHM support only, no mmap). SQLite falls back toxReadfor all page reads.One-line change + regression test.
Regression test
Includes
tests/concurrent_wal_test.rs: a minimal file-backed VFS (no external dependencies beyond libc) with 1 writer + 4 readers in WAL mode for 3 seconds. On unpatched main, this SEGFAULTs ~60% of the time. With the fix, 30/30 pass.ASAN output on unpatched main:
pc=0x0confirms a null function pointer call, not a null data pointer.Future
A separate PR can add
fetch()/unfetch()methods to theVfstrait to legitimately supportiVersion: 3. Default implementations would returnOk(None)(decline mmap, safe fallback). VFS implementations that want actual mmap can override.