postgresql-interfaces/psqlodbc GitHub issues and pull requests (mirror)
help / color / mirror / Atom feed[postgresql-interfaces/psqlodbc] PR #183: Detach freed app descriptors from statements
4+ messages / 2 participants
[nested] [flat]
* [postgresql-interfaces/psqlodbc] PR #183: Detach freed app descriptors from statements
@ 2026-05-07 02:00 "jarvis24young (@jarvis24young)" <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: jarvis24young (@jarvis24young) @ 2026-05-07 02:00 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
Fix a dangling application descriptor reference left on statements after
SQLFreeHandle(SQL_HANDLE_DESC).
When an application descriptor is attached through SQL_ATTR_APP_ROW_DESC or
SQL_ATTR_APP_PARAM_DESC, the statement stores the descriptor pointer directly.
PGAPI_FreeDesc() removed and freed the descriptor from the connection descriptor
list, but did not reset statements still pointing at it. A later statement
attribute write through SC_get_ARDF()/SC_get_APDF() can therefore access freed
memory.
The fix detaches a non-embedded descriptor from all statements on the same
connection before destroying it. Statements are reset to their implicit ARD/APD.
The new regression test covers both descriptor classes:
- attach an application row descriptor, free it, then set SQL_ATTR_ROW_ARRAY_SIZE
- attach an application parameter descriptor, free it, then set SQL_ATTR_PARAMSET_SIZE
- verify SQLGetStmtAttr no longer returns the freed descriptor handle
Verification performed in WSL with ASan/UBSan:
```text
# vulnerable build, minimal public-API reproducer
ERROR: AddressSanitizer: heap-use-after-free
WRITE of size 8
#0 PGAPI_SetStmtAttr pgapi30.c:2491
#1 SQLSetStmtAttr odbcapi30.c:698
freed by:
#1 PGAPI_FreeDesc descriptor.c:442
#2 SQLFreeHandle odbcapi30.c:291
```
After this patch, from a clean clone of this branch:
```text
./bootstrap
./configure --with-unixodbc=__without_odbc_config \
CFLAGS='-O1 -g -fno-omit-frame-pointer -fsanitize=address,undefined -I/usr/include/postgresql -DSQLCOLATTRIBUTE_SQLLEN -Wall' \
LDFLAGS='-fsanitize=address,undefined'
make -j2
cd test
make exe/descriptors-free-test LIBODBC=-lodbc
ASAN_OPTIONS=detect_leaks=0:halt_on_error=1:abort_on_error=1 \
UBSAN_OPTIONS=halt_on_error=1 \
ODBCSYSINI=. ODBCINSTINI=./odbcinst.ini ODBCINI=./odbc.ini \
./exe/descriptors-free-test
```
Result:
```text
connected
freeing attached application row descriptor
freeing attached application parameter descriptor
disconnecting
```
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #183: Detach freed app descriptors from statements
@ 2026-05-08 10:16 ` "davecramer (@davecramer)" <[email protected]>
2 siblings, 0 replies; 4+ messages in thread
From: davecramer (@davecramer) @ 2026-05-08 10:16 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
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.
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #183: Detach freed app descriptors from statements
@ 2026-05-09 01:17 ` "jarvis24young (@jarvis24young)" <[email protected]>
2 siblings, 0 replies; 4+ messages in thread
From: jarvis24young (@jarvis24young) @ 2026-05-09 01:17 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
Thanks for the review.
The core detach logic is unchanged.
The Linux CI failure was caused by a missing expected output file for the new regression test. I have now added `test/expected/descriptors-free.out`, which matches the test stdout and lets `runsuite` execute the case instead of bailing out before comparison.
This keeps the patch focused on the lifetime fix in `PGAPI_FreeDesc()` / `CC_detach_descriptor()` and addresses the blocking issue raised by the pipeline.
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #183: Detach freed app descriptors from statements
@ 2026-05-09 02:37 ` "jarvis24young (@jarvis24young)" <[email protected]>
2 siblings, 0 replies; 4+ messages in thread
From: jarvis24young (@jarvis24young) @ 2026-05-09 02:37 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
The missing expected output file has been added. The new checks currently show action_required, so they may need maintainer approval to run for this fork PR.
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-05-09 02:37 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-05-07 02:00 [postgresql-interfaces/psqlodbc] PR #183: Detach freed app descriptors from statements "jarvis24young (@jarvis24young)" <[email protected]>
2026-05-08 10:16 ` "davecramer (@davecramer)" <[email protected]>
2026-05-09 01:17 ` "jarvis24young (@jarvis24young)" <[email protected]>
2026-05-09 02:37 ` "jarvis24young (@jarvis24young)" <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox