Message-ID: <gh-postgresql-interfaces-psqlodbc-183-c4405617165@github.com>
From: "davecramer (@davecramer)" <noreply+davecramer@github.com>
To: "postgresql-interfaces/psqlodbc" <noreply+postgresql-interfaces-psqlodbc@github.com>
Date: Fri, 08 May 2026 10:16:04 +0000
Subject: Re: [postgresql-interfaces/psqlodbc] PR #183: Detach freed app descriptors from statements
In-Reply-To: <gh-postgresql-interfaces-psqlodbc-183@github.com>
References: <gh-postgresql-interfaces-psqlodbc-183@github.com>
List-Id: <gh-postgresql-interfaces-psqlodbc.github.com>
X-GitHub-Author-Login: davecramer
X-GitHub-Comment-Id: 4405617165
X-GitHub-Comment-Type: issue_comment
X-GitHub-Issue: 183
X-GitHub-Repo: postgresql-interfaces/psqlodbc
X-GitHub-Type: comment
X-GitHub-Url: https://github.com/postgresql-interfaces/psqlodbc/pull/183#issuecomment-4405617165
Content-Type: text/plain; charset=utf-8

This PR fixes a use-after-free vulnerability where freeing an explicitly-allocated application descriptor
  (SQLFreeHandle(SQL_HANDLE_DESC)) leaves dangling pointers in statements that had the descriptor attached via
  SQL_ATTR_APP_ROW_DESC / SQL_ATTR_APP_PARAM_DESC. Subsequent operations on those statements dereference freed memory.
  
  The Bug
  
  1. App allocates a descriptor with SQLAllocHandle(SQL_HANDLE_DESC)
  2. App attaches it to a statement via SQLSetStmtAttr(SQL_ATTR_APP_ROW_DESC, hdesc)
  3. stmt->ard now points to the allocated descriptor
  4. App frees the descriptor with SQLFreeHandle(SQL_HANDLE_DESC)
  5. stmt->ard still points to freed memory
  6. Any subsequent access via SC_get_ARDF(stmt) → heap-use-after-free
  
  The Fix
  
  A new CC_detach_descriptor() function iterates all statements on the connection and resets any ard/apd pointer
  matching the descriptor being freed back to the statement's implicit (embedded) descriptor (stmt->ardi /
  stmt->apdi).
  
  Analysis
  
  Correctness:
  
  - The detach logic is correct: stmt->ard = &(stmt->ardi) and stmt->apd = &(stmt->apdi) restore the implicit
  descriptors, which is exactly what the ODBC spec requires when an explicit descriptor is freed.
  - The function is called before DC_Destructor(desc) and free(desc), so the pointer comparison is valid.
  - The embedded flag and conn pointer are saved before DC_Destructor is called. This is important because
  DC_Destructor frees internal fields. In practice DC_Destructor doesn't clear conn_conn, but saving it upfront is
  defensive and correct.
  - The !embedded guard ensures this only runs for explicitly-allocated descriptors (not the implicit ones embedded in
  statements).
  
  Thread safety:
  
  - CONNLOCK_ACQUIRE(conn) / CONNLOCK_RELEASE(conn) protects the iteration over conn->stmts[]. This is the same lock
  used elsewhere when modifying the statement list, so it's the right synchronization primitive.
  - However, there's a subtle window: between CC_detach_descriptor() returning and DC_Destructor(desc) being called,
  another thread could theoretically re-attach the same descriptor to a statement (via SQLSetStmtAttr). This is an
  application error (using a handle after freeing it), so it's not a driver bug — but worth noting.
  
  Order of operations in PGAPI_FreeDesc:
  
  The PR reorders the existing code slightly:
  
  // Before (original):
  DC_Destructor(desc);
  if (!desc->deschd.embedded) { /* remove from conn->descs[] and free */ }
  
  // After (PR):
  const char embedded = desc->deschd.embedded;
  ConnectionClass *conn = DC_get_conn(desc);
  if (!embedded) CC_detach_descriptor(conn, desc);
  DC_Destructor(desc);
  if (!embedded) { /* remove from conn->descs[] and free */ }
  
  This is correct — embedded and conn must be read before DC_Destructor potentially invalidates internal state, and
  detachment must happen while the pointer is still valid for comparison.
  
  Issues / Suggestions
  
  1. IRD/IPD not handled: The fix only detaches ard and apd. The implementation row/parameter descriptors (ird/ipd)
  are not checked. This is correct because ODBC only allows applications to explicitly allocate and attach ARD/APD —
  IRD and IPD are always implicit. No issue here, just confirming the design is intentional. ✅
  2. No expected output file for the test: The test is added to test/tests but there's no corresponding
  test/expected/descriptors-free.out file in the diff. Looking at other tests in this project, each test binary
  typically has a matching .out file. If the test harness compares stdout against expected output, this will fail. The
  test prints:
  
     freeing attached application row descriptor
     freeing attached application parameter descriptor
  
     An expected output file should be added.
  
  3. Multiple statements with same descriptor: The fix correctly iterates all statements on the connection, not just
  one. ODBC allows the same descriptor to be attached to multiple statements, so this is necessary and correct. ✅
  4. Test coverage is good: The test exercises both ARD and APD paths, verifies the handle is no longer returned by
  SQLGetStmtAttr, and then exercises a write through the descriptor (setting array/paramset size) to confirm the
  implicit descriptor is functional. The ASan verification in the PR description confirms the UAF is caught without
  the fix.
  5. Minor: const char embedded: Using char for a boolean is fine (matches the existing deschd.embedded type), but
  BOOL or int would be more idiomatic for this codebase. Very minor style nit.