Subject: Re: [PATCH] json_lex_string: don't overread on bad UTF8

From: Michael Paquier <[email-masked]>
Date: Wed, 8 May 2024 14:30:43 +0900

On Tue, May 07, 2024 at 02:06:10PM -0700, Jacob Champion wrote:
> Maybe I've misunderstood, but isn't that what's being done in v2?

Something a bit different..  I was wondering if it could be possible
to tweak this code to truncate the data in the generated error string
so as the incomplete multi-byte sequence is entirely cut out, which
would come to setting token_terminator to "s" (last byte before the
incomplete byte sequence) rather than "term" (last byte available,
even if incomplete):
#define FAIL_AT_CHAR_END(code) \
do { \
   char       *term = s + pg_encoding_mblen(lex->input_encoding, s); \
   lex->token_terminator = (term <= end) ? term : s; \
   return code; \
} while (0)

But looking closer, I can see that in the JSON_INVALID_TOKEN case,
when !tok_done, we set token_terminator to point to the end of the
token, and that would include an incomplete byte sequence like in your
case.  :/

At the end of the day, I think that I'm OK with your patch and avoid
the overread for now in the back-branches.  This situation makes me
uncomfortable and we should put more effort in printing error messages
in a readable format, but that could always be tackled later as a
separate problem..  And I don't see something backpatchable at short
sight for v16.

Thoughts and/or objections?
--
Michael

---

From: Jacob Champion <[email-masked]>
Date: Wed, 8 May 2024 07:01:08 -0700

> token, and that would include an incomplete byte sequence like in your

Ah, I see what you're saying. Yeah, that approach would need some more
invasive changes.

> This situation makes me

Agreed. Fortunately (or unfortunately?) I think the JSON
client-encoding work is now a prerequisite for OAuth in libpq, so
hopefully some improvements can fall out of that work too.

> Thoughts and/or objections?

None here.

Thanks!
--Jacob

---

From: Michael Paquier <[email-masked]>
Date: Thu, 9 May 2024 13:27:08 +0900

> Ah, I see what you're saying. Yeah, that approach would need some more

My first feeling was actually to do that, and report the location in
the input string where we are seeing issues.  All code paths playing
with token_terminator would need to track that.

> hopefully some improvements can fall out of that work too.

I'm afraid so.  I don't quite see how this would be OK to tweak on
stable branches, but all areas that could report error states with
partial byte sequence contents would benefit from such a change.

>> Thoughts and/or objections?

This is a bit mitigated by the fact that d6607016c738 is recent, but
this is incorrect since v13 so backpatched down to that.
--
Michael

---

From: Jacob Champion <[email-masked]>
Date: Thu, 9 May 2024 10:21:00 -0700

> this is incorrect since v13 so backpatched down to that.

Thank you!

--Jacob