postgresql-interfaces/psqlodbc GitHub issues and pull requests (mirror)
help / color / mirror / Atom feed[postgresql-interfaces/psqlodbc] PR #179: Handle ARD bookmark allocation failure
2+ messages / 2 participants
[nested] [flat]
* [postgresql-interfaces/psqlodbc] PR #179: Handle ARD bookmark allocation failure
@ 2026-04-28 07:11 "jarvis24young (@jarvis24young)" <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: jarvis24young (@jarvis24young) @ 2026-04-28 07:11 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
## Summary
This hardens ARD bookmark allocation so low-memory failures are reported as ODBC errors instead of dereferencing a NULL pointer.
`ARD_AllocBookmark()` previously called `malloc(sizeof(BindInfoClass))` and immediately cleared the returned pointer. If the allocation failed, statement allocation or bookmark binding could crash the host process.
The patch:
- returns `NULL` from `ARD_AllocBookmark()` when bookmark allocation fails;
- handles that failure in `PGAPI_AllocStmt()` by removing the partially registered statement, destroying it, clearing the output handle, and returning `SQL_ERROR` with the existing statement allocation diagnostic;
- handles the same failure in `SQLBindCol()` bookmark-column binding with `STMT_NO_MEMORY_ERROR`;
- handles ARD bookmark descriptor field updates with `DESC_NO_MEMORY_ERROR`;
- adds a black-box ODBC regression test for the normal statement-allocation path that initializes the ARD bookmark.
## Validation
Built and tested in WSL against the real driver through unixODBC.
Default regression checks:
```sh
cd ~/psqlodbc-current-oom/test
ODBCSYSINI=. ODBCINSTINI=./odbcinst.ini ODBCINI=./odbc.ini \
./runsuite ard-bookmark-oom bookmark bulkoperations --inputdir=.
```
Result:
```text
ok 1 - ard-bookmark-oom
ok 2 - bookmark
ok 3 - bulkoperations
```
Manual OOM fault-injection validation with the real driver:
```sh
PODBC_FAIL_MALLOC_SIZE=40 PODBC_FAIL_MALLOC_N=178 \
LD_PRELOAD=./exe/malloc_fail_shim.so \
ODBCSYSINI=. ODBCINSTINI=./odbcinst.ini ODBCINI=./odbc.ini \
./exe/ard-bookmark-oom-bugwatch-test
```
Before the fix this produced `Segmentation fault (core dumped)` at `descriptor.c:345` via `statement.c:232`.
After the fix it returns a normal ODBC allocation error without crashing:
```text
SQLAllocHandle stmt rc=-1 hstmt=(nil)
HY001=No more memory to allocate a further SQL-statement
statement allocation returned an error without crashing
```
The LD_PRELOAD fault-injection harness is not registered in the portable regression suite; the committed regression test covers the public ODBC statement-allocation path under normal conditions.
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #179: Handle ARD bookmark allocation failure
@ 2026-04-29 13:44 "davecramer (@davecramer)" <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: davecramer (@davecramer) @ 2026-04-29 13:44 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
AI review
Summary
This PR adds NULL-check handling for ARD_AllocBookmark() across all its call sites. Previously, if
malloc failed inside ARD_AllocBookmark(), the code would dereference NULL via
pg_memset(ardopts->bookmark, 0, ...), crashing the process. The fix propagates the failure as
proper ODBC errors.
The Bug
In the existing ARD_AllocBookmark():
BindInfoClass *ARD_AllocBookmark(ARDFields *ardopts)
{
if (!ardopts->bookmark) {
ardopts->bookmark = (BindInfoClass *) malloc(sizeof(BindInfoClass));
pg_memset(ardopts->bookmark, 0, sizeof(BindInfoClass)); // NULL deref if malloc fails
}
return ardopts->bookmark;
}
If malloc returns NULL, pg_memset dereferences it — instant crash. This is called from:
1. PGAPI_AllocStmt() — every statement allocation
2. PGAPI_BindCol() (bind.c) — bookmark column binding
3. ARDSetField() (pgapi30.c) — descriptor field updates
4. ARDFields_copy() — descriptor copy (already handled NULL)
The Fix
descriptor.c — Core fix:
ardopts->bookmark = (BindInfoClass *) malloc(sizeof(BindInfoClass));
+if (!ardopts->bookmark)
+ return NULL;
pg_memset(ardopts->bookmark, 0, sizeof(BindInfoClass));
statement.c — PGAPI_AllocStmt caller:
-ARD_AllocBookmark(ardopts);
+if (!ARD_AllocBookmark(ardopts))
+{
+ CC_set_error(conn, CONN_STMT_ALLOC_ERROR, "No more memory...", func);
+ CC_remove_statement(conn, stmt);
+ SC_Destructor(stmt);
+ *phstmt = SQL_NULL_HSTMT;
+ return SQL_ERROR;
+}
bind.c — SQLBindCol bookmark path:
bookmark = ARD_AllocBookmark(opts);
+if (!bookmark)
+{
+ SC_set_error(stmt, STMT_NO_MEMORY_ERROR, "Could not allocate memory for bookmark binding.",
func);
+ ret = SQL_ERROR;
+ goto cleanup;
+}
pgapi30.c — ARDSetField descriptor path:
BindInfoClass *bookmark = ARD_AllocBookmark(opts);
+if (!bookmark)
+{
+ DC_set_error(desc, DESC_NO_MEMORY_ERROR, "Could not allocate memory for bookmark descriptor");
+ return SQL_ERROR;
+}
Assessment
Correctness: ✅ Good
Each call site uses the appropriate error-reporting mechanism for its context:
- PGAPI_AllocStmt → CC_set_error on the connection (correct, since the statement is being
- PGAPI_BindCol → SC_set_error on the statement + goto cleanup (follows existing pattern in that
function)
- ARDSetField → DC_set_error on the descriptor (correct for descriptor operations)
Cleanup in PGAPI_AllocStmt: ✅ Thorough
The failure path does CC_remove_statement → SC_Destructor → clear *phstmt. This is the right
sequence — the statement was already added to the connection via CC_add_statement earlier in the
function, so it must be removed before destruction. This matches the pattern used for the
CC_add_statement failure case just above it in the existing code.
ARDFields_copy already handled: ✅ Noted
Looking at the existing code in ARDFields_copy():
BindInfoClass *bookmark = ARD_AllocBookmark(target);
if (bookmark)
BindInfoClass_copy(src->bookmark, bookmark);
This already silently handles NULL — it just skips the copy. The PR doesn't change this, which is
correct since PGAPI_CopyDesc doesn't have a clean way to propagate the error (it would need more
refactoring).
Test: ✅ Pragmatic
The test exercises the normal SQLAllocHandle(SQL_HANDLE_STMT) path that triggers ARD_AllocBookmark.
It can't deterministically trigger OOM in a portable way, so it validates the happy path. The PR
description documents manual fault-injection testing with LD_PRELOAD that confirmed the crash
before and the clean error after. This is a reasonable approach — the code change is obviously
correct from inspection, and the fault injection provides the empirical evidence.
Issues
1. Minor: DESC_NO_MEMORY_ERROR SQLSTATE mapping
Looking at the descriptor error table, DESC_NO_MEMORY_ERROR maps to SQLSTATE HY001 (memory
allocation error). This is the correct SQLSTATE for an OOM condition per the ODBC spec. Good.
2. The ARDFields_copy silent failure is a latent issue (not introduced by this PR)
If ARD_AllocBookmark returns NULL during PGAPI_CopyDesc, the bookmark is silently lost. This is
pre-existing behavior (the code already checked for NULL) and not made worse by this PR. A
follow-up could propagate the error, but it's out of scope here.
3. Test name ard-bookmark-oom is slightly misleading
The committed test doesn't actually test OOM — it tests the normal allocation path. The name
suggests it's an OOM test. Something like ard-bookmark-alloc would be more accurate. But this is
cosmetic and the test file comments explain the situation clearly.
4. Test placement in test/tests
The test is inserted after exe/bookmark-test in the test list, which is a logical grouping. Good.
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2026-04-29 13:44 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-04-28 07:11 [postgresql-interfaces/psqlodbc] PR #179: Handle ARD bookmark allocation failure "jarvis24young (@jarvis24young)" <[email protected]>
2026-04-29 13:44 Re: [postgresql-interfaces/psqlodbc] PR #179: Handle ARD bookmark allocation failure "davecramer (@davecramer)" <[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