Message-ID: <gh-postgresql-interfaces-psqlodbc-179-c4344304438@github.com>
From: "davecramer (@davecramer)" <noreply+davecramer@github.com>
To: "postgresql-interfaces/psqlodbc" <noreply+postgresql-interfaces-psqlodbc@github.com>
Date: Wed, 29 Apr 2026 13:44:46 +0000
Subject: Re: [postgresql-interfaces/psqlodbc] PR #179: Handle ARD bookmark allocation failure
In-Reply-To: <gh-postgresql-interfaces-psqlodbc-179@github.com>
References: <gh-postgresql-interfaces-psqlodbc-179@github.com>
List-Id: <gh-postgresql-interfaces-psqlodbc.github.com>
X-GitHub-Author-Login: davecramer
X-GitHub-Comment-Id: 4344304438
X-GitHub-Comment-Type: issue_comment
X-GitHub-Issue: 179
X-GitHub-Repo: postgresql-interfaces/psqlodbc
X-GitHub-Type: comment
X-GitHub-Url: https://github.com/postgresql-interfaces/psqlodbc/pull/179#issuecomment-4344304438
Content-Type: text/plain; charset=utf-8

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.