Message-ID: <gh-postgresql-interfaces-psqlodbc-185-c4439528574@github.com>
From: "davecramer (@davecramer)" <noreply+davecramer@github.com>
To: "postgresql-interfaces/psqlodbc" <noreply+postgresql-interfaces-psqlodbc@github.com>
Date: Wed, 13 May 2026 09:39:29 +0000
Subject: Re: [postgresql-interfaces/psqlodbc] PR #185: Avoid reading before leading string literals
In-Reply-To: <gh-postgresql-interfaces-psqlodbc-185@github.com>
References: <gh-postgresql-interfaces-psqlodbc-185@github.com>
List-Id: <gh-postgresql-interfaces-psqlodbc.github.com>
X-GitHub-Author-Login: davecramer
X-GitHub-Comment-Id: 4439528574
X-GitHub-Comment-Type: issue_comment
X-GitHub-Issue: 185
X-GitHub-Repo: postgresql-interfaces/psqlodbc
X-GitHub-Type: comment
X-GitHub-Url: https://github.com/postgresql-interfaces/psqlodbc/pull/185#issuecomment-4439528574
Content-Type: text/plain; charset=utf-8

 Summary
  
  Fixes a heap-buffer-overflow (1-byte read before buffer start) when the SQL string begins with a string literal
  (e.g., 'abc') and standard_conforming_strings is on (CC_get_escape() returns 0). The scanner checks for a preceding
  E character (PostgreSQL's E'...' escape string syntax) by reading ptr[-1], but when the literal is at position 0,
  that's a read before the buffer.
  
  The Bug
  
  Three independent scanner paths have the same pattern:
  
  if (LITERAL_EXT == ptr[-1])  // reads before buffer when at position 0
  
  Where ptr is:
  
  - ENCODE_PTR(encstr) = encstr.encstr + encstr.pos in statement.c
  - F_OldPtr(qp) = qp->statement + qp->opos in convert.c
  - tstr (advancing pointer) in parse.c
  
  The Fix
  
  Each site adds a bounds check before the [-1] access:
  
  ┌─────────────┬─────────────────────────────┐
  │ File        │ Guard added                 │
  ├─────────────┼─────────────────────────────┤
  │ statement.c │ encstr.pos > 0 &&           │
  ├─────────────┼─────────────────────────────┤
  │ convert.c   │ qp->opos > 0 &&             │
  ├─────────────┼─────────────────────────────┤
  │ parse.c     │ tstr > (const UCHAR *) s && │
  └─────────────┴─────────────────────────────┘
  
  Short-circuit evaluation ensures [-1] is never reached when at position 0.
  
  Assessment
  
  Correctness: ✅
  
  - All three guards are correct for their respective pointer arithmetic.
  - encstr.pos > 0 directly maps to ENCODE_PTR(encstr)[-1] being valid since ENCODE_PTR is encstr + pos.
  - qp->opos > 0 directly maps to F_OldPtr(qp)[-1] being valid since F_OldPtr is statement + opos.
  - tstr > (const UCHAR *) s is the pointer-based equivalent for parse.c where s is the start of the string and tstr
   advances through it.
  - When at position 0, the E'' prefix check is correctly skipped — a literal at position 0 can't have a preceding E
   anyway.
  
  Completeness: ✅
  
  - All three scanner paths that have this pattern are fixed. I checked — there are no other LITERAL_EXT == ...[-1]
   patterns in the codebase.
  
  Test: ✅
  
  - Minimal reproducer: SQLPrepare(hstmt, "'abc'", SQL_NTS) followed by SQLNumParams().
  - Expected output file included.
  - Test registered in test/tests.
  - ASan verification documented in the PR description.
  
  Risk: Very low
  
  - The fix only adds a guard that skips an impossible condition (can't have E before position 0). No behavioral
  change for any valid input.
  
  Verdict
  
  Textbook bounds-check fix. Correct, minimal, well-tested across all three affected sites. Merge-worthy as-is.