Message-ID: <gh-postgresql-interfaces-psqlodbc-175-c4303875247@github.com>
From: "davecramer (@davecramer)" <noreply+davecramer@github.com>
To: "postgresql-interfaces/psqlodbc" <noreply+postgresql-interfaces-psqlodbc@github.com>
Date: Thu, 23 Apr 2026 11:07:38 +0000
Subject: Re: [postgresql-interfaces/psqlodbc] PR #175: Validate percent escapes before decoding connection-string values
In-Reply-To: <gh-postgresql-interfaces-psqlodbc-175@github.com>
References: <gh-postgresql-interfaces-psqlodbc-175@github.com>
List-Id: <gh-postgresql-interfaces-psqlodbc.github.com>
X-GitHub-Author-Login: davecramer
X-GitHub-Comment-Id: 4303875247
X-GitHub-Comment-Type: issue_comment
X-GitHub-Issue: 175
X-GitHub-Repo: postgresql-interfaces/psqlodbc
X-GitHub-Type: comment
X-GitHub-Url: https://github.com/postgresql-interfaces/psqlodbc/pull/175#issuecomment-4303875247
Content-Type: text/plain; charset=utf-8

1. `conv_from_hex()` itself is still fragile — the validation is only in decode(), but             
  conv_from_hex() is a separate function that could be called from elsewhere in the future without   
  the guard. Consider adding a defensive check inside conv_from_hex() itself, or at minimum adding a 
  comment that callers must validate input first. This isn't a blocker for this PR, but worth noting.
  2. The `else` branch silently passes `%` through as a literal — this is the right behavior for     
  robustness, but it's a semantic change. Previously, %G1 would produce a garbage character; now it  
  produces literal %G1. This is strictly better, but the PR description should explicitly call out   
  this behavior change for anyone relying on the old (broken) behavior. The PR description does      
  mention this — good.                                                                               
  3. Missing `test/tests` in the diff context — the diff shows the test is appended after            
  exe/interval-overflow-test, which is from our local branch. On main, the last entry is             
  exe/primarykeys-include-test. This will cause a merge conflict if our interval-overflow change     
  lands first, but that's a trivial conflict to resolve.                                             
  4. Minor: `snprintf` for a single character — the existing code uses snprintf(&outs[o], ilen + 1 - 
  o, "%c", conv_from_hex(&in[i])) to write one byte. A simple outs[o] = (char) conv_from_hex(&in[i]) 
  would be clearer and avoid the format string overhead. But this is pre-existing code, not          
  introduced by this PR.                                                                             
                                                                                                     
  Verdict                                                                                            
                                                                                                     
  This is a clean, well-scoped fix for a real UB issue. The test is properly integrated and verifies 
  actual connection success rather than just crash-freedom. Ready to merge with one minor suggestion:
  add a comment on conv_from_hex() noting that callers must ensure two hex digits follow s[0].