Subject: [postgresql-interfaces/psqlodbc] PR #175: Validate percent escapes before decoding connection-string values

From: "jarvis24young (@jarvis24young)" <[email-masked]>
Date: Thu, 23 Apr 2026 09:07:37 +0000

This fixes sanitizer-detected undefined behavior in connection-string percent decoding.

Root cause:
- `decode()` treated every `%` in a connection-string value as the beginning of a `%xx` percent escape.
- It called `conv_from_hex(&in[i])` without first checking that two following hex digits were present.
- For a truncated value such as `pqopt=application_name=%`, `conv_from_hex()` reads the string terminator, computes a negative value, and left-shifts it. UBSan reports this as undefined behavior.

Fix:
- Decode only valid `%xx` escapes where both following characters are hex digits.
- Leave malformed or truncated percent sequences unchanged, preserving compatibility while avoiding undefined behavior.

Regression test:
- Adds `percent-decode-test`, which exercises the public `SQLDriverConnect()` path with `pqopt=application_name=%`, `%A`, `%G1`, and `%20`.
- The test requires `SQLDriverConnect()` to succeed for each case, so it verifies more than just "no crash".

Verification performed under ASan/UBSan:

```bash
cd ~/psqlodbc-build/test
export LD_PRELOAD=/usr/lib/gcc/x86_64-linux-gnu/13/libasan.so
ODBCSYSINI=. ODBCINSTINI=./odbcinst.ini ODBCINI=./odbc.ini \
ASAN_OPTIONS=halt_on_error=1:abort_on_error=1 \
UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1 \
./exe/percent-decode-test
```

Output:

```text
truncated percent: ok
one hex digit: ok
non-hex escape: ok
valid percent escape: ok
```

---

From: "jarvis24young (@jarvis24young)" <[email-masked]>
Date: Thu, 23 Apr 2026 09:18:52 +0000

I updated the regression test to make it stronger.

Instead of using `PWD=...` and accepting an authentication failure, the test now uses `pqopt=application_name=...`. That still exercises the same `decode_or_remove_braces()` / `decode()` path for connection-string percent decoding, but it does not override the DSN password, so `SQLDriverConnect()` is expected to succeed for each case.

For reference, before this patch the truncated percent case triggers UBSan in my local ASan/UBSan build:

```text
dlg_specific.c:1584:12: runtime error: left shift of negative value -48
    #0 conv_from_hex /home/yjw/psqlodbc-build/dlg_specific.c:1584
    #1 decode /home/yjw/psqlodbc-build/dlg_specific.c:1612
    #2 decode_or_remove_braces /home/yjw/psqlodbc-build/dlg_specific.c:1658
    #3 copyConnAttributes /home/yjw/psqlodbc-build/dlg_specific.c:639
    #4 dconn_get_attributes /home/yjw/psqlodbc-build/drvconn.c:577
    #5 PGAPI_DriverConnect /home/yjw/psqlodbc-build/drvconn.c:157
    #6 SQLDriverConnect /home/yjw/psqlodbc-build/odbcapi.c:213
```

The `ok` output in the committed regression test represents the fixed behavior: malformed percent escapes no longer reach `conv_from_hex()`, and the connection path completes successfully.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 23 Apr 2026 11:07:38 +0000

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

---

From: "jarvis24young (@jarvis24young)" <[email-masked]>
Date: Thu, 23 Apr 2026 11:51:27 +0000

Thanks for the review.

I addressed the minor suggestion by adding a precondition comment to `conv_from_hex()`:

```c
/* Callers must ensure that s[1] and s[2] are hex digits. */
```

I kept the implementation scoped to the current fix rather than changing `conv_from_hex()` behavior directly, since `decode()` now validates before calling it.

I also checked the `test/tests` context against the current upstream `main`; `interval-overflow-test` is already present there after #174, so the appended `percent-decode-test` is now based on the latest main branch.

---

From: "jarvis24young (@jarvis24young)" <[email-masked]>
Date: Mon, 27 Apr 2026 12:17:41 +0000

@davecramer   Hi, I rebased this PR onto the latest main and resolved the conflict in test/tests.
  The PR is now mergeable, but the new GitHub Actions runs are waiting for maintainer approval because they show
  action_required. Could you please approve/run the checks when you have a chance? Thanks.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 28 Apr 2026 10:33:54 +0000

Summary
  
  This PR fixes undefined behavior in decode() in dlg_specific.c. When a connection-string value
  contains a malformed percent escape (e.g., %, %A, %G1), conv_from_hex() reads past the valid data
  and performs a left-shift of a negative value — UB caught by UBSan. The fix validates that two hex
  digits follow % before calling conv_from_hex(), and passes malformed sequences through as literals.
  
  The Bug
  
  In the existing decode():
  
  else if (inc == '%')
  {
      snprintf(&outs[o], ilen + 1 - o, "%c", conv_from_hex(&in[i]));
      o++;
      i += 2;
  }
  
  This unconditionally calls conv_from_hex(&in[i]), which reads s[1] and s[2] relative to the %. If
  the string is % at the end, s[1] is '\0' and s[2] is past the allocation. conv_from_hex computes
  val = '\0' - '0' = -48, then does y += val << 4 — left-shifting a negative value is undefined
  behavior per C11 §6.5.7.
  
  The Fix
  
  Three changes:
  
  1. New validation function is_valid_percent_encoding():
  
  static BOOL
  is_valid_percent_encoding(const char *s, size_t len)
  {
      if (len < 3)
          return FALSE;
      return isxdigit((unsigned char) s[1]) &&
             isxdigit((unsigned char) s[2]);
  }
  
  2. Guarded decode path — only calls conv_from_hex when valid; otherwise passes % through as a
  literal character.
  3. Precondition comment on conv_from_hex(): /* Callers must ensure that s[1] and s[2] are hex
  digits. */
  
  Assessment
  
  Correctness: ✅ Good
  
  The len < 3 check in is_valid_percent_encoding correctly handles both truncated (%, %A) and
  end-of-string cases by passing ilen - i as the remaining length. The isxdigit calls with (unsigned
  char) casts are correct — this avoids UB from negative char values on platforms where char is
  signed. The fallthrough to outs[o++] = inc (copying the literal %) is the right behavior for
  malformed escapes.
  
  Semantic change: ✅ Acknowledged and acceptable
  
  Previously %G1 would produce a garbage byte (from the UB computation). Now it produces the literal
  string %G1. This is strictly better — no real-world user depends on garbage output from UB. The PR
  description explicitly calls this out.
  
  Test: ✅ Well-designed
  
  The test covers four cases:
  
  - % — truncated at end of string
  - %A — only one hex digit
  - %G1 — non-hex character
  - %20 — valid escape (space)
  
  Each case goes through the full SQLDriverConnect() path and asserts success, not just
  crash-freedom. The test uses pqopt=application_name=... which exercises decode_or_remove_braces() →
  decode() without interfering with authentication. This is a smart test design choice.
  
  conv_from_hex still fragile: ⚠️ Noted but acceptable
  
  The precondition comment is the right minimal approach for this PR. A defensive isxdigit check
  inside conv_from_hex itself would be belt-and-suspenders, but it would also change the return type
  semantics (what does it return on invalid input?). The comment is sufficient — conv_from_hex is
  static and only called from decode(), so the blast radius of a future misuse is limited to this
  file.
  
  Issues found
  
  One minor concern with i += 2 on the valid path:
  
  In the valid branch:
  
  if (is_valid_percent_encoding(&in[i], ilen - i))
  {
      snprintf(&outs[o], ilen + 1 - o, "%c", conv_from_hex(&in[i]));
      o++;
      i += 2;
  }
  
  The i += 2 skips past the two hex digits, and then the for loop does i++, so i advances by 3 total
  (past %XX). This is the same logic as the original code and is correct. No issue here.
  
  The snprintf for a single byte is wasteful (as noted by davecramer in the PR comments), but it's
  pre-existing code and out of scope for this fix. Agreed — not a blocker.
  
  Verdict
  
  Approve. This is a clean, well-scoped fix for real undefined behavior with a thoughtful regression
  test. The contributor responded well to review feedback (added the precondition comment, rebased
  onto latest main). The test/tests file now correctly appends after conn-settings-test from PR
  #176's branch. Ready to merge once CI passes.