Rewrite libxml error handling to be more robust.

libxml reports some errors (like invalid xmlns attributes) via the error
handler hook, but still returns a success indicator to the library caller.
This causes us to miss some errors that are important to report.  Since the
"generic" error handler hook doesn't know whether the message it's getting
is for an error, warning, or notice, stop using that and instead start
using the "structured" error handler hook, which gets enough information
to be useful.

While at it, arrange to save and restore the error handler hook setting in
each libxml-using function, rather than assuming we can set and forget the
hook.  This should improve the odds of working nicely with third-party
libraries that also use libxml.

In passing, volatile-ize some local variables that get modified within
PG_TRY blocks.  I noticed this while testing with an older gcc version
than I'd previously tried to compile xml.c with.

Florian Pflug and Tom Lane, with extensive review/testing by Noah Misch

2 files changed, 127 insertions, 60 deletions

diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c
index 44c600e1348..2ddee59fcb7 100644
--- a/contrib/xml2/xpath.c
+++ b/contrib/xml2/xpath.c
@@ -38,7 +38,7 @@ Datum		xpath_table(PG_FUNCTION_ARGS);
 
 /* exported for use by xslt_proc.c */
 
-void		pgxml_parser_init(void);
+PgXmlErrorContext *pgxml_parser_init(PgXmlStrictness strictness);
 
 /* workspace for pgxml_xpath() */
 
@@ -68,18 +68,27 @@ static void cleanup_workspace(xpath_workspace *workspace);
 
 /*
  * Initialize for xml parsing.
+ *
+ * As with the underlying pg_xml_init function, calls to this MUST be followed
+ * by a PG_TRY block that guarantees that pg_xml_done is called.
  */
-void
-pgxml_parser_init(void)
+PgXmlErrorContext *
+pgxml_parser_init(PgXmlStrictness strictness)
 {
+	PgXmlErrorContext *xmlerrcxt;
+
 	/* Set up error handling (we share the core's error handler) */
-	pg_xml_init();
+	xmlerrcxt = pg_xml_init(strictness);
+
+	/* Note: we're assuming an elog cannot be thrown by the following calls */
 
 	/* Initialize libxml */
 	xmlInitParser();
 
 	xmlSubstituteEntitiesDefault(1);
 	xmlLoadExtDtdDefaultValue = 1;
+
+	return xmlerrcxt;
 }
 
 
@@ -98,16 +107,33 @@ Datum
 xml_is_well_formed(PG_FUNCTION_ARGS)
 {
 	text	   *t = PG_GETARG_TEXT_P(0);		/* document buffer */
+	bool		result = false;
 	int32		docsize = VARSIZE(t) - VARHDRSZ;
 	xmlDocPtr	doctree;
+	PgXmlErrorContext *xmlerrcxt;
+
+	xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
+
+	PG_TRY();
+	{
+		doctree = xmlParseMemory((char *) VARDATA(t), docsize);
+
+		result = (doctree != NULL);
+
+		if (doctree != NULL)
+			xmlFreeDoc(doctree);
+	}
+	PG_CATCH();
+	{
+		pg_xml_done(xmlerrcxt, true);
 
-	pgxml_parser_init();
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
 
-	doctree = xmlParseMemory((char *) VARDATA(t), docsize);
-	if (doctree == NULL)
-		PG_RETURN_BOOL(false);	/* i.e. not well-formed */
-	xmlFreeDoc(doctree);
-	PG_RETURN_BOOL(true);
+	pg_xml_done(xmlerrcxt, false);
+
+	PG_RETURN_BOOL(result);
 }
 
 
@@ -399,41 +425,52 @@ static xmlXPathObjectPtr
 pgxml_xpath(text *document, xmlChar *xpath, xpath_workspace *workspace)
 {
 	int32		docsize = VARSIZE(document) - VARHDRSZ;
-	xmlXPathObjectPtr res;
+	PgXmlErrorContext *xmlerrcxt;
 	xmlXPathCompExprPtr comppath;
 
 	workspace->doctree = NULL;
 	workspace->ctxt = NULL;
 	workspace->res = NULL;
 
-	pgxml_parser_init();
+	xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
 
-	workspace->doctree = xmlParseMemory((char *) VARDATA(document), docsize);
-	if (workspace->doctree == NULL)
-		return NULL;			/* not well-formed */
+	PG_TRY();
+	{
+		workspace->doctree = xmlParseMemory((char *) VARDATA(document),
+											docsize);
+		if (workspace->doctree != NULL)
+		{
+			workspace->ctxt = xmlXPathNewContext(workspace->doctree);
+			workspace->ctxt->node = xmlDocGetRootElement(workspace->doctree);
 
-	workspace->ctxt = xmlXPathNewContext(workspace->doctree);
-	workspace->ctxt->node = xmlDocGetRootElement(workspace->doctree);
+			/* compile the path */
+			comppath = xmlXPathCompile(xpath);
+			if (comppath == NULL)
+				xml_ereport(xmlerrcxt, ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
+							"XPath Syntax Error");
 
-	/* compile the path */
-	comppath = xmlXPathCompile(xpath);
-	if (comppath == NULL)
+			/* Now evaluate the path expression. */
+			workspace->res = xmlXPathCompiledEval(comppath, workspace->ctxt);
+
+			xmlXPathFreeCompExpr(comppath);
+		}
+	}
+	PG_CATCH();
 	{
 		cleanup_workspace(workspace);
-		xml_ereport(ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
-					"XPath Syntax Error");
-	}
 
-	/* Now evaluate the path expression. */
-	res = xmlXPathCompiledEval(comppath, workspace->ctxt);
-	workspace->res = res;
+		pg_xml_done(xmlerrcxt, true);
 
-	xmlXPathFreeCompExpr(comppath);
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
 
-	if (res == NULL)
+	if (workspace->res == NULL)
 		cleanup_workspace(workspace);
 
-	return res;
+	pg_xml_done(xmlerrcxt, false);
+
+	return workspace->res;
 }
 
 /* Clean up after processing the result of pgxml_xpath() */
@@ -534,6 +571,8 @@ xpath_table(PG_FUNCTION_ARGS)
 								 * document */
 	bool		had_values;		/* To determine end of nodeset results */
 	StringInfoData query_buf;
+	PgXmlErrorContext *xmlerrcxt;
+	volatile xmlDocPtr doctree = NULL;
 
 	/* We only have a valid tuple description in table function mode */
 	if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
@@ -659,14 +698,15 @@ xpath_table(PG_FUNCTION_ARGS)
 	 * Setup the parser.  This should happen after we are done evaluating the
 	 * query, in case it calls functions that set up libxml differently.
 	 */
-	pgxml_parser_init();
+	xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
 
+	PG_TRY();
+	{
 	/* For each row i.e. document returned from SPI */
 	for (i = 0; i < proc; i++)
 	{
 		char	   *pkey;
 		char	   *xmldoc;
-		xmlDocPtr	doctree;
 		xmlXPathContextPtr ctxt;
 		xmlXPathObjectPtr res;
 		xmlChar    *resstr;
@@ -718,11 +758,9 @@ xpath_table(PG_FUNCTION_ARGS)
 					/* compile the path */
 					comppath = xmlXPathCompile(xpaths[j]);
 					if (comppath == NULL)
-					{
-						xmlFreeDoc(doctree);
-						xml_ereport(ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
+						xml_ereport(xmlerrcxt, ERROR,
+									ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
 									"XPath Syntax Error");
-					}
 
 					/* Now evaluate the path expression. */
 					res = xmlXPathCompiledEval(comppath, ctxt);
@@ -737,8 +775,7 @@ xpath_table(PG_FUNCTION_ARGS)
 								if (res->nodesetval != NULL &&
 									rownr < res->nodesetval->nodeNr)
 								{
-									resstr =
-										xmlXPathCastNodeToString(res->nodesetval->nodeTab[rownr]);
+									resstr = xmlXPathCastNodeToString(res->nodesetval->nodeTab[rownr]);
 									had_values = true;
 								}
 								else
@@ -776,13 +813,31 @@ xpath_table(PG_FUNCTION_ARGS)
 			} while (had_values);
 		}
 
-		xmlFreeDoc(doctree);
+		if (doctree != NULL)
+			xmlFreeDoc(doctree);
+		doctree = NULL;
 
 		if (pkey)
 			pfree(pkey);
 		if (xmldoc)
 			pfree(xmldoc);
 	}
+	}
+	PG_CATCH();
+	{
+		if (doctree != NULL)
+			xmlFreeDoc(doctree);
+
+		pg_xml_done(xmlerrcxt, true);
+
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
+
+	if (doctree != NULL)
+		xmlFreeDoc(doctree);
+
+	pg_xml_done(xmlerrcxt, false);
 
 	tuplestore_donestoring(tupstore);
 
diff --git a/contrib/xml2/xslt_proc.c b/contrib/xml2/xslt_proc.c
index f8f7d7263f9..a8e481a3ce8 100644
--- a/contrib/xml2/xslt_proc.c
+++ b/contrib/xml2/xslt_proc.c
@@ -38,7 +38,7 @@ Datum		xslt_process(PG_FUNCTION_ARGS);
 #ifdef USE_LIBXSLT
 
 /* declarations to come from xpath.c */
-extern void pgxml_parser_init(void);
+extern PgXmlErrorContext *pgxml_parser_init(PgXmlStrictness strictness);
 
 /* local defs */
 static const char **parse_params(text *paramstr);
@@ -56,13 +56,14 @@ xslt_process(PG_FUNCTION_ARGS)
 	text	   *ssheet = PG_GETARG_TEXT_P(1);
 	text	   *paramstr;
 	const char **params;
-	xsltStylesheetPtr stylesheet = NULL;
-	xmlDocPtr	doctree;
-	xmlDocPtr	restree;
-	xmlDocPtr	ssdoc = NULL;
-	xmlChar    *resstr;
-	int			resstat;
-	int			reslen;
+	PgXmlErrorContext *xmlerrcxt;
+	volatile xsltStylesheetPtr stylesheet = NULL;
+	volatile xmlDocPtr doctree = NULL;
+	volatile xmlDocPtr restree = NULL;
+	volatile xmlDocPtr ssdoc = NULL;
+	volatile int resstat = -1;
+	xmlChar    *resstr = NULL;
+	int			reslen = 0;
 
 	if (fcinfo->nargs == 3)
 	{
@@ -77,9 +78,11 @@ xslt_process(PG_FUNCTION_ARGS)
 	}
 
 	/* Setup parser */
-	pgxml_parser_init();
+	xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
 
-	/* Check to see if document is a file or a literal */
+	PG_TRY();
+	{
+		/* Check to see if document is a file or a literal */
 
 	if (VARDATA(doct)[0] == '<')
 		doctree = xmlParseMemory((char *) VARDATA(doct), VARSIZE(doct) - VARHDRSZ);
@@ -87,7 +90,7 @@ xslt_process(PG_FUNCTION_ARGS)
 		doctree = xmlParseFile(text_to_cstring(doct));
 
 	if (doctree == NULL)
-		xml_ereport(ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
+		xml_ereport(xmlerrcxt, ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
 					"error parsing XML document");
 
 	/* Same for stylesheet */
@@ -96,35 +99,44 @@ xslt_process(PG_FUNCTION_ARGS)
 		ssdoc = xmlParseMemory((char *) VARDATA(ssheet),
 							   VARSIZE(ssheet) - VARHDRSZ);
 		if (ssdoc == NULL)
-		{
-			xmlFreeDoc(doctree);
-			xml_ereport(ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
+			xml_ereport(xmlerrcxt, ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
 						"error parsing stylesheet as XML document");
-		}
 
 		stylesheet = xsltParseStylesheetDoc(ssdoc);
 	}
 	else
 		stylesheet = xsltParseStylesheetFile((xmlChar *) text_to_cstring(ssheet));
 
-
 	if (stylesheet == NULL)
-	{
-		xmlFreeDoc(doctree);
-		xsltCleanupGlobals();
-		xml_ereport(ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
+		xml_ereport(xmlerrcxt, ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
 					"failed to parse stylesheet");
-	}
 
 	restree = xsltApplyStylesheet(stylesheet, doctree, params);
 	resstat = xsltSaveResultToString(&resstr, &reslen, restree, stylesheet);
+	}
+	PG_CATCH();
+	{
+		if (stylesheet != NULL)
+			xsltFreeStylesheet(stylesheet);
+		if (restree != NULL)
+			xmlFreeDoc(restree);
+		if (doctree != NULL)
+			xmlFreeDoc(doctree);
+		xsltCleanupGlobals();
+
+		pg_xml_done(xmlerrcxt, true);
+
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
 
 	xsltFreeStylesheet(stylesheet);
 	xmlFreeDoc(restree);
 	xmlFreeDoc(doctree);
-
 	xsltCleanupGlobals();
 
+	pg_xml_done(xmlerrcxt, false);
+
 	if (resstat < 0)
 		PG_RETURN_NULL();