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@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;elseneeded = 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);
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);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
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 -
Marcus Meissner -
Nikolay Sivov