public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jacob Champion <[email protected]>
To: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Subject: Re: [PATCH] json_lex_string: don't overread on bad UTF8
Date: Wed, 1 May 2024 16:22:24 -0700
Message-ID: <CAOYmi+=AQKWhcU1nuW8pcMdby6UV6Sd-zugqv4v4a+NUF3FPBg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAOYmi+ncM7pwLS3AnKCSmoqqtpjvA8wmCdoBtKA3ZrB2hZG6zA@mail.gmail.com>
<[email protected]>
On Tue, Apr 30, 2024 at 11:09 PM Michael Paquier <[email protected]> wrote:
> Not sure to like much the fact that this advances token_terminator
> first. Wouldn't it be better to calculate pg_encoding_mblen() first,
> then save token_terminator? I feel a bit uneasy about saving a value
> in token_terminator past the end of the string. That a nit in this
> context, still..
v2 tries it that way; see what you think. Is the concern that someone
might add code later that escapes that macro early?
> Hmm. Keeping it around as currently designed means that it could
> cause more harm than anything in the long term, even in the stable
> branches if new code uses it. There is a risk of seeing this new code
> incorrectly using it again, even if its top comment tells that we rely
> on the string being nul-terminated. A safer alternative would be to
> redesign it so as the length of the string is provided in input,
> removing the dependency of strlen in its internals, perhaps. Anyway,
> without any callers of it, I'd be tempted to wipe it from HEAD and
> call it a day.
Removed in v2.
> > The new test needs to record two versions of the error message, one
> > for invalid token and one for invalid escape sequence. This is
> > because, for smaller chunk sizes, the partial-token logic in the
> > incremental JSON parser skips the affected code entirely when it can't
> > find an ending double-quote.
>
> Ah, that makes sense. That looks OK here. A comment around the test
> would be adapted to document that, I guess.
Done.
> Advancing the tracking pointer 's' before reporting an error related
> the end of the string is a bad practive, IMO, and we should avoid
> that. json_lex_string() does not offer a warm feeling regarding that
> with escape characters, at least :/
Yeah... I think some expansion of the json_errdetail test coverage is
probably in my future. :)
Thanks,
--Jacob
Attachments:
[application/octet-stream] v2-0001-json_lex_string-don-t-overread-on-bad-UTF8.patch (3.8K, ../CAOYmi+=AQKWhcU1nuW8pcMdby6UV6Sd-zugqv4v4a+NUF3FPBg@mail.gmail.com/2-v2-0001-json_lex_string-don-t-overread-on-bad-UTF8.patch)
download | inline diff:
From 7573859b6f66b4ed370725f33077361c1cb81cb7 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Mon, 8 Apr 2024 15:31:17 -0700
Subject: [PATCH v2] json_lex_string: don't overread on bad UTF8
Inputs to pg_parse_json[_incremental] are not guaranteed to be
null-terminated, so pg_encoding_mblen_bounded (which uses strnlen) can
walk off the end of the buffer. Check against the end pointer instead.
pg_encoding_mblen_bounded() no longer has any callers and has been
removed.
TODO:
- Do we really want to print incomplete UTF-8 sequences as-is once we
know they're bad?
---
src/common/jsonapi.c | 4 ++--
src/common/wchar.c | 13 +------------
src/include/mb/pg_wchar.h | 1 -
src/test/modules/test_json_parser/t/002_inline.pl | 8 ++++++++
4 files changed, 11 insertions(+), 15 deletions(-)
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index fc0cb36974..26e1f43ed3 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -1689,8 +1689,8 @@ json_lex_string(JsonLexContext *lex)
} while (0)
#define FAIL_AT_CHAR_END(code) \
do { \
- lex->token_terminator = \
- s + pg_encoding_mblen_bounded(lex->input_encoding, s); \
+ char *term = s + pg_encoding_mblen(lex->input_encoding, s); \
+ lex->token_terminator = (term <= end) ? term : end; \
return code; \
} while (0)
diff --git a/src/common/wchar.c b/src/common/wchar.c
index 76b7dfdfcb..97e9b61dba 100644
--- a/src/common/wchar.c
+++ b/src/common/wchar.c
@@ -2062,8 +2062,7 @@ const pg_wchar_tbl pg_wchar_table[] = {
*
* Caution: when dealing with text that is not certainly valid in the
* specified encoding, the result may exceed the actual remaining
- * string length. Callers that are not prepared to deal with that
- * should use pg_encoding_mblen_bounded() instead.
+ * string length.
*/
int
pg_encoding_mblen(int encoding, const char *mbstr)
@@ -2073,16 +2072,6 @@ pg_encoding_mblen(int encoding, const char *mbstr)
pg_wchar_table[PG_SQL_ASCII].mblen((const unsigned char *) mbstr));
}
-/*
- * Returns the byte length of a multibyte character; but not more than
- * the distance to end of string.
- */
-int
-pg_encoding_mblen_bounded(int encoding, const char *mbstr)
-{
- return strnlen(mbstr, pg_encoding_mblen(encoding, mbstr));
-}
-
/*
* Returns the display length of a multibyte character.
*/
diff --git a/src/include/mb/pg_wchar.h b/src/include/mb/pg_wchar.h
index 249cd18a35..ac65bfcbef 100644
--- a/src/include/mb/pg_wchar.h
+++ b/src/include/mb/pg_wchar.h
@@ -663,7 +663,6 @@ extern int pg_valid_server_encoding_id(int encoding);
* earlier in this file are also available from libpgcommon.
*/
extern int pg_encoding_mblen(int encoding, const char *mbstr);
-extern int pg_encoding_mblen_bounded(int encoding, const char *mbstr);
extern int pg_encoding_dsplen(int encoding, const char *mbstr);
extern int pg_encoding_verifymbchar(int encoding, const char *mbstr, int len);
extern int pg_encoding_verifymbstr(int encoding, const char *mbstr, int len);
diff --git a/src/test/modules/test_json_parser/t/002_inline.pl b/src/test/modules/test_json_parser/t/002_inline.pl
index f83cec03f8..60bb930e92 100644
--- a/src/test/modules/test_json_parser/t/002_inline.pl
+++ b/src/test/modules/test_json_parser/t/002_inline.pl
@@ -128,5 +128,13 @@ test(
"incorrect escape count",
'"\\\\\\\\\\\\\\"',
error => qr/Token ""\\\\\\\\\\\\\\"" is invalid/);
+test(
+ "incomplete UTF-8 sequence",
+ # Three bytes: double-quote, backslash, <f5>
+ "\"\\\x{F5}",
+ # Both invalid-token and invalid-escape are possible, because for smaller
+ # chunk sizes the incremental parser will skip the string parsing when it
+ # can't find an ending quote.
+ error => qr/(Token|Escape sequence) ""?\\\x{F5}" is invalid/);
done_testing();
--
2.34.1
view thread (4+ 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: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [PATCH] json_lex_string: don't overread on bad UTF8
In-Reply-To: <CAOYmi+=AQKWhcU1nuW8pcMdby6UV6Sd-zugqv4v4a+NUF3FPBg@mail.gmail.com>
* 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