postgresql-interfaces/psqlodbc GitHub issues and pull requests (mirror)  

help / color / mirror / Atom feed

From: davecramer (@davecramer) <[email protected]>
To: postgresql-interfaces/psqlodbc <[email protected]>
Subject: Re: [postgresql-interfaces/psqlodbc] PR #175: Validate percent escapes before decoding connection-string values
Date: Thu, 23 Apr 2026 11:07:38 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>

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].  

view thread (6+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: github://postgresql-interfaces/psqlodbc
  Cc: [email protected], [email protected]
  Subject: Re: [postgresql-interfaces/psqlodbc] PR #175: Validate percent escapes before decoding connection-string values
  In-Reply-To: <<[email protected]>>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox