Small patch to improve safety of utf8_to_unicode().

On Dec 13, 2025, at 07:24, Jeff Davis <pgsql@j-davis.com> wrote:

Attached.

<v1-0001-Make-utf8_to_unicode-safer.patch>

This patch adds a length check to utf8_to_unicode(), I think which is where “safety” comes from. Can you please add an a little bit more to the commit message instead of only saying “improve safety”. It also deleted two redundant function declarations from pg_wchar.h, which may also worth a quick note in the commit message.

The code changes all look good to me. Only nitpicks are:

1
```
diff --git a/contrib/fuzzystrmatch/daitch_mokotoff.c b/contrib/fuzzystrmatch/daitch_mokotoff.c
index 07f895ae2bf..47bd2814460 100644
--- a/contrib/fuzzystrmatch/daitch_mokotoff.c
+++ b/contrib/fuzzystrmatch/daitch_mokotoff.c
@@ -401,7 +401,8 @@ read_char(const unsigned char *str, int *ix)

 	/* Decode UTF-8 character to ISO 10646 code point. */
 	str += *ix;
-	c = utf8_to_unicode(str);
+	/* Assume byte sequence has not been broken. */
+	c = utf8_to_unicode(str, MAX_MULTIBYTE_CHAR_LEN);
```

Here we need an empty line above the new comment.

2
```
diff --git a/src/common/wchar.c b/src/common/wchar.c
index a4bc29921de..c113cadf815 100644
--- a/src/common/wchar.c
+++ b/src/common/wchar.c
@@ -661,7 +661,8 @@ ucs_wcwidth(pg_wchar ucs)
 static int
 pg_utf_dsplen(const unsigned char *s)
 {
-	return ucs_wcwidth(utf8_to_unicode(s));
+	/* trust that input is not a truncated byte sequence */
+	return ucs_wcwidth(utf8_to_unicode(s, MAX_MULTIBYTE_CHAR_LEN));
 }
```

For the new comment, as a code reader, I wonder why we “trust” that? To me, it more feels like because of lacking length information, we have to trust. I would like this comment to be enhanced a little bit with more information.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Sun, 2025-12-14 at 07:22 +0800, Chao Li wrote:

This patch adds a length check to utf8_to_unicode(), I think which is
where “safety” comes from. Can you please add an a little bit more to
the commit message instead of only saying “improve safety”.

Right: it does not read past pg_mblen(), nor past the supplied length.

We could go further and check for valid continuation bytes, but the
other routines don't do that.

It also deleted two redundant function declarations from pg_wchar.h,
which may also worth a quick note in the commit message.

I committed that as an independent change and removed it from this
patch.

+	/* Assume byte sequence has not been broken. */
+	c = utf8_to_unicode(str, MAX_MULTIBYTE_CHAR_LEN);
```

Here we need an empty line above the new comment.

Done, and I expanded the comment to explain why it's safe.

 pg_utf_dsplen(const unsigned char *s)
 {
-	return ucs_wcwidth(utf8_to_unicode(s));
+	/* trust that input is not a truncated byte sequence */
+	return ucs_wcwidth(utf8_to_unicode(s,
MAX_MULTIBYTE_CHAR_LEN));
 }
```

For the new comment, as a code reader, I wonder why we “trust” that?

We could use strlen(), but I was concerned that it might be used for
string fragments that aren't NUL-terminated, because it's intended for
a single character. A caller might reasonably assume that it wouldn't
read past pg_mblen().

So I changed the comment slightly to just say that it requires the
input is a valid UTF-8 sequence. Let me know if you have another
suggestion.

Regards,
Jeff Davis

On Dec 16, 2025, at 04:23, Jeff Davis <pgsql@j-davis.com> wrote:

On Sun, 2025-12-14 at 07:22 +0800, Chao Li wrote:

This patch adds a length check to utf8_to_unicode(), I think which is
where “safety” comes from. Can you please add an a little bit more to
the commit message instead of only saying “improve safety”.

Right: it does not read past pg_mblen(), nor past the supplied length.

We could go further and check for valid continuation bytes, but the
other routines don't do that.

It also deleted two redundant function declarations from pg_wchar.h,
which may also worth a quick note in the commit message.

I committed that as an independent change and removed it from this
patch.

+ /* Assume byte sequence has not been broken. */
+ c = utf8_to_unicode(str, MAX_MULTIBYTE_CHAR_LEN);
```

Here we need an empty line above the new comment.

Done, and I expanded the comment to explain why it's safe.

pg_utf_dsplen(const unsigned char *s)
{
- return ucs_wcwidth(utf8_to_unicode(s));
+ /* trust that input is not a truncated byte sequence */
+ return ucs_wcwidth(utf8_to_unicode(s,
MAX_MULTIBYTE_CHAR_LEN));
}
```

For the new comment, as a code reader, I wonder why we “trust” that?

We could use strlen(), but I was concerned that it might be used for
string fragments that aren't NUL-terminated, because it's intended for
a single character. A caller might reasonably assume that it wouldn't
read past pg_mblen().

So I changed the comment slightly to just say that it requires the
input is a valid UTF-8 sequence. Let me know if you have another
suggestion.

Regards,
Jeff Davis

<v2-0001-Make-utf8_to_unicode-safer.patch>

V2 LGTM.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Tue, 2025-12-16 at 07:34 +0800, Chao Li wrote:

<v2-0001-Make-utf8_to_unicode-safer.patch>

V2 LGTM.

On second thought, if we're going to change something here, we should
probably have a more flexible API for both utf8_to_unicode() and
unicode_to_utf8().

Looking at the callers, I think we want to have signatures something
like:

/* returns number of bytes consumed, or -1 */
static inline ssize_t
utf8_to_unicode(char32_t *cp, const unsigned char *src, size_t srclen)
{
...
}

/* returns number of bytes written, or -1 */
static inline ssize_t
unicode_to_utf8(unsigned char *dst, size_t dstsize, char32_t cp)
{
...
}

That would make both APIs safer, and the caller wouldn't need to call
unicode_utf8len() or pg_utf8_mblen() separately.

We could also do more validation, but of course then the callers would
need to do something if they encounter a failure. We could also try to
catch NUL terminators in the middle of a sequence, which might be
useful.

Regards,
Jeff Davis

On Wed, 2025-12-17 at 11:37 -0800, Jeff Davis wrote:

On Tue, 2025-12-16 at 07:34 +0800, Chao Li wrote:

<v2-0001-Make-utf8_to_unicode-safer.patch>

V2 LGTM.

On second thought, if we're going to change something here, we should
probably have a more flexible API for both utf8_to_unicode() and
unicode_to_utf8().

New series:

0001: validates UTF8 before calling into unicode_case.c. Extra defense,
and simple to backport, but regresses performance of those functions.
It also might risk errors if somehow there is invalid UTF8.

0002: refactors to create an error path from unicode_case.c into
pg_locale_builtin.c, where a proper error can be thrown. This wins back
the performance lost in the previous commit. This is perhaps
backportable, but technically it changes an exported function
signature, so carries some very low risk.

0003: Adds utf8encode() and utf8decode(), which are iteration-friendly
and inlinable, and fully-validate UTF8 (e.g. rejects surrogate halves).
This is an enhancement so should not be backported.

0004: Make use of new API from unicode_case.c.

Regards,
Jeff Davis

On Fri, 2026-06-19 at 16:22 -0700, Jeff Davis wrote:

On Wed, 2025-12-17 at 11:37 -0800, Jeff Davis wrote:

On Tue, 2025-12-16 at 07:34 +0800, Chao Li wrote:

<v2-0001-Make-utf8_to_unicode-safer.patch>

V2 LGTM.

On second thought, if we're going to change something here, we
should
probably have a more flexible API for both utf8_to_unicode() and
unicode_to_utf8().

v4 attached.

The main difference is that the first patch is more backportable. For
backbranches, I think the safest thing if we encounter invalid UTF8 is
to just terminate and return early. In master, we can change the API to
properly return the error upward.

Performance is not affected much, though in my brief tests it appeared
that 0002 lost a bit and then 0004 gained it back. But we gain full
UTF8 validation and safer UTF8 iterator APIs.

Regards,
Jeff Davis

On Mon, 2026-06-22 at 19:02 -0700, Jeff Davis wrote:

v4 attached.

v5 attached.

There's an extra patch 0002 to fix a logic bug when handling final
sigma (only affects the builtin pg_unicode_fast locale), which I think
should be backported to 18.

Also added tests.

Regards,
Jeff Davis

On Jun 24, 2026, at 13:45, Jeff Davis <pgsql@j-davis.com> wrote:

On Mon, 2026-06-22 at 19:02 -0700, Jeff Davis wrote:

v4 attached.

v5 attached.

There's an extra patch 0002 to fix a logic bug when handling final
sigma (only affects the builtin pg_unicode_fast locale), which I think
should be backported to 18.

Also added tests.

Regards,
Jeff Davis

<v5-0001-unicode_case.c-defend-against-invalid-UTF8.patch><v5-0002-pg_unicode_fast-fix-final-sigma-logic.patch><v5-0003-unicode_case.c-change-API-to-signal-UTF8-decoding.patch><v5-0004-Validating-iterator-friendly-UTF8-encoder-decoder.patch><v5-0005-unicode_case.c-use-new-utf8encode-utf8decode-APIs.patch>

There is a compile warning against pg_wchar.h in 0004:
```
../../../src/include/mb/pg_wchar.h:523:11: warning: variable 'codepoint' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
523 | else if (nbytes == 4)
| ^~~~~~~~~~~
../../../src/include/mb/pg_wchar.h:540:16: note: uninitialized use occurs here
540 | *pcodepoint = codepoint;
| ^~~~~~~~~
../../../src/include/mb/pg_wchar.h:523:7: note: remove the 'if' if its condition is always true
523 | else if (nbytes == 4)
| ^~~~~~~~~~~~~~~~
524 | {
../../../src/include/mb/pg_wchar.h:469:20: note: initialize the variable 'codepoint' to silence this warning
469 | char32_t codepoint;
| ^
| = 0
1 warning generated.
```

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Hi,

On Wed, 24 Jun 2026 at 11:15, Jeff Davis <pgsql@j-davis.com> wrote:

On Mon, 2026-06-22 at 19:02 -0700, Jeff Davis wrote:

v4 attached.

v5 attached.

There's an extra patch 0002 to fix a logic bug when handling final
sigma (only affects the builtin pg_unicode_fast locale), which I think
should be backported to 18.

Also added tests.

Thanks for the patch!

I took a look at the v5 series and tried it locally. The split between the
backpatchable defensive change, the final-sigma fix, and the newer
utf8encode/utf8decode API work for master makes sense to me.

On my machine, case-check fails before reaching the new invalid-UTF8
assertions. I think it's because PostgreSQL's Unicode tables are 17.0 while
my
system ICU is 15.1, so the exhaustive ICU comparison in test_icu() hits a
changed mapping and exits first.

Would it be worth running test_convert_case() before
test_icu()? It wouldn't make case-check pass on a mismatched-ICU system,
but it
would at least let the non-ICU conversion and the new invalid-UTF8 cases run
before the ICU comparison aborts.

Regards,
Ayush

On Wed, 2026-06-24 at 16:44 +0800, Chao Li wrote:

There is a compile warning against pg_wchar.h in 0004:

Fixed. I also used a loop in utf8decode() which is slightly smaller,
which is good if we intend it to be inlined by a lot of callers.

Regards,
Jeff Davis

On Wed, 2026-06-24 at 14:59 +0530, Ayush Tiwari wrote:

I took a look at the v5 series and tried it locally. The split
between the
backpatchable defensive change, the final-sigma fix, and the newer
utf8encode/utf8decode API work for master makes sense to me. 

Thank you for taking a look.

On my machine, case-check fails before reaching the new invalid-UTF8
assertions. I think it's because PostgreSQL's Unicode tables are 17.0
while my
system ICU is 15.1, so the exhaustive ICU comparison in test_icu()
hits a
changed mapping and exits first.

Yes, that test is a bit awkward because most of it is a comparison to
results from ICU, so if ICU is unavailable or based on an older version
of Unicode, then the test doesn't work.

Would it be worth running test_convert_case() before
test_icu()?

If we want to make those independent of ICU, I think we'd move them to
a regular test suite that's exercised everywhere (not just as part of
the 'update-unicode' target). But if we did so, that would be a very
small test suite, because most of the results are already checked in
the normal SQL tests. What makes these tests different is that they are
exercising invalid UTF8 behavior, which we don't expect to happen
through ordinary SQL.

Regards,
Jeff Davis

On Jun 25, 2026, at 05:57, Jeff Davis <pgsql@j-davis.com> wrote:

On Wed, 2026-06-24 at 16:44 +0800, Chao Li wrote:

There is a compile warning against pg_wchar.h in 0004:

Fixed. I also used a loop in utf8decode() which is slightly smaller,
which is good if we intend it to be inlined by a lot of callers.

Regards,
Jeff Davis

<v5-0001-unicode_case.c-defend-against-invalid-UTF8.patch><v5-0002-pg_unicode_fast-fix-final-sigma-logic.patch><v5-0003-unicode_case.c-change-API-to-signal-UTF8-decoding.patch><v5-0004-Validating-iterator-friendly-UTF8-encoder-decoder.patch><v5-0005-unicode_case.c-use-new-utf8encode-utf8decode-APIs.patch>

I just reviewed v5-0001 and got one concern.

In initcap_wbnext(), the new check only verifies that the input has enough bytes:
```
if (wbstate->offset + ulen > wbstate->len)
```

What about an invalid continuation byte, for example "\xCE "? In this case, pg_utf_mblen() sees \xCE, so ulen will be 2. Since there is still one more byte, the length check won't catch the invalid continuation byte \x20, and the code will proceed to utf8_to_unicode().

Looking at utf8_to_unicode():
```
static inline char32_t
utf8_to_unicode(const unsigned char *c)
{
if ((*c & 0x80) == 0)
return (char32_t) c[0];
else if ((*c & 0xe0) == 0xc0)
return (char32_t) (((c[0] & 0x1f) << 6) |
(c[1] & 0x3f));
else if ((*c & 0xf0) == 0xe0)
return (char32_t) (((c[0] & 0x0f) << 12) |
((c[1] & 0x3f) << 6) |
(c[2] & 0x3f));
else if ((*c & 0xf8) == 0xf0)
return (char32_t) (((c[0] & 0x07) << 18) |
((c[1] & 0x3f) << 12) |
((c[2] & 0x3f) << 6) |
(c[3] & 0x3f));
else
return PG_INVALID_CODEPOINT;
}
```

For "\xCE ", it will take this branch:
```
else if ((*c & 0xe0) == 0xc0)
return (char32_t) (((c[0] & 0x1f) << 6) |
(c[1] & 0x3f));
```

This uses the second byte, \x20, without validating. So it looks like the patch prevents reading past the end of the string, but it may not fully defend against invalid UTF-8 sequences.

Am I missing anything?

(I will continue to review 0002 tomorrow.)

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Thu, 2026-06-25 at 12:10 +0800, Chao Li wrote:

This uses the second byte, \x20, without validating. So it looks like
the patch prevents reading past the end of the string, but it may not
fully defend against invalid UTF-8 sequences.

Correct. We don't do full UTF8 validation until the last patch in the
series, which is not being backported.

Trying to do full validation in the backbranches seems more likely to
cause problems than prevent them. We aren't expecting invalid UTF8, but
in the event it got there somehow (perhaps from an old upgraded
instance), throwing errors after a minor release is probably not
helpful.

Even in master, I am not 100% sure we want to detect other kinds of
validation errors while processing the UTF8. By the time we are using
the value, maybe truncated multibyte sequences are the only thing we
care about, and we just need to be sure the code can handle anything
that fits in a char32_t.

Another thing to consider is an embedded NUL character, which is valid
UTF8 but not valid in a TEXT value.

Regards,
Jeff Davis

On Jun 26, 2026, at 01:38, Jeff Davis <pgsql@j-davis.com> wrote:

On Thu, 2026-06-25 at 12:10 +0800, Chao Li wrote:

This uses the second byte, \x20, without validating. So it looks like
the patch prevents reading past the end of the string, but it may not
fully defend against invalid UTF-8 sequences.

Correct. We don't do full UTF8 validation until the last patch in the
series, which is not being backported.

Sounds like 0001 will be back patched. In that case, the commit message "defend against invalid UTF8” seems too broad. Does it make sense to add some brief description about the defend behavior to the function header comment and the commit message?

Then I continue to review 0002-0005:

0002 - overall looks good. A small comment is:
```
+ for (int i = offset; i > 0;)

+ for (int i = offset + ulen; i < len;)
```

As offset is of type size_t, the loop variable i is better to be size_t.

0003 - looks good.

0004 - looks good. This commit introduces a new helper utf8decode() that will resolve my previous concern on 0001.

0005 - Mostly looks good. This commit applies the new help and my previous concern is resolved. But from what you talked, I guess 0004 and 0005 will only be pushed to HEAD.

Just one tiny comment on 0005:
```
+	/* invalid UTF8: surrogates */
+	needed = unicode_strfold(NULL, 0, "abc\xED\xA0\x81xyz", 7, &consumed, false);
+	Assert(needed == 3 && consumed == 3);
```

This test passes a 10-char string but uses 7 as srclen. I know that doesn’t affect the test result, but it just adds unnecessary confusion to readers. So maybe change 7 to 10 to reflect to the real string length.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Fri, 2026-06-26 at 12:38 +0800, Chao Li wrote:

Sounds like 0001 will be back patched. In that case, the commit
message "defend against invalid UTF8” seems too broad. Does it make
sense to add some brief description about the defend behavior to the
function header comment and the commit message?

Right. Would "defend against truncated byte sequences" or "defend
against truncated UTF8" be better wording?

Regards,
Jeff Davis

On Jun 27, 2026, at 22:02, Jeff Davis <pgsql@j-davis.com> wrote:

On Fri, 2026-06-26 at 12:38 +0800, Chao Li wrote:

Sounds like 0001 will be back patched. In that case, the commit
message "defend against invalid UTF8” seems too broad. Does it make
sense to add some brief description about the defend behavior to the
function header comment and the commit message?

Right. Would "defend against truncated byte sequences" or "defend
against truncated UTF8" be better wording?

Regards,
Jeff Davis

Yes, I think they are better. “Truncated” is more specific than “invalid”. I'm slightly more keen on the second phrase.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Fri, 2026-06-26 at 12:38 +0800, Chao Li wrote:

Sounds like 0001 will be back patched. In that case, the commit
message "defend against invalid UTF8” seems too broad. Does it make
sense to add some brief description about the defend behavior to the
function header comment and the commit message?

Committed 0001 and backported to 17. I think adding too much
explanation about behavior we don't expect to actually see would just
add confusion.

Then I continue to review 0002-0005:

0002 - overall looks good. A small comment is:
```
+ for (int i = offset; i > 0;)

+ for (int i = offset + ulen; i < len;)
```

Committed 0002 and backported to 18. The 'int' is pre-existing, so I
left it as-is.

0003 - looks good.

Committed to master only.

0004 - looks good. This commit introduces a new helper utf8decode()
that will resolve my previous concern on 0001.

0005 - Mostly looks good. This commit applies the new help and my
previous concern is resolved. But from what you talked, I guess 0004
and 0005 will only be pushed to HEAD.

Just one tiny comment on 0005:
```
+	/* invalid UTF8: surrogates */
+	needed = unicode_strfold(NULL, 0, "abc\xED\xA0\x81xyz", 7,
&consumed, false);
+	Assert(needed == 3 && consumed == 3);
```

This test passes a 10-char string but uses 7 as srclen. I know that
doesn’t affect the test result, but it just adds unnecessary
confusion to readers. So maybe change 7 to 10 to reflect to the real
string length.

Thank you, attached with fix.

I'd like to wait for more comments before I commit these last two
patches, to see if the functions are generally useful for other callers
as well.

Regards,
Jeff Davis

On Jul 7, 2026, at 17:56, Jeff Davis <pgsql@j-davis.com> wrote:

On Fri, 2026-06-26 at 12:38 +0800, Chao Li wrote:

Sounds like 0001 will be back patched. In that case, the commit
message "defend against invalid UTF8” seems too broad. Does it make
sense to add some brief description about the defend behavior to the
function header comment and the commit message?

Committed 0001 and backported to 17. I think adding too much
explanation about behavior we don't expect to actually see would just
add confusion.

Then I continue to review 0002-0005:

0002 - overall looks good. A small comment is:
```
+ for (int i = offset; i > 0;)

+ for (int i = offset + ulen; i < len;)
```

Committed 0002 and backported to 18. The 'int' is pre-existing, so I
left it as-is.

0003 - looks good.

Committed to master only.

0004 - looks good. This commit introduces a new helper utf8decode()
that will resolve my previous concern on 0001.

0005 - Mostly looks good. This commit applies the new help and my
previous concern is resolved. But from what you talked, I guess 0004
and 0005 will only be pushed to HEAD.

Just one tiny comment on 0005:
```
+ /* invalid UTF8: surrogates */
+ needed = unicode_strfold(NULL, 0, "abc\xED\xA0\x81xyz", 7,
&consumed, false);
+ Assert(needed == 3 && consumed == 3);
```

This test passes a 10-char string but uses 7 as srclen. I know that
doesn’t affect the test result, but it just adds unnecessary
confusion to readers. So maybe change 7 to 10 to reflect to the real
string length.

Thank you, attached with fix.

I'd like to wait for more comments before I commit these last two
patches, to see if the functions are generally useful for other callers
as well.

Regards,
Jeff Davis

<v6-0001-Validating-iterator-friendly-UTF8-encoder-decoder.patch><v6-0002-unicode_case.c-use-new-utf8encode-utf8decode-APIs.patch>

V6 looks good to me.

Best regards,
—
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Chao Li <li.evan.chao@gmail.com> writes:

On Jul 7, 2026, at 17:56, Jeff Davis <pgsql@j-davis.com> wrote:

I'd like to wait for more comments before I commit these last two
patches, to see if the functions are generally useful for other callers
as well.

V6 looks good to me.

Coverity is unimpressed, although after reading 07211f64a I wonder if
this patch didn't simply trigger it to pick up a pre-existing problem
(it does seem to act that way sometimes). It now complains:

/srv/coverity/git/pgsql-git/postgresql/src/common/unicode_case.c: 324 in convert_case()
318 }
319
320 if (result_len < dstsize)
321 dst[result_len] = '\0';
322
323 *pconsumed = srcoff;

CID 1696680: (INTEGER_OVERFLOW)
"result_len", which might have overflowed, is returned from the function.

324 return result_len;
325 }

What I think this is pointing out is that even if the input string's
length fits in size_t, the length of the case-converted equivalent
string might not. The code won't write past dstsize, but it will
try to compute the full output length required, and it won't notice
if result_len overflows and wraps around.

I would have written this off as an unreachable edge case, but our
recent experience with unicode_normalize (cf. commit 066b7b144)
makes me hesitant to assume that case-folding can't result in
integer-multiple growth of the string length. If it can, the overflow
might be reachable on 32-bit platforms, resulting in a silently-broken
result (but no memory clobber AFAICS).

Catering for this case seems like it would require messy warts on the
casefolding functions' API. So if we can convince ourselves that
overflow can't really occur, I'd be content to add a comment
demonstrating that. But if we can't prove that, I think we need some
warts :-(

regards, tom lane