On Mon, Jul 06, 2026 at 02:31:06PM -0700, Jeff Davis wrote:
> On Mon, 2026-06-29 at 18:29 -0700, Noah Misch wrote:
> > I've copied Claude's whole
> > review for the commit to
> > https://docs.google.com/document/d/19_xKpKUQlY5N51qbfXyipXs3rqIYEC3qk5oaUDJ48YA/edit?tab=t.0
> > .
> > Two of its other findings sound potentially important, but I didn't
> > verify
> > them.
>
> I'll refer to the issue numbers below (NB: the doc's summary uses
> different numbering, which is confusing):
>
> Issue 1: Real issue for windows and code points > 0xFFFF. Other cases
> are just defensive, I believe. Fixed in 0003.
>
> Issue 2: I don't see an issue here, though sort of related to the fix
> in 0003.
>
> Issue 3: I don't see an issue here.
>
> Issue 4: The issue you raised above; fixed in 0001.
>
> Issue 5: I don't see an issue here.
>
> I also addressed summary item 4, the comment block fix, in 0002. And I
> added some missing casts in 0004.
>
> Does that cover the two other findings you were concerned about?
Yep. I didn't read the patches, but this per-issue triage is reasonable.
> In the future, if practical, can you please include reports like this
> as text attachments for the sake of the archives?
I've attached it.
I'll analyze this patch carefully looking for behavioral changes and
transcription errors. Let me examine each section systematically.Let me walk
through the key areas of concern:
## Issue 1: Loss of bounds checking in `regc_pg_locale.c` (likely a real bug)
In the original code for `pg_wc_isdigit` (and all the other `pg_wc_is*`
functions), the LIBC paths had explicit bounds checks:
**Original `LIBC_WIDE` (UTF8):**
```c
if (sizeof(wchar_t) >= 4 || c <= (pg_wchar) 0xFFFF)
return iswdigit_l((wint_t) c, pg_regex_locale->info.lt);
/* FALL THRU */
```
**Original `LIBC_1BYTE`:**
```c
return (c <= (pg_wchar) UCHAR_MAX &&
isdigit_l((unsigned char) c, pg_regex_locale->info.lt));
```
In the new code, these become unconditional calls like `wc_isdigit_libc_sb`
which just does:
```c
return isdigit_l((unsigned char) wc, locale->info.lt);
```
The cast `(unsigned char) wc` truncates `wc` silently. So if `c > UCHAR_MAX`,
the old code returned 0 (false); the new code calls `isdigit_l` on the
truncated byte, which could return true for, say, `c = 0x100 + '0'`.
**However**, this is mitigated by the `max_chr` field. For
`ctype_methods_libc_sb` and `ctype_methods_libc_other_mb`, `max_chr =
UCHAR_MAX`, and `pg_ctype_get_cache` uses this to limit the range explored. So
in regex character class caching, the new code only invokes these methods for
`c <= UCHAR_MAX`.
But the `pg_wc_is*` functions are also called directly from `pg_regc_locale.c`
outside the cache path (e.g., for testing specific characters during regex
matching). Looking at the original code's `LIBC_1BYTE` branch, it explicitly
guarded `c <= UCHAR_MAX`. The new code doesn't. **This is a behavioral
regression for single-byte LIBC**: a `pg_wchar` value above 255 will now
silently truncate and yield a possibly-wrong result rather than returning 0.
Similarly for `LIBC_WIDE` (UTF8 path): the old code had a `sizeof(wchar_t) >= 4
|| c <= 0xFFFF` guard, falling through to `LIBC_1BYTE` (with its `<= UCHAR_MAX`
check) when wchar_t is 16-bit and `c > 0xFFFF`. The new `wc_isdigit_libc_mb`
unconditionally does `iswdigit_l((wint_t) wc, ...)`, which on 16-bit-wchar_t
platforms (Windows) truncates `wc` to 16 bits. The old behavior on such
platforms was to fall through to the single-byte path, which then returned 0
for `c > UCHAR_MAX`. **New behavior on 16-bit wchar_t platforms differs from
old.**
## Issue 2: `toupper_libc_mb` / `tolower_libc_mb` on 16-bit wchar_t platforms
Original code:
```c
case PG_REGEX_STRATEGY_LIBC_WIDE:
if (pg_regex_locale->is_default && c <= 127) ...
if (sizeof(wchar_t) >= 4 || c <= 0xFFFF)
return towupper_l(...);
/* FALL THRU */
case PG_REGEX_STRATEGY_LIBC_1BYTE:
if (pg_regex_locale->is_default && c <= 127) ...
if (c <= UCHAR_MAX)
return toupper_l(...);
return c;
```
New `toupper_libc_mb`:
```c
if (sizeof(wchar_t) >= 4 || wc <= (pg_wchar) 0xFFFF)
return towupper_l((wint_t) wc, locale->info.lt);
else
return wc;
```
The fall-through on 16-bit wchar_t platforms is lost. The old code, when
`sizeof(wchar_t) < 4` and `c > 0xFFFF`, would fall through and try the
single-byte path. New code just returns `wc` unchanged. This is a behavioral
change on Windows/16-bit-wchar_t platforms for characters above 0xFFFF — but in
UTF8 the old fall-through to `toupper_l` was nearly useless anyway (since the
byte truncation is meaningless), so this is arguably a fix. Still worth
flagging.
## Issue 3: `char_is_cased_libc` — behavioral change for libc, single-byte,
high-bit set
In `like_support.c`, original code:
```c
else if (is_multibyte && IS_HIGHBIT_SET(c))
return true;
else if (locale->provider != COLLPROVIDER_LIBC)
return IS_HIGHBIT_SET(c) ||
(c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z');
else
return isalpha_l((unsigned char) c, locale->info.lt);
```
For **libc, single-byte encoding**, with high-bit-set char: original called
`isalpha_l`. New `char_is_cased_libc`:
```c
if (is_multibyte && IS_HIGHBIT_SET(ch))
return true;
else
return isalpha_l((unsigned char) ch, locale->info.lt);
```
For libc + single-byte + high-bit-set, both old and new call `isalpha_l`. ✓
Matches.
For libc + multibyte + high-bit-set: both return true. ✓ Matches.
For libc + ASCII letter: both call `isalpha_l`. ✓ Matches.
For libc + ASCII non-letter: both call `isalpha_l` → false. ✓ Matches.
OK, that one is fine.
## Issue 4: `Generic_Text_IC_like` semantic change
Original:
```c
if (pg_database_encoding_max_length() > 1 || (locale->provider ==
COLLPROVIDER_ICU))
/* call lower() then match */
else
/* SB_IMatchText fast path */
```
New:
```c
if (locale->ctype_is_c ||
(char_tolower_enabled(locale) &&
pg_database_encoding_max_length() == 1))
/* SB_IMatchText fast path */
else
/* call lower() then match */
```
Old fast-path condition: `max_length == 1 && provider != ICU` — meaning libc or
builtin with single-byte encoding.
New fast-path condition: `ctype_is_c || (char_tolower_enabled && max_length ==
1)`.
- `char_tolower` is defined only for libc (the `ctype_methods_libc_*` tables
set `.char_tolower = char_tolower_libc`). ICU and builtin don't set it.
- For libc + single-byte: old took fast path, new takes fast path. ✓
- For ICU + single-byte: old took slow path, new takes slow path (no
`char_tolower`). ✓
- For builtin + single-byte: builtin only supports UTF8, so `max_length == 1`
is impossible. N/A.
- For `ctype_is_c`: old behavior depended on provider/encoding; new always
takes fast path.
What does `ctype_is_c` mean? It's set for the C/POSIX libc locale and for the
builtin "C" locale. In those cases the old code's behavior:
- libc "C" with single-byte: old went fast path. New goes fast path. ✓
- libc "C" with multibyte (UTF8): old went slow path (called `lower()` then
`MB_MatchText`). New goes fast path with `SB_IMatchText`.
**This is a behavioral change.** With `ctype_is_c` and UTF8 encoding, the new
code uses single-byte matching while the old used multibyte. For the C locale,
ASCII-only case folding is correct byte-wise even in UTF8 (since multibyte
sequences have only high-bit-set continuation bytes, and `SB_lower_char` with
`ctype_is_c` only lowercases ASCII via `pg_ascii_tolower`). So this is likely
intentional and correct — but it is a behavioral change worth noting. It may
even be an improvement (skipping the `lower()` call).
Actually, let me reconsider: `SB_IMatchText` operates byte-by-byte and uses
`SB_lower_char`. With `ctype_is_c`, that returns `pg_ascii_tolower(c)`, which
is safe on UTF8 continuation bytes (they're > 127, untouched). Pattern matching
for LIKE with `_` wildcard might have problems in UTF8 though — `_` matches one
character, but `SB_IMatchText` would match one *byte*. Looking at the existing
code... yes, this was already a known consideration: the old code routed UTF8
to `MB_MatchText` precisely because `_` needs to match a character, not a byte.
**So for `ctype_is_c` + UTF8, LIKE `_` will now match a single byte instead of
a single character.** That's a real regression for queries like `'café' ILIKE
'caf_'` under the C locale in a UTF8 database. The old code would lowercase
then call `MB_MatchText` (correct: `_` matches `é`). The new code calls
`SB_IMatchText` which makes `_` match a single byte (incorrect: `é` is two
bytes in UTF8, so `'caf_'` wouldn't match `'café'`).
This looks like a **real bug**.
## Issue 5: `SB_lower_char` change
Old:
```c
return tolower_l(c, locale->info.lt);
```
New:
```c
return char_tolower(c, locale);
```
`char_tolower` dispatches to `locale->ctype->char_tolower`. This is only set
for libc. `SB_lower_char` is called from `SB_IMatchText`, which (in the old
code) was only reached when `max_length == 1 && provider != ICU`. In the old
code, all those cases were libc (since builtin requires UTF8).
In the new code, `SB_IMatchText` can be reached for `ctype_is_c` regardless of
provider — including builtin "C" (UTF8). Builtin doesn't set `char_tolower`, so
`char_tolower(c, locale)` would crash with a NULL function pointer.
But wait — let's look more carefully. In the `SB_lower_char` path,
`locale->is_default` is checked first; if true, `pg_tolower` is used. For
`ctype_is_c`, the code path enters this function only when `locale->ctype_is_c`
is true. Looking at `SB_lower_char`:
```c
if (locale->ctype_is_c)
return pg_ascii_tolower(c);
else if (locale->is_default)
return pg_tolower(c);
else
return char_tolower(c, locale);
```
OK so when `ctype_is_c`, it returns early via `pg_ascii_tolower`. So no NULL
deref. ✓
But the `_` wildcard byte-vs-character mismatch in UTF8 with `ctype_is_c` still
stands as a real bug.
Let me also check the `max_chr` field for builtin and ICU — they don't set it,
which defaults to 0. In `pg_ctype_get_cache`:
```c
if (pg_regex_locale->ctype->max_chr != 0 &&
pg_regex_locale->ctype->max_chr <= MAX_SIMPLE_CHR)
{
max_chr = pg_regex_locale->ctype->max_chr;
pcc->cv.cclasscode = -1;
}
else
max_chr = (pg_wchar) MAX_SIMPLE_CHR;
```
The original code for `LIBC_1BYTE` had:
```c
#if MAX_SIMPLE_CHR >= UCHAR_MAX
max_chr = (pg_wchar) UCHAR_MAX;
pcc->cv.cclasscode = -1;
#else
max_chr = (pg_wchar) MAX_SIMPLE_CHR;
#endif
```
This was conditional on `MAX_SIMPLE_CHR >= UCHAR_MAX`. New code drops the `#if`
— it sets `max_chr = UCHAR_MAX` unconditionally when libc-sb method table is in
use, and the runtime check `max_chr <= MAX_SIMPLE_CHR` handles the case where
`MAX_SIMPLE_CHR < UCHAR_MAX`. So that's fine for production but the `cclasscode
= -1` logic is preserved correctly. ✓
## Summary of findings
Here are the issues I'd flag, in order of severity:
**1. Real bug — LIKE/ILIKE wildcard semantics change for C locale in UTF8
database.** In `Generic_Text_IC_like`, the new fast-path condition
`locale->ctype_is_c || (char_tolower_enabled(locale) &&
pg_database_encoding_max_length() == 1)` sends C-locale UTF8 databases through
`SB_IMatchText` (byte-by-byte) instead of `MB_MatchText`
(character-by-character). This means `_` will match a single byte instead of a
single character. The old condition guaranteed `SB_IMatchText` was used only
when `max_length == 1`. Suggest: change the condition to also require
`pg_database_encoding_max_length() == 1` for the `ctype_is_c` branch, or split
it explicitly.
**2. Lost bounds check on libc ctype calls.** The new `wc_is*_libc_sb`
functions unconditionally do `isdigit_l((unsigned char) wc, ...)`, truncating
`wc` silently. The original code had explicit `c <= UCHAR_MAX` guards that
returned 0 for out-of-range values. The `max_chr = UCHAR_MAX` field protects
the regex cache path, but direct calls to `pg_wc_isdigit` etc. for `wc > 255`
will now return a result derived from a truncated byte rather than 0. Worth
verifying whether any code path passes `wc > 255` to these.
**3. Lost wchar_t-width fallback on 16-bit wchar_t platforms (Windows).** The
old `LIBC_WIDE` branch had `if (sizeof(wchar_t) >= 4 || c <= 0xFFFF)` with
`FALL THRU` to `LIBC_1BYTE` for codepoints above 0xFFFF on 16-bit-wchar_t
platforms. The new `wc_is*_libc_mb` functions just call `iswdigit_l((wint_t)
wc, ...)` unconditionally, and `toupper_libc_mb`/`tolower_libc_mb` return `wc`
unchanged for `c > 0xFFFF` on 16-bit-wchar_t. The old fall-through path may
have been doing something sketchy already, but it is a behavioral change on
Windows.
**4. Minor — comment block relocation note.** The big comment block moved from
`regc_pg_locale.c` to `pg_locale_libc.c`. The paragraph "We store the active
collation setting in static variables. In principle it could be passed down to
here via the regex library's 'struct vars' data structure…" is about the regex
library specifically and now lives in the libc file where it's somewhat out of
place. Not a bug, just slightly misleading documentation.
**5. Style — the new `pg_wc_toupper`/`pg_wc_tolower` `ctype_is_c` blocks** drop
the explicit `case`-style brace indentation but are otherwise faithful; the
`return c;` for `c > 127` is preserved.
Issue #1 looks like the most serious — it'd be worth writing a regression test
like `SELECT 'café' ILIKE 'caf_' COLLATE "C"` in a UTF8 database to confirm.
