From 57f1630cf096d5e173e5de80f50ad1bf66e15587 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 24 Dec 2006 18:25:58 +0000 Subject: [PATCH] Bring some order and sanity to error handling in the xml patch. Use a TRY block instead of (inadequate) ad-hoc coding to ensure that libxml is cleaned up after a failure. Report the intended SQLCODE instead of defaulting to XX000. Avoid risking use of a dangling pointer by keeping the persistent error buffer in TopMemoryContext. Be less trusting that error messages don't contain %. This patch doesn't do anything about changing the way the messages are put together --- this is just about mechanism. --- src/backend/utils/adt/xml.c | 411 ++++++++++++++++++++---------------- 1 file changed, 227 insertions(+), 184 deletions(-) diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c index dc6a7f197b..7babc82551 100644 --- a/src/backend/utils/adt/xml.c +++ b/src/backend/utils/adt/xml.c @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/backend/utils/adt/xml.c,v 1.4 2006/12/24 00:57:48 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/utils/adt/xml.c,v 1.5 2006/12/24 18:25:58 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -19,7 +19,8 @@ * fail. For one thing, this avoids having to manage variant catalog * installations. But it also has nice effects such as that you can * dump a database containing XML type data even if the server is not - * linked with libxml. + * linked with libxml. Thus, make sure xml_out() works even if nothing + * else does. */ #include "postgres.h" @@ -36,6 +37,7 @@ #include "mb/pg_wchar.h" #include "nodes/execnodes.h" #include "utils/builtins.h" +#include "utils/memutils.h" #include "utils/xml.h" @@ -43,28 +45,27 @@ #define PG_XML_DEFAULT_URI "dummy.xml" +static StringInfo xml_err_buf = NULL; static void xml_init(void); static void *xml_palloc(size_t size); static void *xml_repalloc(void *ptr, size_t size); static void xml_pfree(void *ptr); static char *xml_pstrdup(const char *string); -static void xml_ereport(int level, char *msg, void *ctxt); +static void xml_ereport(int level, int sqlcode, + const char *msg, void *ctxt); static void xml_errorHandler(void *ctxt, const char *msg, ...); -static void xml_ereport_by_code(int level, char *msg, int errcode); +static void xml_ereport_by_code(int level, int sqlcode, + const char *msg, int errcode); static xmlChar *xml_text2xmlChar(text *in); static xmlDocPtr xml_parse(text *data, int opts, bool is_document); - -/* Global variables */ -/* taken from contrib/xml2 */ -/* FIXME: DO NOT USE global vars !!! */ -static char *xml_errmsg = NULL; /* overall error message */ - #endif /* USE_LIBXML */ - -#define NO_XML_SUPPORT() ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("no XML support in this installation"))) +#define NO_XML_SUPPORT() \ + ereport(ERROR, \ + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), \ + errmsg("no XML support in this installation"))) Datum @@ -292,54 +293,80 @@ xmlvalidate(PG_FUNCTION_ARGS) #ifdef USE_LIBXML text *data = PG_GETARG_TEXT_P(0); text *dtdOrUri = PG_GETARG_TEXT_P(1); - bool result = FALSE; - xmlParserCtxtPtr ctxt; /* the parser context */ - xmlDocPtr doc; /* the resulting document tree */ - xmlDtdPtr dtd; + bool result = false; + xmlParserCtxtPtr ctxt = NULL; + xmlDocPtr doc = NULL; + xmlDtdPtr dtd = NULL; xml_init(); - ctxt = xmlNewParserCtxt(); - if (ctxt == NULL) - xml_ereport(ERROR, "could not allocate parser context", ctxt); - doc = xmlCtxtReadMemory(ctxt, (char *) VARDATA(data), - VARSIZE(data) - VARHDRSZ, PG_XML_DEFAULT_URI, NULL, 0); - if (doc == NULL) - xml_ereport(ERROR, "could not parse XML data", ctxt); + /* We use a PG_TRY block to ensure libxml is cleaned up on error */ + PG_TRY(); + { + ctxt = xmlNewParserCtxt(); + if (ctxt == NULL) + xml_ereport(ERROR, ERRCODE_INTERNAL_ERROR, + "could not allocate parser context", ctxt); + + doc = xmlCtxtReadMemory(ctxt, (char *) VARDATA(data), + VARSIZE(data) - VARHDRSZ, + PG_XML_DEFAULT_URI, NULL, 0); + if (doc == NULL) + xml_ereport(ERROR, ERRCODE_INVALID_XML_DOCUMENT, + "could not parse XML data", ctxt); #if 0 - uri = xmlCreateURI(); - ereport(NOTICE, (errcode(0),errmsg(" dtd - %s", dtdOrUri))); - dtd = palloc(sizeof(xmlDtdPtr)); - uri = xmlParseURI(dtdOrUri); - if (uri == NULL) - xml_ereport(ERROR, "not implemented yet... (TODO)", ctxt); - else + uri = xmlCreateURI(); + elog(NOTICE, "dtd - %s", dtdOrUri); + dtd = palloc(sizeof(xmlDtdPtr)); + uri = xmlParseURI(dtdOrUri); + if (uri == NULL) + xml_ereport(ERROR, ERRCODE_INTERNAL_ERROR, + "not implemented yet... (TODO)", ctxt); + else #endif - dtd = xmlParseDTD(NULL, xml_text2xmlChar(dtdOrUri)); + dtd = xmlParseDTD(NULL, xml_text2xmlChar(dtdOrUri)); + + if (dtd == NULL) + xml_ereport(ERROR, ERRCODE_INVALID_XML_DOCUMENT, + "could not load DTD", ctxt); + + if (xmlValidateDtd(xmlNewValidCtxt(), doc, dtd) == 1) + result = true; + + if (!result) + xml_ereport(NOTICE, ERRCODE_INVALID_XML_DOCUMENT, + "validation against DTD failed", ctxt); - if (dtd == NULL) - { #if 0 - xmlFreeDoc(doc); - xmlFreeParserCtxt(ctxt); + if (uri) + xmlFreeURI(uri); #endif - xml_ereport(ERROR, "could not load DTD", ctxt); + if (dtd) + xmlFreeDtd(dtd); + if (doc) + xmlFreeDoc(doc); + if (ctxt) + xmlFreeParserCtxt(ctxt); + xmlCleanupParser(); } - - if (xmlValidateDtd(xmlNewValidCtxt(), doc, dtd) == 1) - result = TRUE; - + PG_CATCH(); + { #if 0 - xmlFreeURI(uri); - xmlFreeDtd(dtd); - xmlFreeDoc(doc); - xmlFreeParserCtxt(ctxt); - xmlCleanupParser(); + if (uri) + xmlFreeURI(uri); #endif + if (dtd) + xmlFreeDtd(dtd); + if (doc) + xmlFreeDoc(doc); + if (ctxt) + xmlFreeParserCtxt(ctxt); + xmlCleanupParser(); - if (!result) - xml_ereport(NOTICE, "validation against DTD failed", ctxt); + PG_RE_THROW(); + } + PG_END_TRY(); PG_RETURN_BOOL(result); #else /* not USE_LIBXML */ @@ -368,12 +395,27 @@ xml_init(void) errdetail("libxml2 has incompatible char type: sizeof(char)=%u, sizeof(xmlChar)=%u.", (int) sizeof(char), (int) sizeof(xmlChar)))); + if (xml_err_buf == NULL) + { + /* First time through: create error buffer in permanent context */ + MemoryContext oldcontext; + + oldcontext = MemoryContextSwitchTo(TopMemoryContext); + xml_err_buf = makeStringInfo(); + MemoryContextSwitchTo(oldcontext); + } + else + { + /* Reset pre-existing buffer to empty */ + xml_err_buf->data[0] = '\0'; + xml_err_buf->len = 0; + } + /* Now that xml_err_buf exists, safe to call xml_errorHandler */ + xmlSetGenericErrorFunc(NULL, xml_errorHandler); + xmlMemSetup(xml_pfree, xml_palloc, xml_repalloc, xml_pstrdup); xmlInitParser(); LIBXML_TEST_VERSION; - /* do not flood PG's logfile with libxml error messages - reset error handler*/ - xmlSetGenericErrorFunc(NULL, xml_errorHandler); - xml_errmsg = NULL; } @@ -391,106 +433,115 @@ xml_init(void) static xmlDocPtr xml_parse(text *data, int opts, bool is_document) { - bool validationFailed = FALSE; - xmlParserCtxtPtr ctxt; /* the parser context */ - xmlDocPtr doc; /* the resulting document tree */ + bool validationFailed = false; int res_code; int32 len; xmlChar *string; + xmlParserCtxtPtr ctxt = NULL; + xmlDocPtr doc = NULL; #ifdef XML_DEBUG_DTD_CONST - xmlDtdPtr dtd; /* pointer to DTD */ + xmlDtdPtr dtd = NULL; #endif - xml_init(); - len = VARSIZE(data) - VARHDRSZ; /* will be useful later */ string = xml_text2xmlChar(data); - ctxt = xmlNewParserCtxt(); - if (ctxt == NULL) - xml_ereport(ERROR, "could not allocate parser context", ctxt); + xml_init(); - /* first, we try to parse the string as XML doc, then, as XML chunk */ - ereport(DEBUG3, (errmsg("string to parse: %s", string))); - if (len >= 5 && strncmp((char *) string, "= 5 && strncmp((char *) string, "intSubset != NULL) || (doc->extSubset != NULL)) -#endif - { - /* assume that inline DTD exists - validation should be performed */ -#ifdef XML_DEBUG_DTD_CONST + dtd = xmlParseDTD(NULL, (xmlChar *) XML_DEBUG_DTD_CONST); + if (dtd == NULL) + xml_ereport(ERROR, ERRCODE_INVALID_XML_DOCUMENT, + "could not parse DTD data", ctxt); if (xmlValidateDtd(xmlNewValidCtxt(), doc, dtd) != 1) + validationFailed = true; #else - if (ctxt->valid == 0) -#endif + /* if dtd for our xml data is detected... */ + if ((doc->intSubset != NULL) || (doc->extSubset != NULL)) { - /* DTD exists, but validator reported 'validation failed' */ - validationFailed = TRUE; + /* assume inline DTD exists - validation should be performed */ + if (ctxt->valid == 0) + { + /* DTD exists, but validator reported 'validation failed' */ + validationFailed = true; + } } +#endif + + if (validationFailed) + xml_ereport(WARNING, ERRCODE_INVALID_XML_DOCUMENT, + "validation against DTD failed", ctxt); + + /* TODO encoding issues + * (thoughts: + * CASE: + * - XML data has explicit encoding attribute in its prolog + * - if not, assume that enc. of XML data is the same as client's one + * + * The common rule is to accept the XML data only if its encoding + * is the same as encoding of the storage (server's). The other possible + * option is to accept all the docs, but DO TRANSFORMATION and, if needed, + * change the prolog. + * + * I think I'd stick the first way (for the 1st version), + * it's much simplier (less errors...) + * ) */ + /* ... */ + +#ifdef XML_DEBUG_DTD_CONST + if (dtd) + xmlFreeDtd(dtd); +#endif + if (doc) + xmlFreeDoc(doc); + if (ctxt) + xmlFreeParserCtxt(ctxt); + xmlCleanupParser(); } + PG_CATCH(); + { +#ifdef XML_DEBUG_DTD_CONST + if (dtd) + xmlFreeDtd(dtd); +#endif + if (doc) + xmlFreeDoc(doc); + if (ctxt) + xmlFreeParserCtxt(ctxt); + xmlCleanupParser(); - if (validationFailed) - xml_ereport(WARNING, "validation against DTD failed", ctxt); - - /* TODO encoding issues - * (thoughts: - * CASE: - * - XML data has explicit encoding attribute in its prolog - * - if not, assume that enc. of XML data is the same as client's one - * - * The common rule is to accept the XML data only if its encoding - * is the same as encoding of the storage (server's). The other possible - * option is to accept all the docs, but DO TRANSFORMATION and, if needed, - * change the prolog. - * - * I think I'd stick the first way (for the 1st version), - * it's much simplier (less errors...) - * ) */ - /* ... */ - - xmlFreeParserCtxt(ctxt); - xmlCleanupParser(); - - ereport(DEBUG3, (errmsg("XML data successfully parsed, encoding: %s", - (char *) doc->encoding))); + PG_RE_THROW(); + } + PG_END_TRY(); return doc; } @@ -549,17 +600,17 @@ xml_pstrdup(const char *string) * Adds detail - libxml's native error message, if any. */ static void -xml_ereport(int level, char *msg, void *ctxt) +xml_ereport(int level, int sqlcode, + const char *msg, void *ctxt) { - char *xmlErrDetail; - int xmlErrLen, i; xmlErrorPtr libxmlErr = NULL; - if (xml_errmsg != NULL) + if (xml_err_buf->len > 0) { - ereport(DEBUG1, (errmsg("%s", xml_errmsg))); - pfree(xml_errmsg); - xml_errmsg = NULL; + ereport(DEBUG1, + (errmsg("%s", xml_err_buf->data))); + xml_err_buf->data[0] = '\0'; + xml_err_buf->len = 0; } if (ctxt != NULL) @@ -567,31 +618,27 @@ xml_ereport(int level, char *msg, void *ctxt) if (libxmlErr == NULL) { - if (level == ERROR) - { - xmlFreeParserCtxt(ctxt); - xmlCleanupParser(); - } - ereport(level, (errmsg(msg))); + ereport(level, + (errcode(sqlcode), + errmsg("%s", msg))); } else { /* as usual, libxml error message contains '\n'; get rid of it */ - xmlErrLen = strlen(libxmlErr->message); /* - 1; */ - xmlErrDetail = (char *) palloc(xmlErrLen); + char *xmlErrDetail; + int xmlErrLen, i; + + xmlErrDetail = pstrdup(libxmlErr->message); + xmlErrLen = strlen(xmlErrDetail); for (i = 0; i < xmlErrLen; i++) { - if (libxmlErr->message[i] == '\n') + if (xmlErrDetail[i] == '\n') xmlErrDetail[i] = '.'; - else - xmlErrDetail[i] = libxmlErr->message[i]; } - if (level == ERROR) - { - xmlFreeParserCtxt(ctxt); - xmlCleanupParser(); - } - ereport(level, (errmsg(msg), errdetail("%s", xmlErrDetail))); + ereport(level, + (errcode(sqlcode), + errmsg("%s", msg), + errdetail("%s", xmlErrDetail))); } } @@ -602,25 +649,22 @@ xml_ereport(int level, char *msg, void *ctxt) static void xml_errorHandler(void *ctxt, const char *msg,...) { - char xml_errbuf[256]; - va_list args; - - /* Format this message ... */ - va_start(args, msg); - vsnprintf(xml_errbuf, sizeof(xml_errbuf)-1, msg, args); - va_end(args); - xml_errbuf[sizeof(xml_errbuf)-1] = '\0'; - - /* ... and append to xml_errbuf */ - if (xml_errmsg == NULL) - xml_errmsg = pstrdup(xml_errbuf); - else + /* Append the formatted text to xml_err_buf */ + for (;;) { - int32 xsize = strlen(xml_errmsg); + va_list args; + bool success; + + /* Try to format the data. */ + va_start(args, msg); + success = appendStringInfoVA(xml_err_buf, msg, args); + va_end(args); - xml_errmsg = repalloc(xml_errmsg, - (size_t) (xsize + strlen(xml_errbuf) + 1)); - strcpy(&xml_errmsg[xsize - 1], xml_errbuf); + if (success) + break; + + /* Double the buffer size and try again. */ + enlargeStringInfo(xml_err_buf, xml_err_buf->maxlen); } } @@ -630,17 +674,21 @@ xml_errorHandler(void *ctxt, const char *msg,...) * TODO make them closer to recommendations from Postgres manual */ static void -xml_ereport_by_code(int level, char *msg, int code) +xml_ereport_by_code(int level, int sqlcode, + const char *msg, int code) { const char *det; - if (code < 0) + if (xml_err_buf->len > 0) { - ereport(level, (errmsg(msg))); - return; + ereport(DEBUG1, + (errmsg("%s", xml_err_buf->data))); + xml_err_buf->data[0] = '\0'; + xml_err_buf->len = 0; } - switch (code) { + switch (code) + { case XML_ERR_INTERNAL_ERROR: det = "libxml internal error"; break; @@ -799,19 +847,14 @@ xml_ereport_by_code(int level, char *msg, int code) det = "Closing tag not found"; break; default: - det = "Unregistered error (libxml error code: %d)"; - ereport(DEBUG1, - (errmsg_internal("Check out \"libxml/xmlerror.h\" and bring errcode \"%d\" processing to \"xml.c\".", code))); - } - - if (xml_errmsg != NULL) - { - ereport(DEBUG1, (errmsg("%s", xml_errmsg))); - pfree(xml_errmsg); - xml_errmsg = NULL; + det = "Unrecognized libxml error code: %d"; + break; } - ereport(level, (errmsg(msg), errdetail(det, code))); + ereport(level, + (errcode(sqlcode), + errmsg("%s", msg), + errdetail(det, code))); } -- 2.40.0