Skip to content

fix(rust/ffi)!: avoid aliasing &mut in cancel shims (soundness) - #33

Open
fornwall wants to merge 1 commit into
mainfrom
fix/ffi-cancel-aliasing-soundness
Open

fix(rust/ffi)!: avoid aliasing &mut in cancel shims (soundness)#33
fornwall wants to merge 1 commit into
mainfrom
fix/ffi-cancel-aliasing-soundness

Conversation

@fornwall

@fornwall fornwall commented Jul 14, 2026

Copy link
Copy Markdown
Owner

The bug

The ADBC C API explicitly sanctions calling AdbcStatementCancel / AdbcConnectionCancel from another thread while a call is in flight on the same object — that is cancel's entire purpose:

This can be called during AdbcStatementExecuteQuery (or similar), or while consuming an ArrowArrayStream returned from such. […] This must always be thread-safe (other operations are not).

(adbc.hAdbcStatementCancel, AdbcConnectionCancel)

But the Rust FFI exporter materializes &'a mut ExportedStatement<DriverType> / &'a mut ExportedConnection<DriverType> from private_data in every shim (statement_private_data, connection_private_data) — including statement_cancel and connection_cancel. So the sanctioned cancel-during-execute sequence creates two simultaneously live &mut references to the same object on two threads. That is undefined behavior under Rust's aliasing rules, unconditionally — it does not matter what the code then does with the references.

It "happens to work" today because drivers' cancel() implementations tend to only poke Arc-backed atomics, and current rustc/LLVM don't exploit the noalias license here. Neither is guaranteed. Miri flags the pattern as UB (see Verification below).

Note that changing the trait to cancel(&self) would not be sufficient: forming even a shared & to the statement in the cancel thread still aliases the executing thread's live &mut over the same bytes. The cancel path must not form any reference to the driver object.

The fix: harvested cancel token + per-field borrows

This is the pattern the rest of the ADBC ecosystem already uses — a cancellation handle that is separate from the driver object, harvested at init time:

This PR applies the same shape to the Rust exporter:

  1. adbc_core (core/src/sync.rs): a new CancelToken trait (fn cancel(&self), Send + Sync) plus a required method fn cancel_token(&mut self) -> Option<Arc<dyn CancelToken>> on Connection and Statement.
  2. adbc_ffi (ffi/src/driver_exporter.rs): ExportedStatement / ExportedConnection become two-field structs { inner, cancel }; the token is harvested once on the single-threaded init path (statement_new / connection_init). Then:
    • ordinary shims borrow only the inner field through the raw private_data pointer (&mut (*exported).inner — a field-place projection; no reference spanning the whole struct is ever formed);
    • the cancel shims read only the cancel field ((*exported).cancel.clone()) and call token.cancel() — never touching inner's bytes.
      Disjoint fields reached by field projection from the raw pointer → no overlapping borrows → sound under Stacked/Tree Borrows.
      The connection shims also stop taking &mut FFI_AdbcConnection on the C-side struct (now check_not_null! + a raw-pointer helper, matching the statement shims), so the cancel thread's read of private_data doesn't alias a whole-struct &mut either.
  3. No fallback path. The exporter never calls cancel(&mut self) — that path was unsound under the sanctioned concurrency, so it is gone rather than kept for compatibility. A driver whose cancel_token() returns None gets ADBC_STATUS_NOT_IMPLEMENTED from AdbcConnectionCancel/AdbcStatementCancel (the C API allows cancel to be unimplemented). Connection::cancel/Statement::cancel remain on the traits as the consumer-facing API (sound where &mut is exclusive, e.g. through the driver manager over the C API).
  4. The in-repo dummy driver implements the token for both connection and statement (the connection token reproduces the same round-trip-test error as before), so the existing exporter round-trip tests exercise the sound path. adbc_driver_manager's ManagedConnection/ManagedStatement implement cancel_token as an explicit None with a comment: every C driver call there is serialized behind a mutex, so a genuinely concurrent token needs a locking redesign (out of scope here; their cancel(&mut self) keeps calling the thread-safe C cancel as before).

Breaking change (0.x)

cancel_token has no default implementation — every driver implementing the adbc_core sync Connection/Statement traits must add it. Return a real token to keep cancel working through the FFI exporter (the compiler now forces the choice to be explicit); return None if the driver cannot support concurrent cancellation, which surfaces as NOT_IMPLEMENTED to C consumers instead of silently-UB fallback behavior.

Verification

In-process test rust/ffi/tests/cancel_during_execute.rs: a minimal driver whose execute_update / get_table_types block until the cancel token fires, driven through the exported C function pointers, with the cancel shim invoked from a second thread while the executing shim is blocked inside the driver — i.e. exactly the sanctioned concurrency. The driver objects are deliberately non-ZST and touched on both sides of the blocking call so the aliasing is observable. A second test drives a tokenless configuration of the same driver (via a database option) and asserts both cancel entry points return ADBC_STATUS_NOT_IMPLEMENTED.

  • cargo +nightly miri test -p adbc_ffi --test cancel_during_execute passes (both tests) with this fix.
  • As a negative control during development, pointing the test driver at the old fallback path (blocking cancel(&mut self) reached via &mut from the cancel shim) makes Miri report the expected aliasing UB: not granting access to tag <…> because that would remove [Unique for <…>] which is strongly protected — the executing thread's protected &mut self vs. the cancel thread's &mut. That path no longer exists in the exporter.
  • The pre-existing dlopen-based driver_exporter_dummy round-trip tests (23) pass, now via the token path, including the test_connection_cancel error round-trip.

Checks run

From rust/ (branched from this fork's main = apache/arrow-adbc main @ 9be0298; rust/ is identical there to rev 198f39a that downstream currently pins):

  • cargo fmt --all --check — clean
  • cargo clippy -p adbc_core -p adbc_ffi -p adbc_dummy --all-targets --all-features -- -Dwarnings — clean; cargo clippy -p adbc_driver_manager --lib -- -Dwarnings — clean
  • cargo test --all-targets --workspace --no-fail-fast — all green except the pre-existing/environmental items below
  • cargo test --doc, RUSTDOCFLAGS=-Dwarnings cargo doc --no-deps -p adbc_core -p adbc_ffi -p adbc_driver_manager — clean except below
  • Miri as described above

Pre-existing on the unmodified base (not introduced here, left untouched):

  • cargo clippy -p adbc_driver_manager --all-targets -- -Dwarnings fails with dead-code lints in driver_manager/tests/common/mod.rs (local stable 1.97).
  • The driver_manager_sqlite test suite and one adbc_driver_manager doctest require libadbc_driver_sqlite.so (installed by CI, absent locally).

🤖 Generated with Claude Code

https://claude.ai/code/session_01MK2VgNRk3cwLigCWwf2yBL

@fornwall
fornwall force-pushed the fix/ffi-cancel-aliasing-soundness branch from f443a0c to 5ca7ead Compare July 14, 2026 15:22
@fornwall fornwall changed the title fix(rust/ffi): avoid aliasing &mut in cancel shims (soundness) fix(rust/ffi)!: avoid aliasing &mut in cancel shims (soundness) Jul 14, 2026
@fornwall
fornwall force-pushed the fix/ffi-cancel-aliasing-soundness branch 2 times, most recently from 8d2bc86 to b008088 Compare July 16, 2026 12:48
The ADBC C API allows AdbcConnectionCancel/AdbcStatementCancel to be
called from another thread while a call is in flight on the same object,
but the exporter shims materialize `&mut` to the exported object from
`private_data` in every shim, including the cancel ones. A sanctioned
cancel-during-execute therefore created two simultaneously live `&mut`
to the same object on two threads, which is undefined behavior.

Fix by adding a required `CancelToken` handle to the adbc_core sync
traits (`cancel_token`), harvesting the token once at
connection/statement init, and storing it in a field disjoint from the
driver object. Ordinary shims borrow only the `inner` field; the cancel
shims read only the `cancel` field, so no borrows overlap.

BREAKING CHANGE: `cancel_token` has no default implementation — every
driver must implement it. There is no fallback to the old
`cancel(&mut self)` path in the exporter (it was unsound under the
sanctioned concurrency); a driver returning `None` gets
ADBC_STATUS_NOT_IMPLEMENTED from the cancel entry points instead.

Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
@fornwall
fornwall force-pushed the fix/ffi-cancel-aliasing-soundness branch from b008088 to 6a5f580 Compare July 16, 2026 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant