Subject: [postgresql-interfaces/psqlodbc] PR #179: Handle ARD bookmark allocation failure

From: "jarvis24young (@jarvis24young)" <[email-masked]>
Date: Tue, 28 Apr 2026 07:11:53 +0000

## 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.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 29 Apr 2026 13:44:46 +0000

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.