postgresql-interfaces/psqlodbc GitHub issues and pull requests (mirror)
help / color / mirror / Atom feed[postgresql-interfaces/psqlodbc] PR #178: Reject overlong cursor names
3+ messages / 2 participants
[nested] [flat]
* [postgresql-interfaces/psqlodbc] PR #178: Reject overlong cursor names
@ 2026-04-28 03:26 "jarvis24young (@jarvis24young)" <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: jarvis24young (@jarvis24young) @ 2026-04-28 03:26 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
## Summary
This patch makes `SQLSetCursorName()` reject cursor names longer than the length advertised by the driver through `SQLGetInfo(SQL_MAX_CURSOR_NAME_LEN)`.
The driver reports `MAX_CURSOR_LEN` (`32`) for `SQL_MAX_CURSOR_NAME_LEN`, but `PGAPI_SetCursorName()` previously accepted and stored longer application-provided cursor names. The new check validates the fully materialized cursor name before replacing the statement's current cursor name.
## Details
`PGAPI_SetCursorName()` now:
- builds the requested cursor name into a temporary buffer first;
- reports `STMT_NO_MEMORY_ERROR` if allocation fails;
- rejects names longer than `MAX_CURSOR_LEN` with `STMT_INVALID_CURSOR_NAME`;
- only replaces `stmt->cursor_name` after allocation and validation both succeed.
This keeps the statement state unchanged on failure and makes `SQLSetCursorName()` consistent with the driver's advertised ODBC cursor-name limit.
## Regression test
The existing `cursor-name` black-box ODBC test now:
- queries `SQL_MAX_CURSOR_NAME_LEN` from the connected driver;
- builds an invalid cursor name with length `SQL_MAX_CURSOR_NAME_LEN + 1`;
- verifies that `SQLSetCursorName()` rejects it;
- verifies the diagnostic SQLSTATE is `34000`;
- then continues through the existing valid cursor-name path.
Verified in WSL against the unixODBC test path with an ASan/UBSan/gcov build:
```sh
cd ~/psqlodbc-build
make -j4
cd test
make LIBODBC='-lodbc' exe/cursor-name-test
ODBCSYSINI=. ODBCINSTINI=./odbcinst.ini ODBCINI=./odbc.ini \
ASAN_OPTIONS=verify_asan_link_order=0:detect_leaks=0 \
LD_PRELOAD=/lib/x86_64-linux-gnu/libasan.so.8 \
./runsuite cursor-name --inputdir=.
```
Result:
```text
TAP version 13
1..1
ok 1 - cursor-name
```
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #178: Reject overlong cursor names
@ 2026-04-28 10:41 ` "davecramer (@davecramer)" <[email protected]>
1 sibling, 0 replies; 3+ messages in thread
From: davecramer (@davecramer) @ 2026-04-28 10:41 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
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.
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #178: Reject overlong cursor names
@ 2026-04-28 11:41 ` "jarvis24young (@jarvis24young)" <[email protected]>
1 sibling, 0 replies; 3+ messages in thread
From: jarvis24young (@jarvis24young) @ 2026-04-28 11:41 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
Thanks for the review. I updated the regression test to also verify the diagnostic state for the rejected overlong cursor name. The test now builds the invalid name as SQL_MAX_CURSOR_NAME_LEN + 1, calls SQLGetDiagRec() after SQLSetCursorName() fails, and requires SQLSTATE 34000 before continuing with the existing valid cursor-name path.
Re-tested in WSL with the unixODBC black-box path and ASan/UBSan/gcov build:
```text
TAP version 13
1..1
ok 1 - cursor-name
```
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-04-28 11:41 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-04-28 03:26 [postgresql-interfaces/psqlodbc] PR #178: Reject overlong cursor names "jarvis24young (@jarvis24young)" <[email protected]>
2026-04-28 10:41 ` "davecramer (@davecramer)" <[email protected]>
2026-04-28 11:41 ` "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