Conversation
added 5 commits
September 10, 2026 12:30
mdbtools' SQLDriverConnect ignores cbConnStrIn and scans for a NUL terminator, while odbc-api passes a Rust &str plus an explicit length. The driver read past the string into uninitialized heap, appended the garbage to the DBQ value and stat'ed a nonexistent path, so connections failed intermittently with "Unable to locate database". Borrow the connection string from a CString so the terminator sits right after it.
mdbtools reports SQLDescribeCol nullability as `!col->is_fixed`, which describes the physical storage layout rather than whether a column can hold NULL. Fixed-width columns containing NULLs were reported NOT NULL and tripped Arrow's non-nullable checks. The reported flag carries no usable information, so always mark MDB columns nullable.
Add integration-tests::setup_mdb_layers() plus the esri_layers.mdb fixture, and tests that read its two feature layers sequentially, from several tasks at once, and through repeated Pool::get calls on distinct connection identities. Wire them into CI.
mdbtools' SQLGetData returns SQL_NO_DATA for a text, memo or OLE value of length zero: `to_c_char` and the MDB_OLE case compare the read position against the value length, and 0 >= 0 holds before anything is copied. odbc-api turns SQL_NO_DATA into a panic, so any table with an empty text or memo cell (such as the ESRI GDB_Items system table) failed to read at all. Read variable-length columns through the raw SQLGetData call instead, where SQL_NO_DATA simply means there is nothing left to read. The bytes gathered so far become the cell value, so an empty value stays empty rather than failing the query.
GDB_Items in the bundled fixture has an empty Path in 10 of its 16 rows, which made the table unreadable before the SQLGetData fix. Rename tests/mdb_concurrent.rs to tests/mdb_layers.rs, since it now covers reads over the ESRI layers rather than only concurrent ones.
Reading a table with a zero-length text, memo or OLE cell panics again with "Unexepcted SQL_NO_DATA returned by ODBC function", so the ESRI GDB_Items system table is unreadable again. The fix is out of scope for this change set and is tracked in systemxlabs#105 instead. This reverts 7673c14 and 4f2831d, restoring the tree of 9208101.
Open
Collaborator
Author
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
1 participant
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
Reading
.mdbfiles over ODBC failed intermittently with:Those trailing bytes after the path are the tell: the path handed to
stat()was corrupted.Root cause
mdbtools'
libmdbodbc.soSQLDriverConnectignores thecbConnStrInlength argument and parses the connection string with NUL-terminated C string functions (strcasestr,strchr,g_strsplit). odbc-api 28 passes a Rust&strpointer plus an explicit length, and a Rust&strhas no trailing NUL — so the driver reads past the string into uninitialized heap and appends whatever it finds to theDBQ=value.Because the garbage depends on prior heap contents, the failure looked intermittent and was reported as a concurrency problem. Reproduced under strace:
Changes
NUL-terminate the connection string.
MdbPool::getnow borrows the string from aCString, whose buffer has the terminator immediately after the bytes, and keeps it alive across the call. This is the only mdbtools entry point that over-reads:SQLExecDirectdelegates toSQLPrepare, which does respect the explicit length.Treat every MDB column as nullable. mdbtools reports
SQLDescribeColnullability as!col->is_fixed(src/odbc/odbc.c), which describes physical storage layout rather than whether a column can hold NULL. Fixed-width columns that do contain NULLs were reported NOT NULL, tripping Arrow'sdeclared as non-nullable but contains null values. The reported flag carries no usable information, so it is no longer consulted.Tests and CI. New
integration-tests::setup_mdb_layers()loader plus a committed 1.9 MBesri_layers.mdbfixture (two feature layers, 47 and 242 rows, plus ESRI system tables).tests/mdb_concurrent.rsreads the layers sequentially, from several tasks at once, and through repeatedPool::getcalls on distinct connection identities. Added to the CI integration test invocation.Known issues
Not addressed here, tracked separately in #105: mdbtools answers
SQL_NO_DATAfor a text, memo or OLE value of length zero, and odbc-api turns that into a panic. Any table holding such a cell — the ESRIGDB_Itemssystem table, for one — still fails to read.Verification
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test -p integration-tests --test mdb(15 passed),--test mdb_concurrent(4 passed),cargo test -p datafusion-remote-tablestraceconfirmsrepeated_pool_getopens the fixture 40 times (previously once, because of the process-global connection cache), and connecting to the file that triggered the report succeeds 10/10 runs where it previously failed 10/10.SHAPEblob is 17,152 bytes and 34 of them exceed 4 KiB; their ESRI headers decode to shape type 3 (PolyLine), as expected for境界线.