fix(rust/ffi): preserve caller's AdbcError private_data on the 1.0.0 path - #2
Closed
fornwall wants to merge 3 commits into
Closed
fix(rust/ffi): preserve caller's AdbcError private_data on the 1.0.0 path#2fornwall wants to merge 3 commits into
fornwall wants to merge 3 commits into
Conversation
fornwall
force-pushed
the
fix-error-compatibility
branch
from
July 6, 2026 22:16
6f19405 to
80e4b50
Compare
fornwall
force-pushed
the
fix-error-compatibility
branch
3 times, most recently
from
July 7, 2026 19:42
098130a to
d5cf695
Compare
…path The FFI driver exporter wrote the entire FFI_AdbcError struct into the caller's out-pointer on every failed method, unconditionally clobbering the private_data (and, at two of the three sites, private_driver) fields. The AdbcError documentation in c/include/arrow-adbc/adbc.h says these fields exist only in the ADBC 1.1.0 layout and that a driver "should read/write these fields if and only if vendor_code is equal to ADBC_ERROR_VENDOR_CODE_PRIVATE_DATA", and "should never touch more than [the 1.0.0-sized] portion of an AdbcError struct" otherwise. Overwriting them corrupts memory an ADBC 1.0.0 caller owns (or does not have at all). Route all three error-writing sites (check_err!, pointer_as_mut! and the panic handler in catch_panic) through a new set_error_out() that: - reads the caller's vendor_code first; when it is not the sentinel, writes only the 1.0.0-sized prefix (message/vendor_code/sqlstate/release) and installs a message-only release, leaving private_data/private_driver as the caller set them; - otherwise behaves as before: preserves private_driver and writes the full struct, with structured details carried in private_data. This makes the C++ validation suite's StatementTest.ErrorCompatibility pass. Adds unit tests for both the opted-in and 1.0.0-compatible paths. Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
fornwall
force-pushed
the
fix-error-compatibility
branch
from
July 8, 2026 00:22
d5cf695 to
75cab7d
Compare
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.
What
The FFI driver exporter wrote the entire
FFI_AdbcErrorstruct into the caller's out-pointer on every failed method, unconditionally clobberingprivate_data(and, at two of the three sites,private_driver).c/include/arrow-adbc/adbc.hdocuments that those fields only exist in the ADBC 1.1.0 layout, and that a driver "should read/write these fields if and only if vendor_code is equal toADBC_ERROR_VENDOR_CODE_PRIVATE_DATA" and "should never touch more than [the 1.0.0-sized] portion of an AdbcError struct" otherwise. Overwriting them corrupts memory an ADBC 1.0.0 caller owns (or does not have at all).How
All three error-writing sites (
check_err!,pointer_as_mut!, and the panic handler incatch_panic) now route through a newset_error_out()that:vendor_codefirst; when it is not the sentinel, writes only the 1.0.0-sized prefix (message/vendor_code/sqlstate/release) and installs a message-only release, leavingprivate_data/private_driverexactly as the caller set them;private_driverand writes the full struct, with structured details carried inprivate_data.Test
Makes the C++ validation suite's
StatementTest.ErrorCompatibilitypass. Adds Rust unit tests for both the opted-in and 1.0.0-compatible paths; the 1.0.0 test fails against the pre-fix behavior (clobberedprivate_data) and passes after.Review follow-up (second commit)
Compared
set_error_outagainst the C reference (InternalAdbcSetErrorVariadic,c/driver/common/utils.c) and aligned two behaviors:adbc.hforbids consumers from readingprivate_data/private_driverunlessvendor_codeholdsADBC_ERROR_VENDOR_CODE_PRIVATE_DATA, and this crate's ownError::try_from(&FFI_AdbcError)gates on it — but the full-struct write replaced the sentinel with the driver error's own vendor code, making the freshly-stashed details unreachable. The sentinel is now kept, as the C reference does (the driver's own vendor code is not representable on this path — that is the documented 1.1.0 trade-off).AdbcErroracross calls no longer leaks the earlier message/details.Tests: the opted-in test now asserts the sentinel survives; a new test covers the reuse path.