Re: msxml3: Fix varargs handling in libxml2 error callback implementation
On Thu, Feb 16, 2012 at 01:55:44AM +0300, Nikolay Sivov wrote:
The problem is that vsnprintf() was called multiple times with same va_list. Ti fix that it was necessary to get rid of some tracing bits like macro-defined callback calls and a single function for all kinds of error types.
As far as I understand this problem it leads to a stack corruption when va_list is used multiple time without va_start/va_end around it, so it's critical to fix.
If I remember correctly, you can even process a va_list only once on some platforms. If you need to process it multiple times, you need to create a copy with va_copy() first. Ciao, Marcus
From d75433c8ff34e125c7db1fb665bdb9271991b9f5 Mon Sep 17 00:00:00 2001 From: Nikolay Sivov <nsivov(a)codeweavers.com> Date: Thu, 16 Feb 2012 01:06:16 +0300 Subject: [PATCH 1/1] Fix varargs handling in libxml2 error callback implementation
--- dlls/msxml3/domdoc.c | 50 +++++------------------- dlls/msxml3/main.c | 49 +++++++++++++++++------- dlls/msxml3/msxml_private.h | 18 ++------- dlls/msxml3/schema.c | 90 ++++++++++++++----------------------------- dlls/msxml3/selection.c | 4 +- 5 files changed, 80 insertions(+), 131 deletions(-)
diff --git a/dlls/msxml3/domdoc.c b/dlls/msxml3/domdoc.c index 64122ce..9bcecdd 100644 --- a/dlls/msxml3/domdoc.c +++ b/dlls/msxml3/domdoc.c @@ -425,23 +425,7 @@ static void sax_characters(void *ctx, const xmlChar *ch, int len) xmlSAX2Characters(ctxt, ch, len); }
-static void LIBXML2_LOG_CALLBACK sax_error(void* ctx, char const* msg, ...) -{ - va_list ap; - va_start(ap, msg); - LIBXML2_CALLBACK_ERR(doparse, msg, ap); - va_end(ap); -} - -static void LIBXML2_LOG_CALLBACK sax_warning(void* ctx, char const* msg, ...) -{ - va_list ap; - va_start(ap, msg); - LIBXML2_CALLBACK_WARN(doparse, msg, ap); - va_end(ap); -} - -static void sax_serror(void* ctx, xmlErrorPtr err) +static void libxml2_sax_structerror(void* ctx, xmlErrorPtr err) { LIBXML2_CALLBACK_SERROR(doparse, err); } @@ -472,9 +456,9 @@ static xmlDocPtr doparse(domdoc* This, char const* ptr, int len, xmlCharEncoding sax_characters, /* ignorableWhitespace */ xmlSAX2ProcessingInstruction, /* processingInstruction */ xmlSAX2Comment, /* comment */ - sax_warning, /* warning */ - sax_error, /* error */ - sax_error, /* fatalError */ + libxml2_warn_callback, /* warning */ + libxml2_error_callback, /* error */ + libxml2_error_callback, /* fatalError */ xmlSAX2GetParameterEntity, /* getParameterEntity */ xmlSAX2CDataBlock, /* cdataBlock */ xmlSAX2ExternalSubset, /* externalSubset */ @@ -482,10 +466,12 @@ static xmlDocPtr doparse(domdoc* This, char const* ptr, int len, xmlCharEncoding NULL, /* _private */ xmlSAX2StartElementNs, /* startElementNs */ xmlSAX2EndElementNs, /* endElementNs */ - sax_serror /* serror */ + libxml2_sax_structerror /* serror */ }; xmlInitParser();
+ TRACE("(%p)->(%p %d %d)\n", This, ptr, len, encoding); + pctx = xmlCreateMemoryParserCtxt(ptr, len); if (!pctx) { @@ -2634,22 +2620,6 @@ static inline BOOL is_wellformed(xmlDocPtr doc) #endif }
-static void LIBXML2_LOG_CALLBACK validate_error(void* ctx, char const* msg, ...) -{ - va_list ap; - va_start(ap, msg); - LIBXML2_CALLBACK_ERR(domdoc_validateNode, msg, ap); - va_end(ap); -} - -static void LIBXML2_LOG_CALLBACK validate_warning(void* ctx, char const* msg, ...) -{ - va_list ap; - va_start(ap, msg); - LIBXML2_CALLBACK_WARN(domdoc_validateNode, msg, ap); - va_end(ap); -} - static HRESULT WINAPI domdoc_validateNode( IXMLDOMDocument3* iface, IXMLDOMNode* node, @@ -2695,11 +2665,11 @@ static HRESULT WINAPI domdoc_validateNode( if (get_doc(This)->intSubset || get_doc(This)->extSubset) { xmlValidCtxtPtr vctx = xmlNewValidCtxt(); - vctx->error = validate_error; - vctx->warning = validate_warning; + vctx->error = libxml2_error_callback; + vctx->warning = libxml2_warn_callback; ++validated;
- if (!((node == (IXMLDOMNode*)iface)? + if (!((node == (IXMLDOMNode*)iface) ? xmlValidateDocument(vctx, get_doc(This)) : xmlValidateElement(vctx, get_doc(This), get_node_obj(node)->node))) { diff --git a/dlls/msxml3/main.c b/dlls/msxml3/main.c index 464ef90..4f9b020 100644 --- a/dlls/msxml3/main.c +++ b/dlls/msxml3/main.c @@ -60,28 +60,47 @@ HINSTANCE MSXML_hInstance = NULL;
#ifdef HAVE_LIBXML2
-void wineXmlCallbackLog(char const* caller, xmlErrorLevel lvl, char const* msg, va_list ap) +#define LIBXML2_LOG_CALLBACK __WINE_PRINTF_ATTR(2,3) + +void LIBXML2_LOG_CALLBACK libxml2_error_callback(void *ctxt, const char *msg, ...) { - char* buf = NULL; int len = 32, needed; - enum __wine_debug_class dbcl = __WINE_DBCL_ERR; - switch (lvl) + char* buf = NULL; + va_list ap; + + do { - case XML_ERR_NONE: - dbcl = __WINE_DBCL_TRACE; - break; - case XML_ERR_WARNING: - dbcl = __WINE_DBCL_WARN; - break; - default: - break; + heap_free(buf); + buf = heap_alloc(len); + va_start(ap, msg); + needed = vsnprintf(buf, len, msg, ap); + va_end(ap); + if (needed == -1) + len *= 2; + else if (needed >= len) + len = needed + 1; + else + needed = 0; } + while (needed); + + wine_dbg_log(__WINE_DBCL_ERR, &__wine_dbch_msxml, "libxml2", "%s", buf); + heap_free(buf); +} + +void LIBXML2_LOG_CALLBACK libxml2_warn_callback(void *ctxt, const char *msg, ...) +{ + int len = 32, needed; + char* buf = NULL; + va_list ap;
do { heap_free(buf); buf = heap_alloc(len); + va_start(ap, msg); needed = vsnprintf(buf, len, msg, ap); + va_end(ap); if (needed == -1) len *= 2; else if (needed >= len) @@ -91,11 +110,13 @@ void wineXmlCallbackLog(char const* caller, xmlErrorLevel lvl, char const* msg, } while (needed);
- wine_dbg_log(dbcl, &__wine_dbch_msxml, caller, "%s", buf); + wine_dbg_log(__WINE_DBCL_WARN, &__wine_dbch_msxml, "libxml2", "%s", buf); heap_free(buf); }
-void wineXmlCallbackError(char const* caller, xmlErrorPtr err) +#undef LIBXML2_LOG_CALLBACK + +void libxml2_structured_error(const char* caller, xmlErrorPtr err) { enum __wine_debug_class dbcl;
diff --git a/dlls/msxml3/msxml_private.h b/dlls/msxml3/msxml_private.h index e51501b..66c3b89 100644 --- a/dlls/msxml3/msxml_private.h +++ b/dlls/msxml3/msxml_private.h @@ -289,21 +289,11 @@ extern xmlNodePtr xmldoc_unlink_xmldecl(xmlDocPtr doc) DECLSPEC_HIDDEN;
extern HRESULT XMLElement_create( IUnknown *pUnkOuter, xmlNodePtr node, LPVOID *ppObj, BOOL own ) DECLSPEC_HIDDEN;
-extern void wineXmlCallbackLog(char const* caller, xmlErrorLevel lvl, char const* msg, va_list ap) DECLSPEC_HIDDEN; -extern void wineXmlCallbackError(char const* caller, xmlErrorPtr err) DECLSPEC_HIDDEN; +extern void libxml2_structured_error(const char* caller, xmlErrorPtr err) DECLSPEC_HIDDEN; +extern void libxml2_error_callback(void *ctxt, const char *msg, ...) DECLSPEC_HIDDEN; +extern void libxml2_warn_callback(void *ctxt, const char *msg, ...) DECLSPEC_HIDDEN;
-#define LIBXML2_LOG_CALLBACK __WINE_PRINTF_ATTR(2,3) - -#define LIBXML2_CALLBACK_TRACE(caller, msg, ap) \ - wineXmlCallbackLog(#caller, XML_ERR_NONE, msg, ap) - -#define LIBXML2_CALLBACK_WARN(caller, msg, ap) \ - wineXmlCallbackLog(#caller, XML_ERR_WARNING, msg, ap) - -#define LIBXML2_CALLBACK_ERR(caller, msg, ap) \ - wineXmlCallbackLog(#caller, XML_ERR_ERROR, msg, ap) - -#define LIBXML2_CALLBACK_SERROR(caller, err) wineXmlCallbackError(#caller, err) +#define LIBXML2_CALLBACK_SERROR(caller, err) libxml2_structured_error(#caller, err)
extern BOOL is_preserving_whitespace(xmlNodePtr node) DECLSPEC_HIDDEN; extern BOOL is_xpathmode(const xmlDocPtr doc) DECLSPEC_HIDDEN; diff --git a/dlls/msxml3/schema.c b/dlls/msxml3/schema.c index 253f901..c06bb69 100644 --- a/dlls/msxml3/schema.c +++ b/dlls/msxml3/schema.c @@ -230,85 +230,53 @@ static const BYTE hash_assoc_values[] = 116, 116, 116, 116, 116, 116 };
-static void LIBXML2_LOG_CALLBACK parser_error(void* ctx, char const* msg, ...) -{ - va_list ap; - va_start(ap, msg); - LIBXML2_CALLBACK_ERR(Schema_parse, msg, ap); - va_end(ap); -} - -static void LIBXML2_LOG_CALLBACK parser_warning(void* ctx, char const* msg, ...) -{ - va_list ap; - va_start(ap, msg); - LIBXML2_CALLBACK_WARN(Schema_parse, msg, ap); - va_end(ap); -} - #ifdef HAVE_XMLSCHEMASSETPARSERSTRUCTUREDERRORS -static void parser_serror(void* ctx, xmlErrorPtr err) +static void libxml2_schema_structerror(void* ctx, xmlErrorPtr err) { - LIBXML2_CALLBACK_SERROR(Schema_parse, err); + LIBXML2_CALLBACK_SERROR(schema_parse, err); } #endif
-static inline xmlSchemaPtr Schema_parse(xmlSchemaParserCtxtPtr spctx) +static inline xmlSchemaPtr schema_parse(xmlSchemaParserCtxtPtr ctxt) { - TRACE("(%p)\n", spctx); + TRACE("(%p)\n", ctxt);
- xmlSchemaSetParserErrors(spctx, parser_error, parser_warning, NULL); + xmlSchemaSetParserErrors(ctxt, libxml2_error_callback, libxml2_warn_callback, NULL); #ifdef HAVE_XMLSCHEMASSETPARSERSTRUCTUREDERRORS - xmlSchemaSetParserStructuredErrors(spctx, parser_serror, NULL); + xmlSchemaSetParserStructuredErrors(ctxt, libxml2_schema_structerror, NULL); #endif
- return xmlSchemaParse(spctx); -} - -static void LIBXML2_LOG_CALLBACK validate_error(void* ctx, char const* msg, ...) -{ - va_list ap; - va_start(ap, msg); - LIBXML2_CALLBACK_ERR(Schema_validate_tree, msg, ap); - va_end(ap); -} - -static void LIBXML2_LOG_CALLBACK validate_warning(void* ctx, char const* msg, ...) -{ - va_list ap; - va_start(ap, msg); - LIBXML2_CALLBACK_WARN(Schema_validate_tree, msg, ap); - va_end(ap); + return xmlSchemaParse(ctxt); }
#ifdef HAVE_XMLSCHEMASSETVALIDSTRUCTUREDERRORS -static void validate_serror(void* ctx, xmlErrorPtr err) +static void libxml2_validate_structerror(void* ctx, xmlErrorPtr err) { - LIBXML2_CALLBACK_SERROR(Schema_validate_tree, err); + LIBXML2_CALLBACK_SERROR(schema_validate_tree, err); } #endif
-static inline HRESULT Schema_validate_tree(xmlSchemaPtr schema, xmlNodePtr tree) +static inline HRESULT schema_validate_tree(xmlSchemaPtr schema, xmlNodePtr tree) { - xmlSchemaValidCtxtPtr svctx; + xmlSchemaValidCtxtPtr ctxt; int err;
TRACE("(%p, %p)\n", schema, tree); /* TODO: if validateOnLoad property is false, * we probably need to validate the schema here. */ - svctx = xmlSchemaNewValidCtxt(schema); - xmlSchemaSetValidErrors(svctx, validate_error, validate_warning, NULL); + ctxt = xmlSchemaNewValidCtxt(schema); + xmlSchemaSetValidErrors(ctxt, libxml2_error_callback, libxml2_warn_callback, NULL); #ifdef HAVE_XMLSCHEMASSETVALIDSTRUCTUREDERRORS - xmlSchemaSetValidStructuredErrors(svctx, validate_serror, NULL); + xmlSchemaSetValidStructuredErrors(ctxt, libxml2_validate_structerror, NULL); #endif
if (tree->type == XML_DOCUMENT_NODE) - err = xmlSchemaValidateDoc(svctx, (xmlDocPtr)tree); + err = xmlSchemaValidateDoc(ctxt, (xmlDocPtr)tree); else - err = xmlSchemaValidateOneElement(svctx, tree); + err = xmlSchemaValidateOneElement(ctxt, tree);
- xmlSchemaFreeValidCtxt(svctx); - return err? S_FALSE : S_OK; + xmlSchemaFreeValidCtxt(ctxt); + return err ? S_FALSE : S_OK; }
static DWORD dt_hash(xmlChar const* str, int len /* calculated if -1 */) @@ -599,11 +567,11 @@ HRESULT dt_validate(XDR_DT dt, xmlChar const* content)
if (!datatypes_schema) { - xmlSchemaParserCtxtPtr spctx; + xmlSchemaParserCtxtPtr ctxt; assert(datatypes_src != NULL); - spctx = xmlSchemaNewMemParserCtxt((char const*)datatypes_src, datatypes_len); - datatypes_schema = Schema_parse(spctx); - xmlSchemaFreeParserCtxt(spctx); + ctxt = xmlSchemaNewMemParserCtxt((char const*)datatypes_src, datatypes_len); + datatypes_schema = schema_parse(ctxt); + xmlSchemaFreeParserCtxt(ctxt); }
switch (dt) @@ -656,7 +624,7 @@ HRESULT dt_validate(XDR_DT dt, xmlChar const* content) xmlSetNs(node, ns); xmlDocSetRootElement(tmp_doc, node);
- hr = Schema_validate_tree(datatypes_schema, (xmlNodePtr)tmp_doc); + hr = schema_validate_tree(datatypes_schema, (xmlNodePtr)tmp_doc); xmlFreeDoc(tmp_doc); } else @@ -844,7 +812,7 @@ static BOOL link_datatypes(xmlDocPtr schema) static cache_entry* cache_entry_from_xsd_doc(xmlDocPtr doc, xmlChar const* nsURI, MSXML_VERSION v) { cache_entry* entry = heap_alloc(sizeof(cache_entry)); - xmlSchemaParserCtxtPtr spctx; + xmlSchemaParserCtxtPtr ctxt; xmlDocPtr new_doc = xmlCopyDoc(doc, 1);
link_datatypes(new_doc); @@ -853,9 +821,9 @@ static cache_entry* cache_entry_from_xsd_doc(xmlDocPtr doc, xmlChar const* nsURI * do we need to do something special here? */ entry->type = CacheEntryType_XSD; entry->ref = 0; - spctx = xmlSchemaNewDocParserCtxt(new_doc); + ctxt = xmlSchemaNewDocParserCtxt(new_doc);
- if ((entry->schema = Schema_parse(spctx))) + if ((entry->schema = schema_parse(ctxt))) { xmldoc_init(entry->schema->doc, v); entry->doc = entry->schema->doc; @@ -868,7 +836,7 @@ static cache_entry* cache_entry_from_xsd_doc(xmlDocPtr doc, xmlChar const* nsURI heap_free(entry); entry = NULL; } - xmlSchemaFreeParserCtxt(spctx); + xmlSchemaFreeParserCtxt(ctxt); return entry; }
@@ -884,7 +852,7 @@ static cache_entry* cache_entry_from_xdr_doc(xmlDocPtr doc, xmlChar const* nsURI entry->ref = 0; spctx = xmlSchemaNewDocParserCtxt(xsd_doc);
- if ((entry->schema = Schema_parse(spctx))) + if ((entry->schema = schema_parse(spctx))) { entry->doc = new_doc; xmldoc_init(entry->schema->doc, version); @@ -1443,7 +1411,7 @@ HRESULT SchemaCache_validate_tree(IXMLDOMSchemaCollection2* iface, xmlNodePtr tr /* TODO: if the ns is not in the cache, and it's a URL, * do we try to load from that? */ if (schema) - return Schema_validate_tree(schema, tree); + return schema_validate_tree(schema, tree); else WARN("no schema found for xmlns=%s\n", get_node_nsURI(tree));
diff --git a/dlls/msxml3/selection.c b/dlls/msxml3/selection.c index ba173f9..61e7f24 100644 --- a/dlls/msxml3/selection.c +++ b/dlls/msxml3/selection.c @@ -738,7 +738,7 @@ static void XSLPattern_OP_IGEq(xmlXPathParserContextPtr pctx, int nargs) xmlFree(arg2); }
-static void query_serror(void* ctx, xmlErrorPtr err) +static void libxml2_selection_structerror(void* ctx, xmlErrorPtr err) { LIBXML2_CALLBACK_SERROR(domselection_create, err); } @@ -767,7 +767,7 @@ HRESULT create_selection(xmlNodePtr node, xmlChar* query, IXMLDOMNodeList **out) init_dispex(&This->dispex, (IUnknown*)&This->IXMLDOMSelection_iface, &domselection_dispex); xmldoc_add_ref(This->node->doc);
- ctxt->error = query_serror; + ctxt->error = libxml2_selection_structerror; ctxt->node = node; registerNamespaces(ctxt);
-- 1.5.6.5
On Wed, Feb 15, 2012 at 11:28:37PM +0100, Marcus Meissner wrote:
On Thu, Feb 16, 2012 at 01:55:44AM +0300, Nikolay Sivov wrote:
The problem is that vsnprintf() was called multiple times with same va_list. Ti fix that it was necessary to get rid of some tracing bits like macro-defined callback calls and a single function for all kinds of error types.
As far as I understand this problem it leads to a stack corruption when va_list is used multiple time without va_start/va_end around it, so it's critical to fix.
If I remember correctly, you can even process a va_list only once on some platforms.
If you need to process it multiple times, you need to create a copy with va_copy() first.
Ciao, Marcus
Correct - on architectures that don't pass all arguments on the stack a va_list is a complex data item that can only be processed once. The Microsoft ABI for amd64 reserves stack space for the arguments passed in registers so that the processing of integer/ptr args is easy. For all Unix OS amd64 passed the first 6 (IIRC) integer/ptr args in normal registers, and the first few FP args in FP regs (regardless of the order of the parameters), the va_list data has to remember which register args have been processed. This is all somewhat tricky! and makes support for printf's argument order selection stuff extremely difficult to write! David -- David Laight: david(a)l8s.co.uk
On 2/16/2012 01:28, Marcus Meissner wrote:
On Thu, Feb 16, 2012 at 01:55:44AM +0300, Nikolay Sivov wrote:
The problem is that vsnprintf() was called multiple times with same va_list. Ti fix that it was necessary to get rid of some tracing bits like macro-defined callback calls and a single function for all kinds of error types.
As far as I understand this problem it leads to a stack corruption when va_list is used multiple time without va_start/va_end around it, so it's critical to fix. If I remember correctly, you can even process a va_list only once on some platforms. We use it that way in couple of places, so it seems to work and I can't find a proper description or part of a standard that says it's not portable.
See winegcc/wrc for --- char* strmake(const char* fmt, ...) --- as an example. That probably means vsnprintf() and similar calls were added as part of C99 as well, so their presence implies working va_copy() is available. Anyway calling it many times with same va_list is broken.
If you need to process it multiple times, you need to create a copy with va_copy() first.
Yes, but that's a part of C99.
Ciao, Marcus
On Thu, Feb 16, 2012 at 03:15:59AM +0300, Nikolay Sivov wrote:
If I remember correctly, you can even process a va_list only once on some platforms. We use it that way in couple of places, so it seems to work and I can't find a proper description or part of a standard that says it's not portable.
It definitely isn't portable. The fact that it works sometimes is irrelevant.
See winegcc/wrc for --- char* strmake(const char* fmt, ...) --- as an example.
That probably means vsnprintf() and similar calls were added as part of C99 as well, so their presence implies working va_copy() is available.
vsnprintf() was added to a lot of system much earlier than va_copy(). Last time I looked I couldn't find a va_copy() for the Microsoft C compiler we were using at the time. In my case it wasn't a printf, but a function that took a list of pointers followed by a NULL - so I copied them into an on-stack array. David -- David Laight: david(a)l8s.co.uk
participants (3)
-
David Laight -
Marcus Meissner -
Nikolay Sivov