fix: SQLCopyDesc phase one runs under a panic guard

The forward_ffi! stub calls sql_copy_desc directly and phase one (the
source-descriptor snapshot) had no catch_unwind above it, so a backend
panic in describe_col during an IRD snapshot unwound out of the
extern "system" boundary — a guaranteed host-process abort. Phase one's
closure now catches the panic via panic::catch_panic_as_error and folds
it into the existing error path, so it surfaces as HY000 on the target's
diagnostic queue like every other caught panic, with no new lock site
and the two-phase (never two group locks) structure intact.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: adwk67

AGENTS.md

@@ -1316,6 +1316,22 @@ ODBC Application (e.g. isql)
   for a new entry point, and "it takes no handle" is a reason to reach for
   `panic_safe_unlocked`, not a reason to skip the guard — `ConfigDSNW` had no
   guard at all until 2026-07-30 on exactly that reasoning.
+
+  **`SQLCopyDesc` is the one export a single guard cannot cover, because it is
+  the one export that takes two lock phases rather than one** (see
+  "Descriptors" → "The explicit-descriptor rulings" above for why). Phase two
+  is an ordinary `panic_safe` on the target. Phase one holds only the
+  *source*'s group — through `HandleScope::with_group`, which is a plain
+  lock-then-call with no `catch_unwind` of its own — so a panic reaching
+  `describe_col` through `snapshot_ird` had no guard at all until 2026-07-31,
+  the same gap `ConfigDSNW` had. `panic::catch_panic_as_error` closes it:
+  narrower than `panic_safe_unlocked`, because it does not itself sit at the
+  FFI boundary — it converts the panic into the same `OdbcError` shape a
+  non-panicking phase-one failure (`HY007`) already returns, and phase two's
+  `panic_safe` posts it to the target's queue either way, which is where the
+  whole call's diagnostics belong. So `sql_copy_desc` is fully guarded, just not by
+  "one of the two" in the sense above — the property this bullet is
+  asserting one export needs a third shape to satisfy.
 - **W-only for string-bearing functions**: every ODBC function that takes or
   returns a string is exported only in its Wide (`W`-suffix) form; the Driver
   Manager translates an ANSI application's calls into those. Functions with no
@@ -1367,7 +1383,7 @@ Generic framework. Zero database-specific code.
 | `handles/scope.rs` | `HandleScope` — the only way to reach a handle's contents; token validation without dereferencing the application's pointer |
 | `sync.rs` | The one import path for every lock in the crate; aliases to `loom`'s primitives under `#[cfg(all(loom, test))]`, `std::sync` otherwise |
 | `utf16.rs` | `utf16_to_string`, `write_utf16` (ODBC uses UTF-16LE) |
-| `panic.rs` | `panic_safe` (locks the target's group, builds a `HandleScope`, catches panics) and `panic_safe_unlocked` (`SQLCancel`'s lock-free sibling) |
+| `panic.rs` | `panic_safe` (locks the target's group, builds a `HandleScope`, catches panics), `panic_safe_unlocked` (`SQLCancel`'s lock-free sibling), and `catch_panic_as_error` (`SQLCopyDesc` phase one's panic-to-`OdbcError` guard) |
 | `logging.rs` | `init_logging()` via tracing, configured by `ODBC_LOG_LEVEL` / `ODBC_LOG_FILE` |
 | `function_id.rs` | `FunctionId` enum + `function_id_from_raw()` for `SQL_API_*` constants |
 | `test_support.rs` | `test-support`-feature-gated hooks a driver's test suite uses to put a connection into a handle without `SQLDriverConnectW` |

CHANGELOG.md

@@ -3066,4 +3066,23 @@ call `SQLCloseCursor` or `SQLFreeStmt(SQL_CLOSE)` first, as it already must for
   framing of the attribute as shifting a *buffer* — a pointer with no buffer
   behind it has nothing to shift.
 
+- **`SQLCopyDesc`'s phase one now runs under a panic guard.** It copies a
+  descriptor in two lock phases — the source's group alone, then the target's
+  — and only phase two was wrapped by `panic_safe`. Phase one calls
+  `describe_col` (via `snapshot_ird`) whenever the source is an IRD, which is
+  driver-author code and the exact panic surface every other `Backend` call
+  runs under a guard for; a panic there had no `catch_unwind` above it at all,
+  so it unwound straight through `HandleScope::with_group` and across the
+  `extern "system"` boundary `forward_ffi!` generates for
+  `SQLCopyDesc` — aborting the process rather than returning `SQL_ERROR`.
+
+  The fix is a new `panic::catch_panic_as_error`, narrower than
+  `panic_safe_unlocked`: phase one has no target handle to post a diagnostic
+  through yet, so it folds a caught panic into the same `OdbcError::Panic` a
+  non-panicking phase-one failure (`HY007`) already produces, and phase two's
+  ordinary `panic_safe` posts it to the target's queue as `HY000` — exactly
+  where the spec says this call's diagnostics belong. No SQLSTATE this
+  function returns changed; the previously-unguarded path now returns one
+  (`HY000`) instead of aborting.
+
 [Unreleased]: https://github.com/stackabletech/stackable-odbc-core/commits/HEAD

src/ffi/desc.rs

@@ -1189,8 +1189,14 @@ pub unsafe fn sql_set_desc_rec<B: Backend>(
 /// - `01000` General warning — not produced here.
 /// - `08S01` Communication link failure — not returned; this call performs no
 ///   I/O.
-/// - `HY000` General error — returned for a failure with no more specific code,
-///   including an internal panic caught by `panic_safe`.
+/// - `HY000` General error — returned for a failure with no more specific
+///   code, including an internal panic. Phase two's own panic is caught by
+///   `panic_safe`, as everywhere else; phase one's is caught by
+///   `panic::catch_panic_as_error` (`src/panic.rs`, crate-private), since
+///   phase one holds no target handle to post a diagnostic through yet —
+///   both are folded into the same
+///   `OdbcError::Panic` and posted to the target's queue by phase two's
+///   `panic_safe`, exactly where `HY007` is.
 /// - `HY001` Memory allocation error — not returned; allocation here is an
 ///   infallible `Box`/`HashMap` clone.
 /// - `HY007` Associated statement is not prepared — returned when the source is
@@ -1244,7 +1250,15 @@ pub unsafe fn sql_copy_desc<B: Backend>(
     // guard — so that the lock is released before phase two is a fact its
     // signature states rather than something this function has to remember.
     let Some(snapshot) = HandleScope::with_group(source_desc_handle, HandleKind::Desc, |scope| {
-        scope.snapshot_descriptor::<B>(source_desc_handle)
+        // `with_group` is a plain lock-then-call with no `catch_unwind` of its
+        // own, so a panic from `describe_col` (via `snapshot_ird`) would
+        // otherwise unwind straight through it and across the `extern
+        // "system"` boundary. Caught here and folded into the same `Err`
+        // shape a non-panicking failure already returns, so it flows through
+        // the `snapshot?` below and phase two's `panic_safe` posts it to the
+        // target's queue exactly where every other diagnostic this call
+        // makes belongs.
+        crate::panic::catch_panic_as_error(|| scope.snapshot_descriptor::<B>(source_desc_handle))
     }) else {
         tracing::debug!("SQLCopyDesc -> INVALID_HANDLE (source)");
         return SqlReturn::INVALID_HANDLE;
@@ -1331,8 +1345,9 @@ mod tests {
     use crate::ffi::diag::sql_get_diag_rec_w;
     use crate::ffi::stmt_attr::sql_get_stmt_attr_w;
     use crate::test_utils::{
-        MockAltBackend, MockBackend, MockLongDataBackend, MockRecordingBackend,
-        MockTypeInfoBackend, alloc_env_conn_stmt, cleanup_env_conn_stmt, with_descriptor,
+        MockAltBackend, MockBackend, MockLongDataBackend, MockPanickingDescribeBackend,
+        MockRecordingBackend, MockTypeInfoBackend, alloc_env_conn_stmt, cleanup_env_conn_stmt,
+        with_descriptor,
     };
     use crate::types::sql_state;
     use odbc_sys::{CDataType, HandleType, ParamType, SqlDataType, StatementAttribute};
@@ -3180,6 +3195,87 @@ mod tests {
         }
     }
 
+    /// Audit finding B4: phase one runs `describe_col` (through
+    /// `snapshot_ird`) before phase two's `panic_safe` is ever reached, so a
+    /// panicking `describe_col` used to unwind straight out of
+    /// `sql_copy_desc` and across the `extern "system"` boundary
+    /// `forward_ffi!` generates for it — an abort, not a `SqlReturn`. Driven
+    /// through `catch_unwind` because that escape is exactly what a bare
+    /// `assert_eq!` on the return value cannot see: before the fix, this test
+    /// itself never gets to the `assert_eq!`, because the panic unwinds
+    /// through it too.
+    #[test]
+    fn copy_desc_from_ird_with_panicking_describe_col_returns_error_not_abort() {
+        unsafe {
+            let (env, conn, stmt) =
+                crate::test_utils::alloc_connected_env_conn_stmt::<MockPanickingDescribeBackend>();
+
+            let sql: Vec<u16> = "SELECT 1".encode_utf16().collect();
+            assert_eq!(
+                crate::ffi::execute::sql_exec_direct_w::<MockPanickingDescribeBackend>(
+                    stmt,
+                    sql.as_ptr(),
+                    i32::try_from(sql.len()).expect("short"),
+                ),
+                SqlReturn::SUCCESS,
+                "precondition: the statement ran and the IRD has a column to describe"
+            );
+
+            let ird =
+                desc_token_of::<MockPanickingDescribeBackend>(stmt, StatementAttribute::ImpRowDesc);
+            let mut target: *mut c_void = std::ptr::null_mut();
+            assert_eq!(
+                crate::ffi::handle::sql_alloc_handle::<MockPanickingDescribeBackend>(
+                    HandleType::Desc as i16,
+                    conn,
+                    &mut target,
+                ),
+                SqlReturn::SUCCESS,
+                "precondition: an explicit target descriptor"
+            );
+
+            let result = std::panic::catch_unwind(|| {
+                sql_copy_desc::<MockPanickingDescribeBackend>(ird, target)
+            });
+            assert!(
+                result.is_ok(),
+                "a panic in phase one must be caught and reported as SQL_ERROR, \
+                 not unwind across the extern \"system\" boundary"
+            );
+            assert_eq!(result.unwrap(), SqlReturn::ERROR);
+            assert_eq!(
+                first_sqlstate_of::<MockPanickingDescribeBackend>(target),
+                sql_state::GENERAL_ERROR,
+                "the panic is routed through phase two's panic_safe onto the \
+                 target's queue, exactly where SQLCopyDesc's other diagnostics \
+                 (e.g. HY007) are posted"
+            );
+
+            // The group lock was never held across the panic (it is caught
+            // inside phase one's own closure, before `with_group` even sees
+            // it), so it is not poisoned and the statement is still usable —
+            // prove it with an ordinary follow-up call.
+            let mut columns: i16 = -1;
+            assert_eq!(
+                crate::ffi::cursor::sql_num_result_cols::<MockPanickingDescribeBackend>(
+                    stmt,
+                    &mut columns,
+                ),
+                SqlReturn::SUCCESS,
+                "the statement's group lock must still be usable after the panic"
+            );
+            assert_eq!(columns, 1);
+
+            let _ = crate::ffi::handle::sql_free_handle::<MockPanickingDescribeBackend>(
+                HandleType::Desc as i16,
+                target,
+            );
+            crate::test_utils::cleanup_connected_env_conn_stmt::<MockPanickingDescribeBackend>(
+                env, conn, stmt,
+            );
+        }
+    }
+
     /// The APD's token, as the application receives it.
     unsafe fn apd_of(stmt: *mut c_void) -> *mut c_void {
         let mut token: *mut c_void = std::ptr::null_mut();

src/panic.rs

@@ -109,6 +109,43 @@ where
     }
 }
 
+/// Catch a panic escaping `f`, converting it into the same
+/// [`OdbcError::Panic`] shape a non-panicking failure already returns.
+///
+/// `SQLCopyDesc`'s phase one (`ffi::desc::sql_copy_desc`) calls this from
+/// inside `HandleScope::with_group` on the *source* descriptor's group,
+/// before phase two's [`panic_safe`] ever runs on the *target*. `with_group`
+/// carries no `catch_unwind` of its own — it is a plain lock-then-call, not an
+/// FFI-boundary guard — so a panic reaching `describe_col` through
+/// `snapshot_ird` (driver-author code, the same surface every other
+/// `Backend` call runs under a guard for) would otherwise unwind straight
+/// through it, past `sql_copy_desc`, and across the `extern "system"`
+/// boundary `forward_ffi!` generates for it.
+///
+/// Unlike [`panic_safe_unlocked`], this is not itself an FFI-boundary guard:
+/// it exists so phase one's panic can be turned into the *same* `Err` shape
+/// phase one already returns for a non-panicking failure (`HY007`, an
+/// unpopulated IRD), rather than becoming a second, cruder failure mode. Both
+/// flow through phase two's `panic_safe` unchanged, via the `?` on the
+/// snapshot [`sql_copy_desc`] already had — so the panic is posted as `HY000`
+/// to the *target*'s diagnostic queue, exactly where the spec says this
+/// call's diagnostics belong (and where `HY007` was already posted). There is
+/// no handle to push a diagnostic through at the point this function runs —
+/// the same reason [`panic_safe_unlocked`] posts none on its own two call
+/// sites — so it returns a plain `Result` for its caller to route onward
+/// instead of trying.
+///
+/// [`panic_safe`]: crate::panic::panic_safe
+pub(crate) fn catch_panic_as_error<T>(
+    f: impl FnOnce() -> Result<T, OdbcError>,
+) -> Result<T, OdbcError> {
+    std::panic::catch_unwind(AssertUnwindSafe(f)).unwrap_or_else(|_panic| {
+        Err(OdbcError::Panic {
+            message: "internal driver panic".into(),
+        })
+    })
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

src/test_utils.rs

@@ -4014,6 +4014,135 @@ impl Backend for MockFailingDescribeBackend {
     minimal_capability_decls!();
 }
 
+// ---------------------------------------------------------------------------
+// A backend whose describe_col panics, for SQLCopyDesc phase-one panic safety
+// ---------------------------------------------------------------------------
+
+/// A statement whose `describe_col` panics instead of returning an error.
+///
+/// `column_count` is 1 — enough to make `snapshot_ird`'s loop
+/// (`src/handles/scope.rs`) call `describe_col` at all, which is the one
+/// driver-author call `SQLCopyDesc`'s phase one runs before phase two's
+/// `panic_safe` is even reached.
+pub struct MockPanickingDescribeStatement;
+
+impl StatementBackend for MockPanickingDescribeStatement {
+    type Error = OdbcError;
+
+    fn column_count(&self) -> i16 {
+        1
+    }
+
+    fn describe_col(
+        &self,
+        _col: u16,
+    ) -> Result<Cow<'_, crate::types::ColumnDescriptor>, OdbcError> {
+        panic!("mock describe_col panic");
+    }
+}
+
+/// Hands out statements whose column metadata cannot be described without
+/// panicking.
+///
+/// The `MockFailingDescribeBackend` sibling above proves core surfaces a
+/// backend's own *error*; this one proves core survives a backend's *panic*
+/// — the distinction `SQLCopyDesc`'s phase one needed a guard for.
+pub struct MockPanickingDescribeBackend;
+
+impl Backend for MockPanickingDescribeBackend {
+    type Connection = MockConnection;
+    type Statement = MockPanickingDescribeStatement;
+    type Error = OdbcError;
+    type CancelToken = MockCancelToken;
+
+    fn connect(_: &ConnectParams) -> Result<MockConnection, OdbcError> {
+        Ok(MockConnection)
+    }
+    fn disconnect(_: &mut MockConnection) -> Result<(), OdbcError> {
+        Ok(())
+    }
+    fn cancel_token(_conn: &Self::Connection) -> Self::CancelToken {
+        MockCancelToken::default()
+    }
+    fn cancel(token: &Self::CancelToken) -> Result<(), Self::Error> {
+        token
+            .cancelled
+            .store(true, std::sync::atomic::Ordering::SeqCst);
+        Ok(())
+    }
+    fn is_cancelled(token: &Self::CancelToken) -> bool {
+        token.cancelled.load(std::sync::atomic::Ordering::SeqCst)
+    }
+    fn exec_direct(
+        _: &MockConnection,
+        _: &Self::CancelToken,
+        _: &str,
+    ) -> Result<MockPanickingDescribeStatement, OdbcError> {
+        Ok(MockPanickingDescribeStatement)
+    }
+    fn prepare(
+        _: &MockConnection,
+        _: &Self::CancelToken,
+        _: &str,
+    ) -> Result<MockPanickingDescribeStatement, OdbcError> {
+        Ok(MockPanickingDescribeStatement)
+    }
+    fn execute(
+        _: &MockConnection,
+        _: &Self::CancelToken,
+        _: &mut MockPanickingDescribeStatement,
+        _: &[crate::types::ColumnValue],
+    ) -> Result<crate::types::ExecuteOutcome, OdbcError> {
+        Ok(crate::types::ExecuteOutcome::default())
+    }
+    fn get_info(_: &MockConnection, _: crate::types::InfoType) -> Result<InfoValue, OdbcError> {
+        Err(OdbcError::NotImplemented {
+            feature: "get_info".into(),
+        })
+    }
+    fn get_functions() -> Cow<'static, [crate::function_id::FunctionId]> {
+        Cow::Borrowed(&[])
+    }
+    fn get_type_info(_conn: &Self::Connection) -> Cow<'static, [TypeInfoRow]> {
+        Cow::Borrowed(&[])
+    }
+    fn tables(
+        _: &MockConnection,
+        _: &Self::CancelToken,
+        _: &crate::types::TablesQuery<'_>,
+    ) -> Result<Vec<TableRow>, OdbcError> {
+        Ok(Vec::new())
+    }
+    fn columns(
+        _: &MockConnection,
+        _: &Self::CancelToken,
+        _: &crate::types::ColumnsQuery<'_>,
+    ) -> Result<Vec<ColumnRow>, OdbcError> {
+        Ok(Vec::new())
+    }
+
+    fn supports_catalogs(_conn: &Self::Connection) -> bool {
+        false
+    }
+    fn supports_schemas(_conn: &Self::Connection) -> bool {
+        false
+    }
+    fn alter_table_support(_conn: &Self::Connection) -> u32 {
+        0
+    }
+    fn outer_join_capabilities(_conn: &Self::Connection) -> u32 {
+        0
+    }
+    fn default_txn_isolation(_conn: &Self::Connection) -> u32 {
+        0
+    }
+    fn txn_isolation_options(_conn: &Self::Connection) -> u32 {
+        0
+    }
+
+    minimal_capability_decls!();
+}
+
 // ---------------------------------------------------------------------------
 // A backend that rejects an unknown catalog, for 3D000
 // ---------------------------------------------------------------------------