Date: Thu, 24 Jun 2021 14:56:04 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Jacob Champion <pchampion@vmware.com>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Make jsonapi usable from libpq
Message-ID: <YNQedHwheUaTnnGt@paquier.xyz>
References: <a250d475ba1c0cc0efb7dfec8e538fcc77cdcb8e.camel@vmware.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="7OnI4oC4vBBEqBIk"
Content-Disposition: inline
In-Reply-To: <a250d475ba1c0cc0efb7dfec8e538fcc77cdcb8e.camel@vmware.com>
Archived-At: 
 <https://www.postgresql.org/message-id/YNQedHwheUaTnnGt%40paquier.xyz>
Precedence: bulk


--7OnI4oC4vBBEqBIk
Content-Type: multipart/mixed; boundary="9tPkd5fmao84BUcK"
Content-Disposition: inline


--9tPkd5fmao84BUcK
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Jun 22, 2021 at 10:59:37PM +0000, Jacob Champion wrote:
> Hmm. I'm honestly hesitant to start splitting files apart, mostly
> because json_log_and_abort() is only called from two places, and both
> those places are triggered by programmer error as opposed to user
> error.
>=20
> Would it make more sense to switch to an fprintf-and-abort case, to
> match the approach taken by PGTHREAD_ERROR and the out-of-memory
> conditions in fe-print.c? Or is there already precedent for handling
> can't-happen code paths with in-band errors, through the PGconn?

Not really..

Looking more closely at that, I actually find a bit crazy the
requirement for any logging within jsonapi.c just to cope with the
fact that json_errdetail() and report_parse_error() just want to track
down if the caller is giving some garbage or not, which should never
be the case, really.  So I would be tempted to eliminate this
dependency to begin with.

The second thing is how we should try to handle the way the error
message gets allocated in json_errdetail().  libpq cannot rely on
psprintf(), so I can think about two options here:
- Let the caller of json_errdetail() allocate the memory area of the
error message by itself, pass it down to the function.
- Do the allocation within json_errdetail(), and let callers cope the
case where json_errdetail() itself fails on OOM for any frontend code
using it.

Looking at HEAD, the OAUTH patch and the token handling, the second
option looks more interesting.  I have to admit that handling the
token part makes the patch a bit special, but that avoids duplicating
all those error messages for libpq.  Please see the idea as attached.
--
Michael

--9tPkd5fmao84BUcK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="jsonapi-libpq.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/include/common/jsonapi.h b/src/include/common/jsonapi.h
index ec3dfce9c3..f5e25ca3e3 100644
--- a/src/include/common/jsonapi.h
+++ b/src/include/common/jsonapi.h
@@ -48,6 +48,7 @@ typedef enum
 	JSON_EXPECTED_OBJECT_NEXT,
 	JSON_EXPECTED_STRING,
 	JSON_INVALID_TOKEN,
+	JSON_OUT_OF_MEMORY,
 	JSON_UNICODE_CODE_POINT_ZERO,
 	JSON_UNICODE_ESCAPE_FORMAT,
 	JSON_UNICODE_HIGH_ESCAPE,
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index d376ab152d..cf70a735cc 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -19,19 +19,22 @@
=20
 #include "common/jsonapi.h"
 #include "mb/pg_wchar.h"
-
-#ifdef FRONTEND
-#include "common/logging.h"
-#else
+#ifndef FRONTEND
 #include "miscadmin.h"
 #endif
=20
-#ifdef FRONTEND
-#define check_stack_depth()
-#define json_log_and_abort(...) \
-	do { pg_log_fatal(__VA_ARGS__); exit(1); } while(0)
+/*
+ * In backend, we will use palloc/pfree.  In frontend, use malloc, and
+ * return JSON_OUT_OF_MEMORY on out-of-memory.
+ */
+#ifndef FRONTEND
+#define STRDUP(s) pstrdup(s)
+#define ALLOC(size) palloc(size)
+#define FREE(s) pfree(s)
 #else
-#define json_log_and_abort(...) elog(ERROR, __VA_ARGS__)
+#define STRDUP(s) strdup(s)
+#define ALLOC(size) malloc(size)
+#define FREE(s) free(s)
 #endif
=20
 /*
@@ -291,12 +294,17 @@ parse_scalar(JsonLexContext *lex, JsonSemAction *sem)
 	if (lex_peek(lex) =3D=3D JSON_TOKEN_STRING)
 	{
 		if (lex->strval !=3D NULL)
-			val =3D pstrdup(lex->strval->data);
+		{
+			val =3D STRDUP(lex->strval->data);
+			if (val =3D=3D NULL)
+				return JSON_OUT_OF_MEMORY;
+		}
 	}
 	else
 	{
 		int			len =3D (lex->token_terminator - lex->token_start);
=20
+		//allocation here
 		val =3D palloc(len + 1);
 		memcpy(val, lex->token_start, len);
 		val[len] =3D '\0';
@@ -332,7 +340,11 @@ parse_object_field(JsonLexContext *lex, JsonSemAction =
*sem)
 	if (lex_peek(lex) !=3D JSON_TOKEN_STRING)
 		return report_parse_error(JSON_PARSE_STRING, lex);
 	if ((ostart !=3D NULL || oend !=3D NULL) && lex->strval !=3D NULL)
-		fname =3D pstrdup(lex->strval->data);
+	{
+		fname =3D STRDUP(lex->strval->data);
+		if (fname =3D=3D NULL)
+			return JSON_OUT_OF_MEMORY;
+	}
 	result =3D json_lex(lex);
 	if (result !=3D JSON_SUCCESS)
 		return result;
@@ -378,7 +390,9 @@ parse_object(JsonLexContext *lex, JsonSemAction *sem)
 	JsonTokenType tok;
 	JsonParseErrorType result;
=20
+#ifndef FRONTEND
 	check_stack_depth();
+#endif
=20
 	if (ostart !=3D NULL)
 		(*ostart) (sem->semstate);
@@ -478,7 +492,9 @@ parse_array(JsonLexContext *lex, JsonSemAction *sem)
 	json_struct_action aend =3D sem->array_end;
 	JsonParseErrorType result;
=20
+#ifndef FRONTEND
 	check_stack_depth();
+#endif
=20
 	if (astart !=3D NULL)
 		(*astart) (sem->semstate);
@@ -1047,76 +1063,138 @@ report_parse_error(JsonParseContext ctx, JsonLexCo=
ntext *lex)
 	 * unhandled enum values.  But this needs to be here anyway to cover the
 	 * possibility of an incorrect input.
 	 */
-	json_log_and_abort("unexpected json parse state: %d", (int) ctx);
 	return JSON_SUCCESS;		/* silence stupider compilers */
 }
=20
 /*
- * Construct a detail message for a JSON error.
+ * Construct a detail message for a JSON error, palloc'd or malloc'd.
+ * note that this may return NULL on OOM in the frontend.
  */
 char *
 json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
 {
+	char	   *errstr;
+	char	   *tok =3D NULL;
+
+	/* allocate a string large enough to include the error */
+	errstr =3D ALLOC(256 * sizeof(char));
+	if (errstr =3D=3D NULL)
+		return NULL;
+
+	/* Grab current token from the lexing context, if necessary */
+	switch (error)
+	{
+		case JSON_ESCAPING_INVALID:
+		case JSON_EXPECTED_ARRAY_FIRST:
+		case JSON_EXPECTED_ARRAY_NEXT:
+		case JSON_EXPECTED_COLON:
+		case JSON_EXPECTED_END:
+		case JSON_EXPECTED_JSON:
+		case JSON_EXPECTED_OBJECT_FIRST:
+		case JSON_EXPECTED_OBJECT_NEXT:
+		case JSON_EXPECTED_STRING:
+		case JSON_INVALID_TOKEN:
+			tok =3D extract_token(lex);
+			/* if allocating the token failed, just give up */
+			if (tok =3D=3D NULL)
+				return NULL;
+			break;
+		default:
+			break;
+	}
+
 	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));
+			sprintf(errstr,
+					_("Escape sequence \"\\%s\" is invalid."),
+					tok);
+			break;
 		case JSON_ESCAPING_REQUIRED:
-			return psprintf(_("Character with value 0x%02x must be escaped."),
-							(unsigned char) *(lex->token_terminator));
+			sprintf(errstr,
+					_("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));
+			sprintf(errstr,
+					_("Expected end of input, but found \"%s\"."), tok);
+			break;
 		case JSON_EXPECTED_ARRAY_FIRST:
-			return psprintf(_("Expected array element or \"]\", but found \"%s\"."),
-							extract_token(lex));
+			sprintf(errstr,
+					_("Expected array element or \"]\", but found \"%s\"."),
+					tok);
+			break;
 		case JSON_EXPECTED_ARRAY_NEXT:
-			return psprintf(_("Expected \",\" or \"]\", but found \"%s\"."),
-							extract_token(lex));
+			sprintf(errstr,
+					_("Expected \",\" or \"]\", but found \"%s\"."),
+					tok);
+			break;
 		case JSON_EXPECTED_COLON:
-			return psprintf(_("Expected \":\", but found \"%s\"."),
-							extract_token(lex));
+			sprintf(errstr,
+					_("Expected \":\", but found \"%s\"."),
+					tok);
+			break;
 		case JSON_EXPECTED_JSON:
-			return psprintf(_("Expected JSON value, but found \"%s\"."),
-							extract_token(lex));
+			sprintf(errstr,
+					_("Expected JSON value, but found \"%s\"."),
+					tok);
+			break;
 		case JSON_EXPECTED_MORE:
-			return _("The input string ended unexpectedly.");
+			sprintf(errstr,
+					_("The input string ended unexpectedly."));
+			break;
 		case JSON_EXPECTED_OBJECT_FIRST:
-			return psprintf(_("Expected string or \"}\", but found \"%s\"."),
-							extract_token(lex));
+			sprintf(errstr,
+					_("Expected string or \"}\", but found \"%s\"."),
+					tok);
+			break;
 		case JSON_EXPECTED_OBJECT_NEXT:
-			return psprintf(_("Expected \",\" or \"}\", but found \"%s\"."),
-							extract_token(lex));
+			sprintf(errstr,
+					_("Expected \",\" or \"}\", but found \"%s\"."),
+					tok);
+			break;
 		case JSON_EXPECTED_STRING:
-			return psprintf(_("Expected string, but found \"%s\"."),
-							extract_token(lex));
+			sprintf(errstr,
+					_("Expected string, but found \"%s\"."),
+					tok);
+			break;
 		case JSON_INVALID_TOKEN:
-			return psprintf(_("Token \"%s\" is invalid."),
-							extract_token(lex));
+			sprintf(errstr,
+					_("Token \"%s\" is invalid."),
+					tok);
+			break;
+		case JSON_OUT_OF_MEMORY:
+			sprintf(errstr, _("out of memory."));
+			break;
+
 		case JSON_UNICODE_CODE_POINT_ZERO:
-			return _("\\u0000 cannot be converted to text.");
+			sprintf(errstr, _("\\u0000 cannot be converted to text."));
+			break;
 		case JSON_UNICODE_ESCAPE_FORMAT:
-			return _("\"\\u\" must be followed by four hexadecimal digits.");
+			sprintf(errstr,
+					_("\"\\u\" must be followed by four hexadecimal digits."));
+			break;
 		case JSON_UNICODE_HIGH_ESCAPE:
 			/* note: this case is only reachable in frontend not backend */
-			return _("Unicode escape values cannot be used for code point values ab=
ove 007F when the encoding is not UTF8.");
+			sprintf(errstr,
+					_("Unicode escape values cannot be used for code point values above 0=
07F when the encoding is not UTF8."));
+			break;
 		case JSON_UNICODE_HIGH_SURROGATE:
-			return _("Unicode high surrogate must not follow a high surrogate.");
+			sprintf(errstr,
+					_("Unicode high surrogate must not follow a high surrogate."));
+			break;
 		case JSON_UNICODE_LOW_SURROGATE:
-			return _("Unicode low surrogate must follow a high surrogate.");
+			sprintf(errstr,
+					_("Unicode low surrogate must follow a high surrogate."));
+			break;
 	}
=20
-	/*
-	 * 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.
-	 */
-	json_log_and_abort("unexpected json parse error type: %d", (int) error);
-	return NULL;				/* silence stupider compilers */
+	if (tok)
+		FREE(tok);
+	return errstr;
 }
=20
 /*
@@ -1126,7 +1204,14 @@ static char *
 extract_token(JsonLexContext *lex)
 {
 	int			toklen =3D lex->token_terminator - lex->token_start;
-	char	   *token =3D palloc(toklen + 1);
+	char	   *token =3D ALLOC(toklen + 1);
+
+	/*
+	 * There is not much we can do here on OOM, so just return some dummy
+	 * value.
+	 */
+	if (token =3D=3D NULL)
+		return NULL;
=20
 	memcpy(token, lex->token_start, toklen);
 	token[toklen] =3D '\0';
diff --git a/src/bin/pg_verifybackup/parse_manifest.c b/src/bin/pg_verifyba=
ckup/parse_manifest.c
index 3b13ae5b84..e31a05dc35 100644
--- a/src/bin/pg_verifybackup/parse_manifest.c
+++ b/src/bin/pg_verifybackup/parse_manifest.c
@@ -147,7 +147,10 @@ json_parse_manifest(JsonManifestParseContext *context,=
 char *buffer,
 	/* Run the actual JSON parser. */
 	json_error =3D pg_parse_json(lex, &sem);
 	if (json_error !=3D JSON_SUCCESS)
-		json_manifest_parse_failure(context, json_errdetail(json_error, lex));
+	{
+		char	   *errstr =3D json_errdetail(json_error, lex);
+		json_manifest_parse_failure(context, errstr ? errstr : "out of memory");
+	}
 	if (parse.state !=3D JM_EXPECT_EOF)
 		json_manifest_parse_failure(context, "manifest ended unexpectedly");
=20

--9tPkd5fmao84BUcK--

--7OnI4oC4vBBEqBIk
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmDUHnQACgkQnvQgOdby
QH1Bdg//eRm4NWT3ah8TrEUPd8gVurC+sZTw3Dr0NNcuJY2KWylXiCIBcRsbJYnw
6qirCmz2Jm9kxH3D6NoH5/9W3MX1yr1n28lP/q1UpIR0QKyClgvYiDAe1w8lEOHp
toaxoqIJS4W+sMdR2f+XLJSo78nxkGcVyCar5NmZQyJo7Jusfsczl6UjyCBGwObs
Ur2shqjqUBg2i+onxKRFAs4XpxrBuwxudEwDMS3kLVssu7oE66IsjadYkPqqbcYr
3oEsdIdmQHFdFr01gEOqEFNG4g5zUKfy+FIfbeNeRLzSg6+uMD8qxILIWgKJdvEJ
a7fZUgqcQBhsnUaoilk4v2rtpjrPECV6yajc8n/CNDLgIwQ/1PE6mzmVFmlIsmax
AEfZ2xHuf1QljtOV/VNmA0eZY8JwgahmKHVq524V8rAjDWCT5euW3jLXlZILJ7ql
LOAXL5IfGRUmlz8KqVNpIycQsiaFwYMOK5B+yDdpoAnPCzSE2Acxe+Adar7lA+Nr
oQByBrWC6nAxxjW+CU5LTa2IovDSOVNqIyvJJX+PjS/MFYyyB1WvUJCLumBf4WFo
zQgqSoYr+BX4CHgIYJ0NkZYNfRThOlvIfkJ6wQ6D0vrFa84cVKzX53IPufIR6hls
ZNSB+aNEeQaMKC7Ot0IPnx4FTMO3yQO5AG+PIoaV08E23sCJlBU=
=Fi8P
-----END PGP SIGNATURE-----

--7OnI4oC4vBBEqBIk--