Message-ID: <gh-postgresql-interfaces-psqlodbc-178-c4334474196@github.com>
From: "davecramer (@davecramer)" <noreply+davecramer@github.com>
To: "postgresql-interfaces/psqlodbc" <noreply+postgresql-interfaces-psqlodbc@github.com>
Date: Tue, 28 Apr 2026 10:41:11 +0000
Subject: Re: [postgresql-interfaces/psqlodbc] PR #178: Reject overlong cursor names
In-Reply-To: <gh-postgresql-interfaces-psqlodbc-178@github.com>
References: <gh-postgresql-interfaces-psqlodbc-178@github.com>
List-Id: <gh-postgresql-interfaces-psqlodbc.github.com>
X-GitHub-Author-Login: davecramer
X-GitHub-Comment-Id: 4334474196
X-GitHub-Comment-Type: issue_comment
X-GitHub-Issue: 178
X-GitHub-Repo: postgresql-interfaces/psqlodbc
X-GitHub-Type: comment
X-GitHub-Url: https://github.com/postgresql-interfaces/psqlodbc/pull/178#issuecomment-4334474196
Content-Type: text/plain; charset=utf-8

Summary
  
  This PR adds length validation to PGAPI_SetCursorName() so it rejects cursor names longer than
  MAX_CURSOR_LEN (32), which is the value the driver advertises via
  SQLGetInfo(SQL_MAX_CURSOR_NAME_LEN). Previously, the driver accepted arbitrarily long names despite
  advertising a 32-character limit — an ODBC conformance gap.
  
  The Problem
  
  The ODBC spec says that if a driver reports SQL_MAX_CURSOR_NAME_LEN = 32, applications can rely on
  that as the maximum. But PGAPI_SetCursorName() blindly accepted any length via make_string() and
  stored it. This creates a contract violation: an application that queries the limit and trusts it
  could be surprised by behavior differences, and overlong names could cause issues when sent to
  PostgreSQL (which has its own NAMEDATALEN limit, typically 63).
  
  The Fix
  
  cursor_name = make_string(szCursor, cbCursor, NULL, 0);
  if (!cursor_name)
  {
      SC_set_error(stmt, STMT_NO_MEMORY_ERROR, "No memory available to store cursor name", func);
      return SQL_ERROR;
  }
  if (strlen(cursor_name) > MAX_CURSOR_LEN)
  {
      free(cursor_name);
      SC_set_error(stmt, STMT_INVALID_CURSOR_NAME, "Cursor name exceeds SQL_MAX_CURSOR_NAME_LEN",
  func);
      return SQL_ERROR;
  }
  
  NULL_THE_NAME(stmt->cursor_name);
  SET_NAME_DIRECTLY(stmt->cursor_name, cursor_name);
  
  Assessment
  
  Correctness: ✅ Good
  
  The approach is sound: materialize the name first, validate, then commit. This avoids modifying
  statement state on failure. The error codes are appropriate — STMT_NO_MEMORY_ERROR for allocation
  failure, STMT_INVALID_CURSOR_NAME for the length violation. Looking at the ODBC spec,
  SQLSetCursorName should return SQLSTATE 34000 for invalid cursor names, and
  STMT_INVALID_CURSOR_NAME maps to that in psqlodbc's error handling.
  
  Atomicity: ✅ Good
  
  The old code did NULL_THE_NAME then SET_NAME_DIRECTLY(make_string(...)) — if make_string returned
  NULL (OOM), the cursor name would be silently nulled out. The new code validates everything before
  touching stmt->cursor_name. This is a nice improvement in error handling robustness.
  
  Test: ✅ Appropriate
  
  The test:
  
  1. Queries SQL_MAX_CURSOR_NAME_LEN to get the actual limit (doesn't hardcode 32)
  2. Creates a 63-byte cursor name (well over the 32 limit)
  3. Asserts SQLSetCursorName fails
  4. Continues with the rest of the existing test (setting a valid cursor name)
  
  This is well-integrated into the existing cursor-name-test rather than creating a separate test
  file.
  
  Issues
  
  1. Off-by-one question (non-blocking): > vs >=
  
  The check is strlen(cursor_name) > MAX_CURSOR_LEN. This means a name of exactly 32 characters is
  accepted. The ODBC spec says SQL_MAX_CURSOR_NAME_LEN is "the maximum length of a cursor name in the
  data source" — so 32 characters should be valid, and 33+ should be rejected. The > comparison is
  correct.
  
  2. The make_string call allocates before validating (minor)
  
  The function allocates a potentially large string just to measure its length and reject it. For a
  cursor name this is negligible, but a pedantic approach would check the length from
  szCursor/cbCursor before allocating. However, make_string handles SQL_NTS and other length
  conventions, so replicating that logic would be error-prone. The current approach is the right
  tradeoff — simple and correct.
  
  3. Consider: should the test also verify the error state?
  
  The test checks that SQLSetCursorName returns a failure code, but doesn't verify the SQLSTATE
  (34000) or error message. This would make the test more precise, but it's not critical — the
  important thing is that the overlong name is rejected.
  
  4. SQLFreeStmt(hstmt, SQL_CLOSE) after the rejection
  
  The test calls SQLFreeStmt(hstmt, SQL_CLOSE) after the rejection test. This is fine — it clears any
  error state on the statement handle before proceeding with the valid cursor name test. Good
  practice.