Support json_errdetail in FRONTEND builds
Hello,
Both the incremental JSON [1]/messages/by-id/682c8fff-355c-a04f-57ac-81055c4ccda8@dunslane.net and OAuth [2]/messages/by-id/CAOYmi+kKNZCL7uz-LHyBggM+fEcf4285pFWwm7spkUb8irY7mQ@mail.gmail.com patchsets would be
improved by json_errdetail(), which was removed from FRONTEND builds
in b44669b2ca:
The routine providing a detailed error message based on the error code
is made backend-only, the existing code being unsafe to use in the
frontend as the error message may finish by being palloc'd or point to a
static string, so there is no way to know if the memory of the message
should be pfree'd or not.
Attached is a patch to undo this, by attaching any necessary
allocations to the JsonLexContext so the caller doesn't have to keep
track of them.
This is based on the first patch of the OAuth patchset, which
additionally needs json_errdetail() to be safe to use from libpq
itself. Alvaro pointed out offlist that we don't have to go that far
to re-enable this function for the utilities, so this patch is a sort
of middle ground between what we have now and what OAuth implements.
(There is some additional minimization that could be done to this
patch, but I'm hoping to keep the code structure consistent between
the two, if the result is acceptable.)
Two notes that I wanted to point out explicitly:
- On the other thread, Daniel contributed a destroyStringInfo()
counterpart for makeStringInfo(), which is optional but seemed useful
to include here.
- After this patch, if a frontend client calls json_errdetail()
without calling freeJsonLexContext(), it will leak memory.
Thanks,
--Jacob
[1]: /messages/by-id/682c8fff-355c-a04f-57ac-81055c4ccda8@dunslane.net
[2]: /messages/by-id/CAOYmi+kKNZCL7uz-LHyBggM+fEcf4285pFWwm7spkUb8irY7mQ@mail.gmail.com
Attachments:
0001-common-jsonapi-support-json_errdetail-in-FRONTEND.patchapplication/octet-stream; name=0001-common-jsonapi-support-json_errdetail-in-FRONTEND.patchDownload
From df7ae7991a7daf042729a09092b741b0cf7d14c3 Mon Sep 17 00:00:00 2001
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Tue, 12 Mar 2024 09:44:53 -0700
Subject: [PATCH] common/jsonapi: support json_errdetail in FRONTEND
json_errdetail() now allocates its error message inside memory owned by
the JsonLexContext, so clients don't need to worry about freeing it. We
can now partially revert b44669b2ca.
Extracted from the OAuth patchset; the new destroyStringInfo() API is by
Daniel Gustafsson.
---
src/bin/pg_verifybackup/t/005_bad_manifest.pl | 3 +-
src/common/jsonapi.c | 132 +++++++++++-------
src/common/parse_manifest.c | 2 +-
src/common/stringinfo.c | 16 +++
src/include/common/jsonapi.h | 1 +
src/include/lib/stringinfo.h | 11 +-
6 files changed, 109 insertions(+), 56 deletions(-)
diff --git a/src/bin/pg_verifybackup/t/005_bad_manifest.pl b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
index e278ccea5a..e2a297930e 100644
--- a/src/bin/pg_verifybackup/t/005_bad_manifest.pl
+++ b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
@@ -13,7 +13,8 @@ use Test::More;
my $tempdir = PostgreSQL::Test::Utils::tempdir;
test_bad_manifest('input string ended unexpectedly',
- qr/could not parse backup manifest: parsing failed/, <<EOM);
+ qr/could not parse backup manifest: The input string ended unexpectedly/,
+ <<EOM);
{
EOM
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 32931ded82..d70f4267da 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -170,23 +170,27 @@ makeJsonLexContextCstringLen(JsonLexContext *lex, char *json,
lex->strval = makeStringInfo();
lex->flags |= JSONLEX_FREE_STRVAL;
}
+ lex->errormsg = NULL;
return lex;
}
/*
- * Free memory in a JsonLexContext. There's no need for this if a *lex
- * pointer was given when the object was made and need_escapes was false,
- * or (in backend environment) a memory context delete/reset is imminent.
+ * Free memory in a JsonLexContext.
+ *
+ * There's no need for this if a *lex pointer was given when the object was
+ * made, need_escapes was false, and json_errdetail() was not called; or if (in
+ * backend environment) a memory context delete/reset is imminent.
*/
void
freeJsonLexContext(JsonLexContext *lex)
{
if (lex->flags & JSONLEX_FREE_STRVAL)
- {
- pfree(lex->strval->data);
- pfree(lex->strval);
- }
+ destroyStringInfo(lex->strval);
+
+ if (lex->errormsg)
+ destroyStringInfo(lex->errormsg);
+
if (lex->flags & JSONLEX_FREE_STRUCT)
pfree(lex);
}
@@ -1145,72 +1149,84 @@ report_parse_error(JsonParseContext ctx, JsonLexContext *lex)
return JSON_SUCCESS; /* silence stupider compilers */
}
-
-#ifndef FRONTEND
-/*
- * Extract the current token from a lexing context, for error reporting.
- */
-static char *
-extract_token(JsonLexContext *lex)
-{
- int toklen = lex->token_terminator - lex->token_start;
- char *token = palloc(toklen + 1);
-
- memcpy(token, lex->token_start, toklen);
- token[toklen] = '\0';
- return token;
-}
-
/*
* Construct an (already translated) detail message for a JSON error.
*
- * Note that the error message generated by this routine may not be
- * palloc'd, making it unsafe for frontend code as there is no way to
- * know if this can be safely pfree'd or not.
+ * The returned allocation is either static or owned by the JsonLexContext and
+ * should not be freed.
*/
char *
json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
{
+ int toklen = lex->token_terminator - lex->token_start;
+
+ if (lex->errormsg)
+ resetStringInfo(lex->errormsg);
+ else
+ lex->errormsg = makeStringInfo();
+
switch (error)
{
case JSON_SUCCESS:
/* fall through to the error code after switch */
break;
case JSON_ESCAPING_INVALID:
- return psprintf(_("Escape sequence \"\\%s\" is invalid."),
- extract_token(lex));
+ appendStringInfo(lex->errormsg,
+ _("Escape sequence \"\\%.*s\" is invalid."),
+ toklen, lex->token_start);
+ break;
case JSON_ESCAPING_REQUIRED:
- return psprintf(_("Character with value 0x%02x must be escaped."),
- (unsigned char) *(lex->token_terminator));
+ appendStringInfo(lex->errormsg,
+ _("Character with value 0x%02x must be escaped."),
+ (unsigned char) *(lex->token_terminator));
+ break;
case JSON_EXPECTED_END:
- return psprintf(_("Expected end of input, but found \"%s\"."),
- extract_token(lex));
+ appendStringInfo(lex->errormsg,
+ _("Expected end of input, but found \"%.*s\"."),
+ toklen, lex->token_start);
+ break;
case JSON_EXPECTED_ARRAY_FIRST:
- return psprintf(_("Expected array element or \"]\", but found \"%s\"."),
- extract_token(lex));
+ appendStringInfo(lex->errormsg,
+ _("Expected array element or \"]\", but found \"%.*s\"."),
+ toklen, lex->token_start);
+ break;
case JSON_EXPECTED_ARRAY_NEXT:
- return psprintf(_("Expected \",\" or \"]\", but found \"%s\"."),
- extract_token(lex));
+ appendStringInfo(lex->errormsg,
+ _("Expected \",\" or \"]\", but found \"%.*s\"."),
+ toklen, lex->token_start);
+ break;
case JSON_EXPECTED_COLON:
- return psprintf(_("Expected \":\", but found \"%s\"."),
- extract_token(lex));
+ appendStringInfo(lex->errormsg,
+ _("Expected \":\", but found \"%.*s\"."),
+ toklen, lex->token_start);
+ break;
case JSON_EXPECTED_JSON:
- return psprintf(_("Expected JSON value, but found \"%s\"."),
- extract_token(lex));
+ appendStringInfo(lex->errormsg,
+ _("Expected JSON value, but found \"%.*s\"."),
+ toklen, lex->token_start);
+ break;
case JSON_EXPECTED_MORE:
return _("The input string ended unexpectedly.");
case JSON_EXPECTED_OBJECT_FIRST:
- return psprintf(_("Expected string or \"}\", but found \"%s\"."),
- extract_token(lex));
+ appendStringInfo(lex->errormsg,
+ _("Expected string or \"}\", but found \"%.*s\"."),
+ toklen, lex->token_start);
+ break;
case JSON_EXPECTED_OBJECT_NEXT:
- return psprintf(_("Expected \",\" or \"}\", but found \"%s\"."),
- extract_token(lex));
+ appendStringInfo(lex->errormsg,
+ _("Expected \",\" or \"}\", but found \"%.*s\"."),
+ toklen, lex->token_start);
+ break;
case JSON_EXPECTED_STRING:
- return psprintf(_("Expected string, but found \"%s\"."),
- extract_token(lex));
+ appendStringInfo(lex->errormsg,
+ _("Expected string, but found \"%.*s\"."),
+ toklen, lex->token_start);
+ break;
case JSON_INVALID_TOKEN:
- return psprintf(_("Token \"%s\" is invalid."),
- extract_token(lex));
+ appendStringInfo(lex->errormsg,
+ _("Token \"%.*s\" is invalid."),
+ toklen, lex->token_start);
+ break;
case JSON_UNICODE_CODE_POINT_ZERO:
return _("\\u0000 cannot be converted to text.");
case JSON_UNICODE_ESCAPE_FORMAT:
@@ -1219,9 +1235,19 @@ json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
/* note: this case is only reachable in frontend not backend */
return _("Unicode escape values cannot be used for code point values above 007F when the encoding is not UTF8.");
case JSON_UNICODE_UNTRANSLATABLE:
- /* note: this case is only reachable in backend not frontend */
+
+ /*
+ * note: this case is only reachable in backend not frontend.
+ * #ifdef it away so the frontend doesn't try to link against
+ * backend functionality.
+ */
+#ifndef FRONTEND
return psprintf(_("Unicode escape value could not be translated to the server's encoding %s."),
GetDatabaseEncodingName());
+#else
+ Assert(false);
+ break;
+#endif
case JSON_UNICODE_HIGH_SURROGATE:
return _("Unicode high surrogate must not follow a high surrogate.");
case JSON_UNICODE_LOW_SURROGATE:
@@ -1236,7 +1262,9 @@ json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
* unhandled enum values. But this needs to be here anyway to cover the
* possibility of an incorrect input.
*/
- elog(ERROR, "unexpected json parse error type: %d", (int) error);
- return NULL;
+ if (lex->errormsg->len == 0)
+ appendStringInfo(lex->errormsg,
+ "unexpected json parse error type: %d", (int) error);
+
+ return lex->errormsg->data;
}
-#endif
diff --git a/src/common/parse_manifest.c b/src/common/parse_manifest.c
index 92a97714f3..62d93989be 100644
--- a/src/common/parse_manifest.c
+++ b/src/common/parse_manifest.c
@@ -147,7 +147,7 @@ json_parse_manifest(JsonManifestParseContext *context, char *buffer,
/* Run the actual JSON parser. */
json_error = pg_parse_json(lex, &sem);
if (json_error != JSON_SUCCESS)
- json_manifest_parse_failure(context, "parsing failed");
+ json_manifest_parse_failure(context, json_errdetail(json_error, lex));
if (parse.state != JM_EXPECT_EOF)
json_manifest_parse_failure(context, "manifest ended unexpectedly");
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index c61d5c58f3..da1f3fb5ad 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -350,3 +350,19 @@ enlargeStringInfo(StringInfo str, int needed)
str->maxlen = newlen;
}
+
+/*
+ * destroyStringInfo
+ *
+ * Frees the StringInfo and its buffer (the opposite of makeStringInfo()). This
+ * must only be called on palloc'd StringInfos.
+ */
+void
+destroyStringInfo(StringInfo str)
+{
+ /* read-only StringInfos must not be destroyed */
+ Assert(str->maxlen != 0);
+
+ pfree(str->data);
+ pfree(str);
+}
diff --git a/src/include/common/jsonapi.h b/src/include/common/jsonapi.h
index 02943cdad8..86a0fc2d00 100644
--- a/src/include/common/jsonapi.h
+++ b/src/include/common/jsonapi.h
@@ -89,6 +89,7 @@ typedef struct JsonLexContext
int line_number; /* line number, starting from 1 */
char *line_start; /* where that line starts within input */
StringInfo strval;
+ StringInfo errormsg;
} JsonLexContext;
typedef JsonParseErrorType (*json_struct_action) (void *state);
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 2cd636b01c..3befe01177 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -87,8 +87,9 @@ typedef StringInfoData *StringInfo;
* to be len + 1 in size.
*
* To destroy a StringInfo, pfree() the data buffer, and then pfree() the
- * StringInfoData if it was palloc'd. There's no special support for this.
- * However, if the StringInfo was initialized using initReadOnlyStringInfo()
+ * StringInfoData if it was palloc'd. (For StringInfos created with
+ * makeStringInfo(), destroyStringInfo() is provided for this purpose.) However,
+ * if the StringInfo was initialized using initReadOnlyStringInfo()
* then the caller will need to consider if it is safe to pfree the data
* buffer.
*
@@ -233,4 +234,10 @@ extern void appendBinaryStringInfoNT(StringInfo str,
*/
extern void enlargeStringInfo(StringInfo str, int needed);
+/*------------------------
+ * destroyStringInfo
+ * Frees the StringInfo and its buffer (the opposite of makeStringInfo()).
+ */
+extern void destroyStringInfo(StringInfo str);
+
#endif /* STRINGINFO_H */
--
2.34.1
On 2024-03-12 Tu 14:43, Jacob Champion wrote:
Hello,
Both the incremental JSON [1] and OAuth [2] patchsets would be
improved by json_errdetail(), which was removed from FRONTEND builds
in b44669b2ca:The routine providing a detailed error message based on the error code
is made backend-only, the existing code being unsafe to use in the
frontend as the error message may finish by being palloc'd or point to a
static string, so there is no way to know if the memory of the message
should be pfree'd or not.Attached is a patch to undo this, by attaching any necessary
allocations to the JsonLexContext so the caller doesn't have to keep
track of them.This is based on the first patch of the OAuth patchset, which
additionally needs json_errdetail() to be safe to use from libpq
itself. Alvaro pointed out offlist that we don't have to go that far
to re-enable this function for the utilities, so this patch is a sort
of middle ground between what we have now and what OAuth implements.
(There is some additional minimization that could be done to this
patch, but I'm hoping to keep the code structure consistent between
the two, if the result is acceptable.)
Seems reasonable.
Two notes that I wanted to point out explicitly:
- On the other thread, Daniel contributed a destroyStringInfo()
counterpart for makeStringInfo(), which is optional but seemed useful
to include here.
yeah, although maybe worth a different patch.
- After this patch, if a frontend client calls json_errdetail()
without calling freeJsonLexContext(), it will leak memory.
Not too concerned about this:
1. we tend to be a bit more relaxed about that in frontend programs,
especially those not expected to run for long times and especially on
error paths, where in many cases the program will just exit anyway.
2. the fix is simple where it's needed.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Tue, Mar 12, 2024 at 08:38:43PM -0400, Andrew Dunstan wrote:
On 2024-03-12 Tu 14:43, Jacob Champion wrote:
Two notes that I wanted to point out explicitly:
- On the other thread, Daniel contributed a destroyStringInfo()
counterpart for makeStringInfo(), which is optional but seemed useful
to include here.yeah, although maybe worth a different patch.
- {
- pfree(lex->strval->data);
- pfree(lex->strval);
- }
+ destroyStringInfo(lex->strval);I've wanted that a few times, FWIW. I would do a split, mainly for
clarity.
- After this patch, if a frontend client calls json_errdetail()
without calling freeJsonLexContext(), it will leak memory.Not too concerned about this:
1. we tend to be a bit more relaxed about that in frontend programs,
especially those not expected to run for long times and especially on error
paths, where in many cases the program will just exit anyway.2. the fix is simple where it's needed.
This does not stress me much, either. I can see that OAuth introduces
at least two calls of json_errdetail() in libpq, and that would matter
there.
case JSON_EXPECTED_STRING:
- return psprintf(_("Expected string, but found \"%s\"."),
- extract_token(lex));
+ appendStringInfo(lex->errormsg,
+ _("Expected string, but found \"%.*s\"."),
+ toklen, lex->token_start);Maybe this could be wrapped in a macro to avoid copying around
token_start and toklen for all the error cases.
--
Michael
On Tue, Mar 12, 2024 at 11:38 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Mar 12, 2024 at 08:38:43PM -0400, Andrew Dunstan wrote:
yeah, although maybe worth a different patch.
I've wanted that a few times, FWIW. I would do a split, mainly for
clarity.
Sounds good, split into v2-0002. (That gives me room to switch other
call sites to the new API, too.) Thanks both!
This does not stress me much, either. I can see that OAuth introduces
at least two calls of json_errdetail() in libpq, and that would matter
there.
Yep.
case JSON_EXPECTED_STRING: - return psprintf(_("Expected string, but found \"%s\"."), - extract_token(lex)); + appendStringInfo(lex->errormsg, + _("Expected string, but found \"%.*s\"."), + toklen, lex->token_start);Maybe this could be wrapped in a macro to avoid copying around
token_start and toklen for all the error cases.
I gave it a shot in 0001; see what you think.
Thanks,
--Jacob
Attachments:
v2-0002-Add-a-helper-function-for-cleaning-up-StringInfos.patchapplication/octet-stream; name=v2-0002-Add-a-helper-function-for-cleaning-up-StringInfos.patchDownload
From 0e3db5e04b094fb1bb6d5e497e565659acdde87a Mon Sep 17 00:00:00 2001
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Wed, 13 Mar 2024 09:35:19 -0700
Subject: [PATCH v2 2/2] Add a helper function for cleaning up StringInfos
destroyStringInfo() is a counterpart to makeStringInfo(): it frees a
palloc'd StringInfo and its data. API originally by Daniel Gustafsson,
with docs and tweaks added by me.
Co-authored-by: Daniel Gustafsson <daniel@yesql.se>
---
src/backend/backup/basebackup.c | 3 +--
src/backend/commands/subscriptioncmds.c | 3 +--
src/backend/utils/adt/jsonb.c | 3 +--
src/backend/utils/adt/xml.c | 6 ++----
src/bin/pg_combinebackup/pg_combinebackup.c | 5 +----
src/common/jsonapi.c | 10 ++--------
src/common/stringinfo.c | 16 ++++++++++++++++
src/include/lib/stringinfo.h | 9 ++++++++-
src/test/regress/pg_regress.c | 3 +--
9 files changed, 33 insertions(+), 25 deletions(-)
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 5fbbe5ffd2..5fea86ad0f 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -397,8 +397,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
endtli = backup_state->stoptli;
/* Deallocate backup-related variables. */
- pfree(tablespace_map->data);
- pfree(tablespace_map);
+ destroyStringInfo(tablespace_map);
pfree(backup_state);
}
PG_END_ENSURE_ERROR_CLEANUP(do_pg_abort_backup, BoolGetDatum(false));
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index a05d69922d..5a47fa984d 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -506,8 +506,7 @@ check_publications(WalReceiverConn *wrconn, List *publications)
appendStringInfoChar(cmd, ')');
res = walrcv_exec(wrconn, cmd->data, 1, tableRow);
- pfree(cmd->data);
- pfree(cmd);
+ destroyStringInfo(cmd);
if (res->status != WALRCV_OK_TUPLES)
ereport(ERROR,
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index a5e48744ac..a941654d5a 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -133,8 +133,7 @@ jsonb_send(PG_FUNCTION_ARGS)
pq_begintypsend(&buf);
pq_sendint8(&buf, version);
pq_sendtext(&buf, jtext->data, jtext->len);
- pfree(jtext->data);
- pfree(jtext);
+ destroyStringInfo(jtext);
PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
}
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index beecd0c2ac..3e4ca874d8 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -2163,8 +2163,7 @@ xml_errorHandler(void *data, PgXmlErrorPtr error)
appendBinaryStringInfo(&xmlerrcxt->err_buf, errorBuf->data,
errorBuf->len);
- pfree(errorBuf->data);
- pfree(errorBuf);
+ destroyStringInfo(errorBuf);
return;
}
@@ -2195,8 +2194,7 @@ xml_errorHandler(void *data, PgXmlErrorPtr error)
(errmsg_internal("%s", errorBuf->data)));
}
- pfree(errorBuf->data);
- pfree(errorBuf);
+ destroyStringInfo(errorBuf);
}
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index 31ead7f405..1f8a90fb9e 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -504,10 +504,7 @@ check_backup_label_files(int n_backups, char **backup_dirs)
/* Free memory that we don't need any more. */
if (lastbuf != buf)
- {
- pfree(buf->data);
- pfree(buf);
- }
+ destroyStringInfo(buf);
/*
* Return the data from the first backup_info that we read (which is the
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 6243426f5f..ec58109837 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -186,16 +186,10 @@ void
freeJsonLexContext(JsonLexContext *lex)
{
if (lex->flags & JSONLEX_FREE_STRVAL)
- {
- pfree(lex->strval->data);
- pfree(lex->strval);
- }
+ destroyStringInfo(lex->strval);
if (lex->errormsg)
- {
- pfree(lex->errormsg->data);
- pfree(lex->errormsg);
- }
+ destroyStringInfo(lex->errormsg);
if (lex->flags & JSONLEX_FREE_STRUCT)
pfree(lex);
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index c61d5c58f3..da1f3fb5ad 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -350,3 +350,19 @@ enlargeStringInfo(StringInfo str, int needed)
str->maxlen = newlen;
}
+
+/*
+ * destroyStringInfo
+ *
+ * Frees the StringInfo and its buffer (the opposite of makeStringInfo()). This
+ * must only be called on palloc'd StringInfos.
+ */
+void
+destroyStringInfo(StringInfo str)
+{
+ /* read-only StringInfos must not be destroyed */
+ Assert(str->maxlen != 0);
+
+ pfree(str->data);
+ pfree(str);
+}
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 2cd636b01c..156a83692d 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -87,7 +87,8 @@ typedef StringInfoData *StringInfo;
* to be len + 1 in size.
*
* To destroy a StringInfo, pfree() the data buffer, and then pfree() the
- * StringInfoData if it was palloc'd. There's no special support for this.
+ * StringInfoData if it was palloc'd. (For StringInfos created with
+ * makeStringInfo(), destroyStringInfo() is provided for this purpose.)
* However, if the StringInfo was initialized using initReadOnlyStringInfo()
* then the caller will need to consider if it is safe to pfree the data
* buffer.
@@ -233,4 +234,10 @@ extern void appendBinaryStringInfoNT(StringInfo str,
*/
extern void enlargeStringInfo(StringInfo str, int needed);
+/*------------------------
+ * destroyStringInfo
+ * Frees the StringInfo and its buffer (the opposite of makeStringInfo()).
+ */
+extern void destroyStringInfo(StringInfo str);
+
#endif /* STRINGINFO_H */
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index f1f6011ae0..76f01c73f5 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -1174,8 +1174,7 @@ psql_end_command(StringInfo buf, const char *database)
}
/* Clean up */
- pfree(buf->data);
- pfree(buf);
+ destroyStringInfo(buf);
}
/*
--
2.34.1
v2-0001-common-jsonapi-support-json_errdetail-in-FRONTEND.patchapplication/octet-stream; name=v2-0001-common-jsonapi-support-json_errdetail-in-FRONTEND.patchDownload
From 7b101e956e170e07d5cce340b2eefdfb0cf4fa68 Mon Sep 17 00:00:00 2001
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Tue, 12 Mar 2024 09:44:53 -0700
Subject: [PATCH v2 1/2] common/jsonapi: support json_errdetail in FRONTEND
json_errdetail() now allocates its error message inside memory owned by
the JsonLexContext, so clients don't need to worry about freeing it. We
can now partially revert b44669b2ca.
---
src/bin/pg_verifybackup/t/005_bad_manifest.pl | 3 +-
src/common/jsonapi.c | 119 +++++++++++-------
src/common/parse_manifest.c | 2 +-
src/include/common/jsonapi.h | 1 +
4 files changed, 75 insertions(+), 50 deletions(-)
diff --git a/src/bin/pg_verifybackup/t/005_bad_manifest.pl b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
index e278ccea5a..e2a297930e 100644
--- a/src/bin/pg_verifybackup/t/005_bad_manifest.pl
+++ b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
@@ -13,7 +13,8 @@ use Test::More;
my $tempdir = PostgreSQL::Test::Utils::tempdir;
test_bad_manifest('input string ended unexpectedly',
- qr/could not parse backup manifest: parsing failed/, <<EOM);
+ qr/could not parse backup manifest: The input string ended unexpectedly/,
+ <<EOM);
{
EOM
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 32931ded82..6243426f5f 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -170,14 +170,17 @@ makeJsonLexContextCstringLen(JsonLexContext *lex, char *json,
lex->strval = makeStringInfo();
lex->flags |= JSONLEX_FREE_STRVAL;
}
+ lex->errormsg = NULL;
return lex;
}
/*
- * Free memory in a JsonLexContext. There's no need for this if a *lex
- * pointer was given when the object was made and need_escapes was false,
- * or (in backend environment) a memory context delete/reset is imminent.
+ * Free memory in a JsonLexContext.
+ *
+ * There's no need for this if a *lex pointer was given when the object was
+ * made, need_escapes was false, and json_errdetail() was not called; or if (in
+ * backend environment) a memory context delete/reset is imminent.
*/
void
freeJsonLexContext(JsonLexContext *lex)
@@ -187,6 +190,13 @@ freeJsonLexContext(JsonLexContext *lex)
pfree(lex->strval->data);
pfree(lex->strval);
}
+
+ if (lex->errormsg)
+ {
+ pfree(lex->errormsg->data);
+ pfree(lex->errormsg);
+ }
+
if (lex->flags & JSONLEX_FREE_STRUCT)
pfree(lex);
}
@@ -1145,72 +1155,71 @@ report_parse_error(JsonParseContext ctx, JsonLexContext *lex)
return JSON_SUCCESS; /* silence stupider compilers */
}
-
-#ifndef FRONTEND
-/*
- * Extract the current token from a lexing context, for error reporting.
- */
-static char *
-extract_token(JsonLexContext *lex)
-{
- int toklen = lex->token_terminator - lex->token_start;
- char *token = palloc(toklen + 1);
-
- memcpy(token, lex->token_start, toklen);
- token[toklen] = '\0';
- return token;
-}
-
/*
* Construct an (already translated) detail message for a JSON error.
*
- * Note that the error message generated by this routine may not be
- * palloc'd, making it unsafe for frontend code as there is no way to
- * know if this can be safely pfree'd or not.
+ * The returned allocation is either static or owned by the JsonLexContext and
+ * should not be freed.
*/
char *
json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
{
+ if (lex->errormsg)
+ resetStringInfo(lex->errormsg);
+ else
+ lex->errormsg = makeStringInfo();
+
+ /*
+ * A helper for error messages that should print the current token. format
+ * must contain exactly one %.*s specifier.
+ */
+#define token_error(lex, format) \
+ appendStringInfo((lex)->errormsg, _(format), \
+ (int) ((lex)->token_terminator - (lex)->token_start), \
+ (lex)->token_start);
+
switch (error)
{
case JSON_SUCCESS:
/* fall through to the error code after switch */
break;
case JSON_ESCAPING_INVALID:
- return psprintf(_("Escape sequence \"\\%s\" is invalid."),
- extract_token(lex));
+ token_error(lex, "Escape sequence \"\\%.*s\" is invalid.");
+ break;
case JSON_ESCAPING_REQUIRED:
- return psprintf(_("Character with value 0x%02x must be escaped."),
- (unsigned char) *(lex->token_terminator));
+ appendStringInfo(lex->errormsg,
+ _("Character with value 0x%02x must be escaped."),
+ (unsigned char) *(lex->token_terminator));
+ break;
case JSON_EXPECTED_END:
- return psprintf(_("Expected end of input, but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected end of input, but found \"%.*s\".");
+ break;
case JSON_EXPECTED_ARRAY_FIRST:
- return psprintf(_("Expected array element or \"]\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected array element or \"]\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_ARRAY_NEXT:
- return psprintf(_("Expected \",\" or \"]\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected \",\" or \"]\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_COLON:
- return psprintf(_("Expected \":\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected \":\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_JSON:
- return psprintf(_("Expected JSON value, but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected JSON value, but found \"%.*s\".");
+ break;
case JSON_EXPECTED_MORE:
return _("The input string ended unexpectedly.");
case JSON_EXPECTED_OBJECT_FIRST:
- return psprintf(_("Expected string or \"}\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected string or \"}\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_OBJECT_NEXT:
- return psprintf(_("Expected \",\" or \"}\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected \",\" or \"}\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_STRING:
- return psprintf(_("Expected string, but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected string, but found \"%.*s\".");
+ break;
case JSON_INVALID_TOKEN:
- return psprintf(_("Token \"%s\" is invalid."),
- extract_token(lex));
+ token_error(lex, "Token \"%.*s\" is invalid.");
+ break;
case JSON_UNICODE_CODE_POINT_ZERO:
return _("\\u0000 cannot be converted to text.");
case JSON_UNICODE_ESCAPE_FORMAT:
@@ -1219,9 +1228,19 @@ json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
/* note: this case is only reachable in frontend not backend */
return _("Unicode escape values cannot be used for code point values above 007F when the encoding is not UTF8.");
case JSON_UNICODE_UNTRANSLATABLE:
- /* note: this case is only reachable in backend not frontend */
+
+ /*
+ * note: this case is only reachable in backend not frontend.
+ * #ifdef it away so the frontend doesn't try to link against
+ * backend functionality.
+ */
+#ifndef FRONTEND
return psprintf(_("Unicode escape value could not be translated to the server's encoding %s."),
GetDatabaseEncodingName());
+#else
+ Assert(false);
+ break;
+#endif
case JSON_UNICODE_HIGH_SURROGATE:
return _("Unicode high surrogate must not follow a high surrogate.");
case JSON_UNICODE_LOW_SURROGATE:
@@ -1230,13 +1249,17 @@ json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
/* fall through to the error code after switch */
break;
}
+#undef token_error
/*
* We don't use a default: case, so that the compiler will warn about
* unhandled enum values. But this needs to be here anyway to cover the
* possibility of an incorrect input.
*/
- elog(ERROR, "unexpected json parse error type: %d", (int) error);
- return NULL;
+ if (lex->errormsg->len == 0)
+ appendStringInfo(lex->errormsg,
+ _("unexpected json parse error type: %d"),
+ (int) error);
+
+ return lex->errormsg->data;
}
-#endif
diff --git a/src/common/parse_manifest.c b/src/common/parse_manifest.c
index 92a97714f3..62d93989be 100644
--- a/src/common/parse_manifest.c
+++ b/src/common/parse_manifest.c
@@ -147,7 +147,7 @@ json_parse_manifest(JsonManifestParseContext *context, char *buffer,
/* Run the actual JSON parser. */
json_error = pg_parse_json(lex, &sem);
if (json_error != JSON_SUCCESS)
- json_manifest_parse_failure(context, "parsing failed");
+ json_manifest_parse_failure(context, json_errdetail(json_error, lex));
if (parse.state != JM_EXPECT_EOF)
json_manifest_parse_failure(context, "manifest ended unexpectedly");
diff --git a/src/include/common/jsonapi.h b/src/include/common/jsonapi.h
index 02943cdad8..86a0fc2d00 100644
--- a/src/include/common/jsonapi.h
+++ b/src/include/common/jsonapi.h
@@ -89,6 +89,7 @@ typedef struct JsonLexContext
int line_number; /* line number, starting from 1 */
char *line_start; /* where that line starts within input */
StringInfo strval;
+ StringInfo errormsg;
} JsonLexContext;
typedef JsonParseErrorType (*json_struct_action) (void *state);
--
2.34.1
On Wed, Mar 13, 2024 at 11:20:16AM -0700, Jacob Champion wrote:
Sounds good, split into v2-0002. (That gives me room to switch other
call sites to the new API, too.) Thanks both!
That looks OK to me. I can see 7~8 remaining sites where StringInfo
data is freed, like in the syslogger, but we should not touch the
StringInfo.
case JSON_EXPECTED_STRING: - return psprintf(_("Expected string, but found \"%s\"."), - extract_token(lex)); + appendStringInfo(lex->errormsg, + _("Expected string, but found \"%.*s\"."), + toklen, lex->token_start);Maybe this could be wrapped in a macro to avoid copying around
token_start and toklen for all the error cases.I gave it a shot in 0001; see what you think.
That's cleaner, thanks.
Hmm, I think that it is cleaner to create the new API first, and then
rely on it, reversing the order of both patches (perhaps the extra
destroyStringInfo in freeJsonLexContext() should have been moved in
0001). See the attached with few tweaks to 0001, previously 0002 @-@.
I'd still need to do a more serious lookup of 0002, previously 0001.
--
Michael
Attachments:
v3-0001-Add-a-helper-function-for-cleaning-up-StringInfos.patchtext/x-diff; charset=us-asciiDownload
From 78bfbfe3cbdfd4095bc9ef220dfa11dfdc404556 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 14 Mar 2024 16:58:06 +0900
Subject: [PATCH v3 1/2] Add a helper function for cleaning up StringInfos
destroyStringInfo() is a counterpart to makeStringInfo(): it frees a
palloc'd StringInfo and its data. API originally by Daniel Gustafsson,
with docs and tweaks added by Jacob Champion.
Co-authored-by: Daniel Gustafsson <daniel@yesql.se>
---
src/include/lib/stringinfo.h | 9 ++++++++-
src/backend/backup/basebackup.c | 3 +--
src/backend/commands/subscriptioncmds.c | 3 +--
src/backend/utils/adt/jsonb.c | 3 +--
src/backend/utils/adt/xml.c | 6 ++----
src/common/stringinfo.c | 18 +++++++++++++++++-
src/bin/pg_combinebackup/pg_combinebackup.c | 5 +----
src/test/regress/pg_regress.c | 3 +--
8 files changed, 32 insertions(+), 18 deletions(-)
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 2cd636b01c..cd9632e3fc 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -87,7 +87,8 @@ typedef StringInfoData *StringInfo;
* to be len + 1 in size.
*
* To destroy a StringInfo, pfree() the data buffer, and then pfree() the
- * StringInfoData if it was palloc'd. There's no special support for this.
+ * StringInfoData if it was palloc'd. For StringInfos created with
+ * makeStringInfo(), destroyStringInfo() is provided for this purpose.
* However, if the StringInfo was initialized using initReadOnlyStringInfo()
* then the caller will need to consider if it is safe to pfree the data
* buffer.
@@ -233,4 +234,10 @@ extern void appendBinaryStringInfoNT(StringInfo str,
*/
extern void enlargeStringInfo(StringInfo str, int needed);
+/*------------------------
+ * destroyStringInfo
+ * Frees a StringInfo and its buffer (opposite of makeStringInfo()).
+ */
+extern void destroyStringInfo(StringInfo str);
+
#endif /* STRINGINFO_H */
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 5fbbe5ffd2..5fea86ad0f 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -397,8 +397,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
endtli = backup_state->stoptli;
/* Deallocate backup-related variables. */
- pfree(tablespace_map->data);
- pfree(tablespace_map);
+ destroyStringInfo(tablespace_map);
pfree(backup_state);
}
PG_END_ENSURE_ERROR_CLEANUP(do_pg_abort_backup, BoolGetDatum(false));
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index a05d69922d..5a47fa984d 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -506,8 +506,7 @@ check_publications(WalReceiverConn *wrconn, List *publications)
appendStringInfoChar(cmd, ')');
res = walrcv_exec(wrconn, cmd->data, 1, tableRow);
- pfree(cmd->data);
- pfree(cmd);
+ destroyStringInfo(cmd);
if (res->status != WALRCV_OK_TUPLES)
ereport(ERROR,
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index a5e48744ac..a941654d5a 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -133,8 +133,7 @@ jsonb_send(PG_FUNCTION_ARGS)
pq_begintypsend(&buf);
pq_sendint8(&buf, version);
pq_sendtext(&buf, jtext->data, jtext->len);
- pfree(jtext->data);
- pfree(jtext);
+ destroyStringInfo(jtext);
PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
}
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index beecd0c2ac..3e4ca874d8 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -2163,8 +2163,7 @@ xml_errorHandler(void *data, PgXmlErrorPtr error)
appendBinaryStringInfo(&xmlerrcxt->err_buf, errorBuf->data,
errorBuf->len);
- pfree(errorBuf->data);
- pfree(errorBuf);
+ destroyStringInfo(errorBuf);
return;
}
@@ -2195,8 +2194,7 @@ xml_errorHandler(void *data, PgXmlErrorPtr error)
(errmsg_internal("%s", errorBuf->data)));
}
- pfree(errorBuf->data);
- pfree(errorBuf);
+ destroyStringInfo(errorBuf);
}
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index c61d5c58f3..2910799bfd 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -70,7 +70,7 @@ initStringInfo(StringInfo str)
*
* Reset the StringInfo: the data buffer remains valid, but its
* previous content, if any, is cleared.
- *
+*
* Read-only StringInfos as initialized by initReadOnlyStringInfo cannot be
* reset.
*/
@@ -350,3 +350,19 @@ enlargeStringInfo(StringInfo str, int needed)
str->maxlen = newlen;
}
+
+/*
+ * destroyStringInfo
+ *
+ * Frees a StringInfo and its buffer (opposite of makeStringInfo()).
+ * This must only be called on palloc'd StringInfos.
+ */
+void
+destroyStringInfo(StringInfo str)
+{
+ /* don't allow destroys of read-only StringInfos */
+ Assert(str->maxlen != 0);
+
+ pfree(str->data);
+ pfree(str);
+}
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index 6f0814d9ac..74f8be9eea 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -526,10 +526,7 @@ check_backup_label_files(int n_backups, char **backup_dirs)
/* Free memory that we don't need any more. */
if (lastbuf != buf)
- {
- pfree(buf->data);
- pfree(buf);
- }
+ destroyStringInfo(buf);
/*
* Return the data from the first backup_info that we read (which is the
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index f1f6011ae0..76f01c73f5 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -1174,8 +1174,7 @@ psql_end_command(StringInfo buf, const char *database)
}
/* Clean up */
- pfree(buf->data);
- pfree(buf);
+ destroyStringInfo(buf);
}
/*
--
2.43.0
v3-0002-common-jsonapi-support-json_errdetail-in-FRONTEND.patchtext/x-diff; charset=us-asciiDownload
From 348b32ff2d3886783c173b4b6d721bfabbf9798b Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 14 Mar 2024 17:00:33 +0900
Subject: [PATCH v3 2/2] common/jsonapi: support json_errdetail in FRONTEND
json_errdetail() now allocates its error message inside memory owned by
the JsonLexContext, so clients don't need to worry about freeing it. We
can now partially revert b44669b2ca.
---
src/include/common/jsonapi.h | 1 +
src/common/jsonapi.c | 121 ++++++++++--------
src/common/parse_manifest.c | 2 +-
src/bin/pg_verifybackup/t/005_bad_manifest.pl | 3 +-
4 files changed, 73 insertions(+), 54 deletions(-)
diff --git a/src/include/common/jsonapi.h b/src/include/common/jsonapi.h
index 02943cdad8..86a0fc2d00 100644
--- a/src/include/common/jsonapi.h
+++ b/src/include/common/jsonapi.h
@@ -89,6 +89,7 @@ typedef struct JsonLexContext
int line_number; /* line number, starting from 1 */
char *line_start; /* where that line starts within input */
StringInfo strval;
+ StringInfo errormsg;
} JsonLexContext;
typedef JsonParseErrorType (*json_struct_action) (void *state);
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 32931ded82..ec58109837 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -170,23 +170,27 @@ makeJsonLexContextCstringLen(JsonLexContext *lex, char *json,
lex->strval = makeStringInfo();
lex->flags |= JSONLEX_FREE_STRVAL;
}
+ lex->errormsg = NULL;
return lex;
}
/*
- * Free memory in a JsonLexContext. There's no need for this if a *lex
- * pointer was given when the object was made and need_escapes was false,
- * or (in backend environment) a memory context delete/reset is imminent.
+ * Free memory in a JsonLexContext.
+ *
+ * There's no need for this if a *lex pointer was given when the object was
+ * made, need_escapes was false, and json_errdetail() was not called; or if (in
+ * backend environment) a memory context delete/reset is imminent.
*/
void
freeJsonLexContext(JsonLexContext *lex)
{
if (lex->flags & JSONLEX_FREE_STRVAL)
- {
- pfree(lex->strval->data);
- pfree(lex->strval);
- }
+ destroyStringInfo(lex->strval);
+
+ if (lex->errormsg)
+ destroyStringInfo(lex->errormsg);
+
if (lex->flags & JSONLEX_FREE_STRUCT)
pfree(lex);
}
@@ -1145,72 +1149,71 @@ report_parse_error(JsonParseContext ctx, JsonLexContext *lex)
return JSON_SUCCESS; /* silence stupider compilers */
}
-
-#ifndef FRONTEND
-/*
- * Extract the current token from a lexing context, for error reporting.
- */
-static char *
-extract_token(JsonLexContext *lex)
-{
- int toklen = lex->token_terminator - lex->token_start;
- char *token = palloc(toklen + 1);
-
- memcpy(token, lex->token_start, toklen);
- token[toklen] = '\0';
- return token;
-}
-
/*
* Construct an (already translated) detail message for a JSON error.
*
- * Note that the error message generated by this routine may not be
- * palloc'd, making it unsafe for frontend code as there is no way to
- * know if this can be safely pfree'd or not.
+ * The returned allocation is either static or owned by the JsonLexContext and
+ * should not be freed.
*/
char *
json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
{
+ if (lex->errormsg)
+ resetStringInfo(lex->errormsg);
+ else
+ lex->errormsg = makeStringInfo();
+
+ /*
+ * A helper for error messages that should print the current token. format
+ * must contain exactly one %.*s specifier.
+ */
+#define token_error(lex, format) \
+ appendStringInfo((lex)->errormsg, _(format), \
+ (int) ((lex)->token_terminator - (lex)->token_start), \
+ (lex)->token_start);
+
switch (error)
{
case JSON_SUCCESS:
/* fall through to the error code after switch */
break;
case JSON_ESCAPING_INVALID:
- return psprintf(_("Escape sequence \"\\%s\" is invalid."),
- extract_token(lex));
+ token_error(lex, "Escape sequence \"\\%.*s\" is invalid.");
+ break;
case JSON_ESCAPING_REQUIRED:
- return psprintf(_("Character with value 0x%02x must be escaped."),
- (unsigned char) *(lex->token_terminator));
+ appendStringInfo(lex->errormsg,
+ _("Character with value 0x%02x must be escaped."),
+ (unsigned char) *(lex->token_terminator));
+ break;
case JSON_EXPECTED_END:
- return psprintf(_("Expected end of input, but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected end of input, but found \"%.*s\".");
+ break;
case JSON_EXPECTED_ARRAY_FIRST:
- return psprintf(_("Expected array element or \"]\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected array element or \"]\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_ARRAY_NEXT:
- return psprintf(_("Expected \",\" or \"]\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected \",\" or \"]\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_COLON:
- return psprintf(_("Expected \":\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected \":\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_JSON:
- return psprintf(_("Expected JSON value, but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected JSON value, but found \"%.*s\".");
+ break;
case JSON_EXPECTED_MORE:
return _("The input string ended unexpectedly.");
case JSON_EXPECTED_OBJECT_FIRST:
- return psprintf(_("Expected string or \"}\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected string or \"}\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_OBJECT_NEXT:
- return psprintf(_("Expected \",\" or \"}\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected \",\" or \"}\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_STRING:
- return psprintf(_("Expected string, but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected string, but found \"%.*s\".");
+ break;
case JSON_INVALID_TOKEN:
- return psprintf(_("Token \"%s\" is invalid."),
- extract_token(lex));
+ token_error(lex, "Token \"%.*s\" is invalid.");
+ break;
case JSON_UNICODE_CODE_POINT_ZERO:
return _("\\u0000 cannot be converted to text.");
case JSON_UNICODE_ESCAPE_FORMAT:
@@ -1219,9 +1222,19 @@ json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
/* note: this case is only reachable in frontend not backend */
return _("Unicode escape values cannot be used for code point values above 007F when the encoding is not UTF8.");
case JSON_UNICODE_UNTRANSLATABLE:
- /* note: this case is only reachable in backend not frontend */
+
+ /*
+ * note: this case is only reachable in backend not frontend.
+ * #ifdef it away so the frontend doesn't try to link against
+ * backend functionality.
+ */
+#ifndef FRONTEND
return psprintf(_("Unicode escape value could not be translated to the server's encoding %s."),
GetDatabaseEncodingName());
+#else
+ Assert(false);
+ break;
+#endif
case JSON_UNICODE_HIGH_SURROGATE:
return _("Unicode high surrogate must not follow a high surrogate.");
case JSON_UNICODE_LOW_SURROGATE:
@@ -1230,13 +1243,17 @@ json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
/* fall through to the error code after switch */
break;
}
+#undef token_error
/*
* We don't use a default: case, so that the compiler will warn about
* unhandled enum values. But this needs to be here anyway to cover the
* possibility of an incorrect input.
*/
- elog(ERROR, "unexpected json parse error type: %d", (int) error);
- return NULL;
+ if (lex->errormsg->len == 0)
+ appendStringInfo(lex->errormsg,
+ _("unexpected json parse error type: %d"),
+ (int) error);
+
+ return lex->errormsg->data;
}
-#endif
diff --git a/src/common/parse_manifest.c b/src/common/parse_manifest.c
index 3f6d1dfd3d..40ec3b4f58 100644
--- a/src/common/parse_manifest.c
+++ b/src/common/parse_manifest.c
@@ -152,7 +152,7 @@ json_parse_manifest(JsonManifestParseContext *context, char *buffer,
/* Run the actual JSON parser. */
json_error = pg_parse_json(lex, &sem);
if (json_error != JSON_SUCCESS)
- json_manifest_parse_failure(context, "parsing failed");
+ json_manifest_parse_failure(context, json_errdetail(json_error, lex));
if (parse.state != JM_EXPECT_EOF)
json_manifest_parse_failure(context, "manifest ended unexpectedly");
diff --git a/src/bin/pg_verifybackup/t/005_bad_manifest.pl b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
index 77fdfbb9d0..2068babac5 100644
--- a/src/bin/pg_verifybackup/t/005_bad_manifest.pl
+++ b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
@@ -13,7 +13,8 @@ use Test::More;
my $tempdir = PostgreSQL::Test::Utils::tempdir;
test_bad_manifest('input string ended unexpectedly',
- qr/could not parse backup manifest: parsing failed/, <<EOM);
+ qr/could not parse backup manifest: The input string ended unexpectedly/,
+ <<EOM);
{
EOM
--
2.43.0
On 14 Mar 2024, at 09:06, Michael Paquier <michael@paquier.xyz> wrote:
I think that it is cleaner to create the new API first, and then
rely on it, reversing the order of both patches
I agree with this ordering.
(perhaps the extra destroyStringInfo in freeJsonLexContext() should
have been moved in 0001).
I wouldn't worry about that, seems fine as is to me.
See the attached with few tweaks to 0001, previously 0002 @-@.
I'd still need to do a more serious lookup of 0002, previously 0001.
A few small comments:
- *
+*
Whitespace+ /* don't allow destroys of read-only StringInfos */
+ Assert(str->maxlen != 0);
Considering that StringInfo.c don't own the memory here I think it's warranted
to turn this assert into an elog() to avoid the risk of use-after-free bugs.+ * The returned allocation is either static or owned by the JsonLexContext and
+ * should not be freed.
The most important part of that comment is at the very end, to help readers I
would reword this to just "The returned pointer should not be freed.", or at
least put that part first.+#define token_error(lex, format) \
I'm not sure this buys much more than reduced LoC, the expansion isn't
unreadable to the point that the format constraint encoded in the macro is
worth it IMO.
--
Daniel Gustafsson
On Thu, Mar 14, 2024 at 10:56:46AM +0100, Daniel Gustafsson wrote:
+ /* don't allow destroys of read-only StringInfos */ + Assert(str->maxlen != 0); Considering that StringInfo.c don't own the memory here I think it's warranted to turn this assert into an elog() to avoid the risk of use-after-free bugs.
Hmm. I am not sure how much protection this would offer, TBH. One
thing that I find annoying with common/stringinfo.c as it is currently
is that we have two exit() calls in the enlarge path, and it does not
seem wise to me to spread that even more.
My last argument sounds like a nit for HEAD knowing that this does not
impact libpq that has its own pqexpbuffer.c to avoid issues with
palloc, elog and exit, but that could be a problem if OAuth relies
more on these code paths in libpq.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
Hmm. I am not sure how much protection this would offer, TBH. One
thing that I find annoying with common/stringinfo.c as it is currently
is that we have two exit() calls in the enlarge path, and it does not
seem wise to me to spread that even more.
My last argument sounds like a nit for HEAD knowing that this does not
impact libpq that has its own pqexpbuffer.c to avoid issues with
palloc, elog and exit, but that could be a problem if OAuth relies
more on these code paths in libpq.
I hope nobody is expecting that such code will get accepted. We have
a policy (and an enforcement mechanism) that libpq.so must not call
exit(). OAuth code in libpq will need to cope with using pqexpbuffer.
regards, tom lane
On 2024-Mar-14, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
Hmm. I am not sure how much protection this would offer, TBH. One
thing that I find annoying with common/stringinfo.c as it is currently
is that we have two exit() calls in the enlarge path, and it does not
seem wise to me to spread that even more.
I hope nobody is expecting that such code will get accepted. We have
a policy (and an enforcement mechanism) that libpq.so must not call
exit(). OAuth code in libpq will need to cope with using pqexpbuffer.
FWIW that's exactly what Jacob's OAUTH patch does -- it teaches the
relevant JSON parser code to use pqexpbuffer when in frontend
environment, and continues to use StringInfo in backend. I find that a
bit cumbersome, but if the idea of changing the StringInfo behavior
(avoiding exit()) is too radical, then perhaps it's better that we go
with Jacob's proposal in the other thread:
+/*
+ * In backend, we will use palloc/pfree along with StringInfo. In frontend, use
+ * malloc and PQExpBuffer, and return JSON_OUT_OF_MEMORY on out-of-memory.
+ */
+#ifdef FRONTEND
+
+#define STRDUP(s) strdup(s)
+#define ALLOC(size) malloc(size)
+
+#define appendStrVal appendPQExpBuffer
+#define appendBinaryStrVal appendBinaryPQExpBuffer
+#define appendStrValChar appendPQExpBufferChar
+#define createStrVal createPQExpBuffer
+#define resetStrVal resetPQExpBuffer
+#define destroyStrVal destroyPQExpBuffer
+
+#else /* !FRONTEND */
+
+#define STRDUP(s) pstrdup(s)
+#define ALLOC(size) palloc(size)
+
+#define appendStrVal appendStringInfo
+#define appendBinaryStrVal appendBinaryStringInfo
+#define appendStrValChar appendStringInfoChar
+#define createStrVal makeStringInfo
+#define resetStrVal resetStringInfo
+#define destroyStrVal destroyStringInfo
+
+#endif--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Ni aún el genio muy grande llegaría muy lejos
si tuviera que sacarlo todo de su propio interior" (Goethe)
On 15 Mar 2024, at 09:38, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2024-Mar-14, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
Hmm. I am not sure how much protection this would offer, TBH. One
thing that I find annoying with common/stringinfo.c as it is currently
is that we have two exit() calls in the enlarge path, and it does not
seem wise to me to spread that even more.I hope nobody is expecting that such code will get accepted. We have
a policy (and an enforcement mechanism) that libpq.so must not call
exit(). OAuth code in libpq will need to cope with using pqexpbuffer.FWIW that's exactly what Jacob's OAUTH patch does -- it teaches the
relevant JSON parser code to use pqexpbuffer when in frontend
environment, and continues to use StringInfo in backend. I find that a
bit cumbersome, but if the idea of changing the StringInfo behavior
(avoiding exit()) is too radical, then perhaps it's better that we go
with Jacob's proposal in the other thread:
Correct, the OAuth work does not make any claims to use StringInfo in libpq.
My understanding of this thread was to make it use StringInfo for now to get
this available for frontend binaries now, and reduce scope here, and later
change it up if/when the OAuth patch lands.
--
Daniel Gustafsson
On 15 Mar 2024, at 01:10, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Mar 14, 2024 at 10:56:46AM +0100, Daniel Gustafsson wrote:
+ /* don't allow destroys of read-only StringInfos */ + Assert(str->maxlen != 0); Considering that StringInfo.c don't own the memory here I think it's warranted to turn this assert into an elog() to avoid the risk of use-after-free bugs.Hmm. I am not sure how much protection this would offer, TBH.
I can't see how refusing to free memory owned and controlled by someone else,
and throwing an error if attempted, wouldn't be a sound defensive programming
measure.
--
Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
I can't see how refusing to free memory owned and controlled by someone else,
and throwing an error if attempted, wouldn't be a sound defensive programming
measure.
I think the argument is about what "refusal" looks like.
An Assert seems OK to me, but anything based on elog is
likely to be problematic, because it'll involve exit()
somewhere.
regards, tom lane
On Fri, Mar 15, 2024 at 10:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
I can't see how refusing to free memory owned and controlled by someone
else,
and throwing an error if attempted, wouldn't be a sound defensive
programming
measure.
I think the argument is about what "refusal" looks like.
An Assert seems OK to me, but anything based on elog is
likely to be problematic, because it'll involve exit()
somewhere.
Yeah, I agree an Assert seems safest here.
I'd like to get this done ASAP so I can rebase my incremental parse
patchset. Daniel, do you want to commit it? If not, I can.
cheers
andrew
On 15 Mar 2024, at 21:56, Andrew Dunstan <andrew@dunslane.net> wrote:
On Fri, Mar 15, 2024 at 10:15 AM Tom Lane <tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>> wrote:
Daniel Gustafsson <daniel@yesql.se <mailto:daniel@yesql.se>> writes:I can't see how refusing to free memory owned and controlled by someone else,
and throwing an error if attempted, wouldn't be a sound defensive programming
measure.I think the argument is about what "refusal" looks like.
An Assert seems OK to me, but anything based on elog is
likely to be problematic, because it'll involve exit()
somewhere.Yeah, I agree an Assert seems safest here.
I'd like to get this done ASAP so I can rebase my incremental parse patchset. Daniel, do you want to commit it? If not, I can.
Sure, I can commit these patches. It won't be until tomorrow though since I
prefer to have ample time to monitor the buildfarm, if you are in a bigger rush
than that then feel free to go ahead.
--
Daniel Gustafsson
On Fri, Mar 15, 2024 at 11:23:07PM +0100, Daniel Gustafsson wrote:
On 15 Mar 2024, at 21:56, Andrew Dunstan <andrew@dunslane.net> wrote:
On Fri, Mar 15, 2024 at 10:15 AM Tom Lane <tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>> wrote:
Yeah, I agree an Assert seems safest here.
Cool.
I'd like to get this done ASAP so I can rebase my incremental parse
patchset. Daniel, do you want to commit it? If not, I can.Sure, I can commit these patches. It won't be until tomorrow though since I
prefer to have ample time to monitor the buildfarm, if you are in a bigger rush
than that then feel free to go ahead.
+1. I've not looked much at 0002, but feel free to do so if you think
both are good for shipping.
--
MichaelOn Mar 16, 2024, at 8:53 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
On 15 Mar 2024, at 21:56, Andrew Dunstan <andrew@dunslane.net> wrote:
On Fri, Mar 15, 2024 at 10:15 AM Tom Lane <tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>> wrote:
Daniel Gustafsson <daniel@yesql.se <mailto:daniel@yesql.se>> writes:I can't see how refusing to free memory owned and controlled by someone else,
and throwing an error if attempted, wouldn't be a sound defensive programming
measure.I think the argument is about what "refusal" looks like.
An Assert seems OK to me, but anything based on elog is
likely to be problematic, because it'll involve exit()
somewhere.Yeah, I agree an Assert seems safest here.
I'd like to get this done ASAP so I can rebase my incremental parse patchset. Daniel, do you want to commit it? If not, I can.
Sure, I can commit these patches. It won't be until tomorrow though since I
prefer to have ample time to monitor the buildfarm, if you are in a bigger rush
than that then feel free to go ahead.
tomorrow will be fine, thanks
Cheers
Andrew
On 16 Mar 2024, at 00:59, Andrew Dunstan <andrew@dunslane.net> wrote:
On Mar 16, 2024, at 8:53 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
On 15 Mar 2024, at 21:56, Andrew Dunstan <andrew@dunslane.net> wrote:
I'd like to get this done ASAP so I can rebase my incremental parse patchset. Daniel, do you want to commit it? If not, I can.
Sure, I can commit these patches. It won't be until tomorrow though since I
prefer to have ample time to monitor the buildfarm, if you are in a bigger rush
than that then feel free to go ahead.tomorrow will be fine, thanks
Sorry, I ran into some unforeseen scheduling issues and had less time available
than planned. I have pushed the 0001 StringInfo patch to reduce the work for
tomorrow when I will work on 0002 unless beaten to it.
--
Daniel Gustafsson
On 17 Mar 2024, at 00:00, Daniel Gustafsson <daniel@yesql.se> wrote:
On 16 Mar 2024, at 00:59, Andrew Dunstan <andrew@dunslane.net> wrote:
On Mar 16, 2024, at 8:53 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
On 15 Mar 2024, at 21:56, Andrew Dunstan <andrew@dunslane.net> wrote:
I'd like to get this done ASAP so I can rebase my incremental parse patchset. Daniel, do you want to commit it? If not, I can.
Sure, I can commit these patches. It won't be until tomorrow though since I
prefer to have ample time to monitor the buildfarm, if you are in a bigger rush
than that then feel free to go ahead.tomorrow will be fine, thanks
Sorry, I ran into some unforeseen scheduling issues and had less time available
than planned. I have pushed the 0001 StringInfo patch to reduce the work for
tomorrow when I will work on 0002 unless beaten to it.
I took another look at this tonight and committed it with some mostly cosmetic
changes. Since then, tamandua failed the SSL test on this commit but I am
unable to reproduce any error both on older OpenSSL and matching the 3.1
version on tamandua, so not sure if its related. Other animals have cleared
sslcheck after this, but looking at this highlights just how rare it is for a
buildfarm animal to run sslcheck =/
--
Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
I took another look at this tonight and committed it with some mostly cosmetic
changes. Since then, tamandua failed the SSL test on this commit but I am
unable to reproduce any error both on older OpenSSL and matching the 3.1
version on tamandua, so not sure if its related.
That failure looks like just a random buildfarm burp:
2024-03-17 23:11:30.989 UTC [106988][postmaster][:0] LOG: starting PostgreSQL 17devel on x86_64-linux, compiled by gcc-12.3.0, 64-bit
2024-03-17 23:11:30.989 UTC [106988][postmaster][:0] LOG: could not bind IPv4 address "127.0.0.1": Address already in use
2024-03-17 23:11:30.989 UTC [106988][postmaster][:0] HINT: Is another postmaster already running on port 54588? If not, wait a few seconds and retry.
2024-03-17 23:11:30.990 UTC [106988][postmaster][:0] WARNING: could not create listen socket for "127.0.0.1"
2024-03-17 23:11:30.990 UTC [106988][postmaster][:0] FATAL: could not create any TCP/IP sockets
2024-03-17 23:11:30.993 UTC [106988][postmaster][:0] LOG: database system is shut down
regards, tom lane
On 18 Mar 2024, at 01:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
I took another look at this tonight and committed it with some mostly cosmetic
changes. Since then, tamandua failed the SSL test on this commit but I am
unable to reproduce any error both on older OpenSSL and matching the 3.1
version on tamandua, so not sure if its related.That failure looks like just a random buildfarm burp:
Indeed, and it corrected itself a few hours later.
--
Daniel Gustafsson
On Sun, Mar 17, 2024 at 4:49 PM Daniel Gustafsson <daniel@yesql.se> wrote:
I took another look at this tonight and committed it with some mostly cosmetic
changes.
Great! Thanks everyone.
--Jacob