fix(query): size registration to the host, private spill dir, no root, no extension installs at query time - #10
Merged
Conversation
…, no extension installs at query time Closes the five open findings of the MicroVM defensive review of main 4571e14 and pins findings 6 and 7 (closed by #8) with their exact reproductions. 1. Registration OOM under ~1.9 GiB RAM per core. The eager read_json readers reserve about 2x maximum_object_size per thread, so the 1 GiB constant cost 2 GiB a thread and `SET threads=4; SET memory_limit='6GB'` failed to register any corpus with two or more edges.jsonl files. The bound is now sized from the largest file the reader opens (plus a quarter and 1 MiB, floored at DuckDB's 16 MiB default, capped at 1 GiB), and query applies memory_limit, threads and temp_directory BEFORE register(). The cap derives from available RAM (MemAvailable, 80% ceilings, 8 GiB target, 512 MiB floor) and the thread count from the cap (one per 2 GiB, capped at sched_getaffinity). ATIF_SQL_QUERY_MEMORY_LIMIT and ATIF_SQL_QUERY_THREADS override both; malformed values exit 64. The old _query_memory_limit_bytes is the same function, re-derived. Panel on the 300-session corpus: byte-identical output unlimited and under 6GB/4 threads; unlimited wall 0.88-0.98 s -> 0.73-0.79 s. 2. Root could overwrite the corpus through a granted parquet with COPY ... (USE_TMP_FILE false). DuckDB 1.5.5 has no read-only grant (measured: no write switch in duckdb_settings(), and a read_parquet relation bound before enable_external_access=false is refused without a grant), so two layers: query, search and analyze refuse uid 0 with exit 77 root_refused unless ATIF_SQL_ALLOW_ROOT=1 (logged warning), and query refuses every file-facing statement kind before executing anything (COPY, COPY_DATABASE, EXPORT, ATTACH, DETACH, INSTALL, LOAD, PREPARE, EXECUTE) via DuckDB's own extract_statements on the hardened connection, exit 70 sandbox_refused, for any uid. Probe d2 now fails as any uid. 3. The spill directory was <corpus_root>/.duckdb_tmp and writable by caller SQL, persisting files inside the corpus. It is now a per-process tempfile.mkdtemp (0700) under the system temp dir, the sole allowed_directories entry, removed on every exit path after the connection closes. Nothing under the corpus is writable. 4. register_vss ran INSTALL lance (a 242 MB download) during every registration. It now LOADs the extension only when duckdb_extensions() says it is installed, skips the load entirely when no store directory exists, and binds message_embeddings empty with a warning otherwise. query sets autoinstall_known_extensions=false and autoload_known_extensions=false before registration. Installing is an explicit act: `atif-sql embed --install-extension`, or a real embed run. `atif-sql status` reports vector_search ready | no_store | extension_missing; `search` exits 78 extension_missing rather than pretending the store is empty. 5. duckdb_settings() / current_setting() expose the grants and so every session id. DuckDB cannot hide a setting and the grants must be per file, so this is documented as accepted: the caller is the local user and query is not a privilege boundary. Findings 6 and 7: a transcript named `..jsonl` (stem `.`) and one named `*.jsonl`, materialized over three victims, are rejected and the tree is untouched; a session dir named `*` or `???` counts every session once on both read paths and grants no glob. Tests that fail on main 4571e14 and pass here (verified on a scratch checkout): registration under 4 threads/6GB and 16 threads/8GB (CLI and registry), COPY into the spill dir persists nothing, USE_TMP_FILE false over a granted parquet is refused, root is refused, and a corpus with a store and no extension installs nothing.
conftest.py re-exports the fixture modules with `import *`, which skips underscore names, so `_lance_extension_present` never registered and a runner with no cached extension (CI) ran the store-binding tests without lance. Reproduced locally with an empty HOME: 6 failures, then 407 passed once the fixture had a public name and installed the extension itself.
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.
Closes the five open findings of the MicroVM defensive review of main
4571e14(reach-ranked findings 1 to 5) and pins findings 6 and 7 (closed by #8) with their exact reproductions.Finding 1:
querycould not register on a host with under ~1.9 GiB RAM per coreCause, measured: DuckDB's eager
read_jsonreaders reserve about 2xmaximum_object_sizeper thread, so the 1 GiB constant cost 2 GiB a thread. On this hostSET threads=4; SET memory_limit='6GB'thenregister()failed (6.0 GiB/5.5 GiB used), as did 16 threads under 25 GB; 16 MiB read the same rows in 0.10 s. Two or moreedges.jsonlfiles are enough to trigger it (one file registers on main, two don't).Fix:
maximum_object_sizeis sized from the largest file each reader opens (plus a quarter and 1 MiB, floored at DuckDB's 16 MiB default, capped at 1 GiB).queryappliesmemory_limit,threadsandtemp_directoryBEFOREregister(). The cap derives from available RAM (MemAvailable, 80% ceilings, 8 GiB target, 512 MiB floor) and the thread count from the cap (one per 2 GiB, capped atsched_getaffinity).ATIF_SQL_QUERY_MEMORY_LIMITandATIF_SQL_QUERY_THREADSoverride both; malformed values exit 64._query_memory_limit_bytesis the same function re-derived (no duplicate).Proof on the 300-session frozen corpus (
/tmp/findings-corpus, 2.1 GB,LITELLM_LOCAL_MODEL_COST_MAP=true PYTHONHASHSEED=0), 3 reps each,/usr/bin/time:All six after-fix outputs are byte-identical to the before-fix unlimited outputs (
cmp). The unlimited case got faster because the lance extension is no longer loaded when no store exists (0.2 s). Tests:TestQueryResources::test_registration_fits_under_limits_that_used_to_oom[4-6GB|16-8GB],TestRegistrationUnderHostLimits(registry),TestObjectSizeBound, the cap/thread/parse/env-override tests.Finding 2: root could overwrite a granted parquet with
COPY ... (USE_TMP_FILE false)Investigated as asked: DuckDB 1.5.5 has no read-only grant.
duckdb_settings()lists no write switch, and aread_parquetrelation bound beforeenable_external_access=falseis refused at query time without a grant, so the per-file grants must stay and they're read-write. Two layers close it:query,searchandanalyzerefuse uid 0 with exit 77root_refusedunlessATIF_SQL_ALLOW_ROOT=1(a warning is logged).queryrefuses every file-facing statement kind before executing anything, using DuckDB's ownextract_statementson the hardened connection:COPY,COPY_DATABASE,EXPORT,ATTACH,DETACH,INSTALL,LOAD,PREPARE,EXECUTE(an allowlist, so a new kind fails closed). Exit 70, kindsandbox_refused, for any uid; a batch containing one runs nothing.Probe d2 on the real corpus: before
exit 0as root /exit 70 IO Erroras uid 1000; afterexit 70 sandbox_refusedfor any uid andsteps.parquetsha unchanged. Tests:TestQueryStatementGate,TestQueryRefusesRoot,TestQuerySandboxWrites::test_use_tmp_file_false_over_a_granted_parquet_is_refused_for_any_uid, andTestHardenedConnectionLayerproves DuckDB's own layer separately and pins the read-write grant fact the gate exists for.Finding 3: caller SQL could create and read files in
<corpus>/.duckdb_tmp, persistingFix: a per-process
tempfile.mkdtemp(0700) under the system temp dir is the spill directory and the soleallowed_directoriesentry; removed on every exit path after the connection closes. Nothing under the corpus is writable. Probe d3 (COPYinto a pre-created.duckdb_tmp): before exit 0 and the file persisted; after exit 70sandbox_refused, tree digest unchanged. Tests:TestQuerySpillDirectory(in-process, plus a realpython -m atif_cliprocess asserting the corpus tree hash is unchanged, no.duckdb_tmpappears, and no spill dir outlives the process).Finding 4:
querydownloaded a 242 MB extension over the network during registrationFix:
register_vssnever installs. It LOADslanceonly whenduckdb_extensions()says it's installed, skips the load entirely when no store directory exists, and otherwise bindsmessage_embeddingsempty with a warning.querysetsautoinstall_known_extensions=falseandautoload_known_extensions=falsebefore registration (both exist in duckdb 1.5.5). Installing is explicit:atif-sql embed --install-extension, or a real embed run.atif-sql statusreportsvector_searchasready,no_storeorextension_missing;searchexits 78extension_missinginstead of pretending the store is empty. Tests:TestQueryNeverInstallsExtensions(store present, emptyextension_directory, recorded statements contain no INSTALL, directory stays empty),TestVssNeverInstalls(registry), embed install tests.Finding 5: sandbox settings and the session inventory are readable from caller SQL
Assessed: DuckDB cannot hide
duckdb_settings()/current_setting(), and the grants must be per file for lazy reads to work, so the inventory is a property of the design. Documented as accepted inquery's docstring,docs/reference/cli.md, and SECURITY.md: the caller is the local user who can list the corpus andSELECT session_id FROM sessionsanyway, andqueryis not a privilege boundary.Findings 6 and 7 (closed by #8), pinned
TestFilenamesThatOnceDestroyedTheCorpus:..jsonland*.jsonlover three materialized victims are rejected (('*', '.')) and the tree is untouched.TestGlobShapedNamesDoNotMultiplyRows: session dirs named*and???count each session once on both read paths and grant no glob.Fails on main, passes here
A standalone reproduction file with six tests was run on a scratch checkout of
origin/main(4571e14): 6 failed there, 6 passed on this branch.mise run checkgreen (1473 passed, 1 skipped; up from about 1399),mise run docs:gategreen.