CurtHagenlocher opened a new pull request, #4884:
URL: https://github.com/apache/arrow-adbc/pull/4884
## What's changed
This fixes bugs in the C# driver importer (`CAdbcDriverImporter`) and
exporter (`CAdbcDriverExporter`), plus one in `AdbcOptions`. All of them
are in the code on `main` today. I found them while working on fuller
ADBC 1.1 support in C#, which will follow in a separate PR. None of
these fixes depends on that work, and none is a regression from it.
Each fix is a separate commit with its own explanation, so the PR can be
reviewed one commit at a time.
### Crashes and memory safety
- **`ExecutePartitioned` could crash the process.**
- The importer allocated the partitions struct without zeroing it.
It then called its `release` callback whether or not the driver
had written the struct. Any failure, including a driver that
doesn't implement partitions, called a garbage function pointer.
- On netstandard2.0, `release != null` on an `IntPtr` is always true,
so the callback was also called when it was zero.
- In the exporter, the result schema was never exported. The
partitions struct was also published before its buffers were
allocated.
- **Error messages were freed with the wrong allocator.**
- On .NET 5+, the exporter allocated error messages with CoTaskMem
but freed them with `FreeHGlobal`.
- On netstandard2.0, `MarshalExtensions.StringToCoTaskMemUTF8`
allocated with HGlobal, but the importer frees those strings with
`FreeCoTaskMem`.
- **The exporter's error reporting could crash the caller or itself.**
- It dereferenced a NULL `error`, though the argument is optional.
- It freed an error left in the caller's struct as if this exporter
had created it, instead of calling that error's own `release`.
- It could throw out of an unmanaged entry point, for example from a
driver's `SqlState` override, which ends the process.
### Incorrect behavior
- **Updates failed through the exporter.** A NULL stream passed to
`StatementExecuteQuery` means the caller doesn't want a result set.
The exporter ignored that and exported into the NULL pointer. The
importer's own `ExecuteUpdate` passes NULL, so exporting a managed
driver and importing it again broke updates.
- **Vendor error codes were lost in both directions.** The exporter
always reported 0. The importer read the vendor code only when the
error also had a SQLSTATE.
- **Missing functions failed on .NET Framework.** When a driver leaves
`ConnectionCommit`, `ConnectionRollback`, `ConnectionCancel` or
`StatementCancel` empty, the importer fills in a function that
reports `NotImplemented`. On .NET Framework, calling it threw
`InvalidCastException` instead. The fill-ins were created with a
different delegate type from the one they're called through, and
.NET Framework returns the original delegate for a function pointer
created from a managed delegate.
- **A failed cleanup hid the real error.** When creating a database,
connection or statement failed and the importer's cleanup release
also failed, the caller saw the release error.
- **Managed streams leaked if exporting them failed.**
- **Two ingest modes were unrecognized.**
`AdbcOptions.GetIngestMode(string)` didn't recognize the ADBC 1.1
ingest modes `replace` and `create_append`.
### Docs
- `AdbcConnection.GetInfo` now says that an empty code list requests
all codes. The drivers already treat it that way, and it's how a
NULL list from the C API arrives.
## Testing
- **New tests:** most fixes have a test in
`ExportedDriverRoundTripTests` or the new `AdbcOptionsTests`. I ran
these against `main` without their fixes, and each one fails:
- The update test fails.
- The vendor-code test fails.
- The cleanup-error test fails.
- The partitions test crashes the test host with an access violation.
- The missing-functions test fails with `InvalidCastException` on
net472.
- **Tests that only run on .NET 5 and later:** two tests call the
exporter's function pointers directly. They cover the cleanup-error
fix and the error-reporting fix.
- **No test:** the allocator fix and the stream leak. Making the
export fail would mean writing through a NULL output pointer.
- **Results:**
- The full solution builds without warnings.
- The core test project passes on net10.0, net8.0 and net472.
- The AoT interop tests pass against a fixture published from this
branch.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
