Bug 17353 – XMLTokenizer installs global libxml2 callbacks that can break client applications

• Component: XML
• Status: RESOLVED FIXED

[Alp Toker]

The developers of Yelp (the GNOME integrated help system) found that the XML parsing functionality of their application started to fail after they replaced Mozilla with a WebKit GTK+ WebView widget.

Their WebView code is completely separate from the XML parsing code (which uses libxml2), but both run in the same process.

I did some debugging and tracked down the issue. XMLTokenizer.cpp assigns global match/open/read/write/close functions. By modifying the behaviour of these functions, WebKit cripples XML features throughout the embedding application:

static xmlParserCtxtPtr createStringParser(xmlSAXHandlerPtr handlers, void* userData)
{
    static bool didInit = false;
    if (!didInit) {
        xmlInitParser();
        xmlRegisterInputCallbacks(matchFunc, openFunc, readFunc, closeFunc);
        xmlRegisterOutputCallbacks(matchFunc, openFunc, writeFunc, closeFunc);
        didInit = true;
    }

...

I'm not familiar with XMLTokenizer.cpp so not sure how to proceed. Any thoughts?

[Mark Rowe (bdash)]

The IO callbacks registered by xmlRegisterInputCallbacks/xmlRegisterOutputCallbacks go onto a global stack of callbacks that are asked in turn whether they can handle the IO for a given URL.  The fact that these can only be set globally looks to be a limitation of libxml2.  The fact that the callbacks are only provided with the URL when asked whether they can handle the load makes it challenging to have WebCore's callbacks limit themselves to handling IO for WebCore-related resources.  Not using WebCore's callbacks when globalDocLoader may reduce the problem a little, but is unlikely to be safe in a multithreaded application.

What is the failure mode that is occurring in applications?  My reading of the code suggests that documents may be returned as zero-length if there is no associated WebCore loader, which I would imagine to be the case for uses of libxml2 outside WebCore.

[Alp Toker]

Original bug report:



Hi,

(Feel free to forward this to anyone you feel like / might be more
appropriate)

As you may have seen on planet.gnome.org [1]), I'm trying to get yelp
running with webkit.  I'm hitting some issues working with libxml /
libxslt and webkit_web_view_load_string which I'm using to load local
files.

The problem manifests when I set the mime type to "application/xhtml
+xml", which is required for all our documents.

The problem is that when I set the mime type to application/xhtml+xml,
any subsequent calls to xslt will fail as libxml thinks the document is
empty.  I'm not entirely certain why it thinks this, so I thought I'd
get some expert opinion  ;) 

I've produced a test program, based on the example browser and which can
be found at:
http://www.gnome.org/~dscorgie/main.c

Vague instructions.  Compile using:
gcc -o main main.c `pkg-config --cflags --libs WebKitGtk libxslt`

then run with:
./main text/html
to run the program working.  When you do this, click the link and press
back.  You should see output in the terminal like:
ss1: 0x807d690
ss2: 0x82261e8
which means libxslt has successfully parsed the requested stylesheet.

The output are 2 pointers to stylesheets, the first (ss1) is generated just before running 
the web_view and the second (ss2) generated in the "go_back" callback (for lack of a better place).

To change the mime type to xhtml, you can omit the argument:
./main
which should produce the dreaded output (after clicking the link and pressing back):
ss1: 0x807d690
/usr/share/yelp/xslt/yelp-common.xsl:1: parser error : Document is empty

^
/usr/share/yelp/xslt/yelp-common.xsl:1: parser error : Start tag expected, '<' not found

^
error
xsltParseStylesheetFile : cannot parse /usr/share/yelp/xslt/yelp-common.xsl
ss2: (nil)

which means the program now thinks /usr/share/yelp/xslt/yelp-common.xsl is empty.

I feel I'm doing something stupid, but have been staring at this (and tracing through libxml, 
libxslt and yelp) for 2 weeks and still can't see it.

Oh, I should also mention you need yelp installed on you're system (or change the example to point to 
a different stylesheet in both places).  Also, this is with nightly builds r29907 and r30123 (the latest).

Thanks
Don

[1] progress can be tracked in the bug:
http://bugzilla.gnome.org/show_bug.cgi?id=512827

[Alp Toker]

Created attachment 19125 [details]
Proposed fix

This fixes the reported breakage in applications.

[Darin Adler]

Comment on attachment 19125 [details]
Proposed fix

r=me, with some reservations.

Doesn't this still leave us with the problem that other libxml2 clients could call xmlRegisterInputCallbacks and clobber our callbacks with their own? Is there a "save and restore" approach we could use instead?

Should we file a bug asking libxml2 to come up with a way of doing this that's not global?

+    // TODO: We should restore the original global error handler as well.

We use FIXME, not TODO, for these things.

[Alp Toker]

(In reply to comment #3)
> Created an attachment (id=19125) [edit]
> Proposed fix
> 
> This fixes the reported breakage in applications.
> 

The patch deals with all potential re-entrancy I can think of but any thoughts here are welcome.

It might be worth filing a follow-up bug to track/audit other issues like this in WebCore. The failure mode is very obscure and we're quite lucky this bug was reported to us.

[Alp Toker]

(In reply to comment #4)
> (From update of attachment 19125 [details] [edit])
> r=me, with some reservations.
> 
> Doesn't this still leave us with the problem that other libxml2 clients could
> call xmlRegisterInputCallbacks and clobber our callbacks with their own? Is
> there a "save and restore" approach we could use instead?

Yep, totally. It's not a complete fix. Luckily very few applications seem to use xmlRegisterInputCallbacks(), so this saves our skin for now.

I tried cooking up a save/restore form of this patch but it still didn't address the reverse scenario you described. Could you elaborate or maybe give a proof of concept?

> 
> Should we file a bug asking libxml2 to come up with a way of doing this that's
> not global?

I think dv is aware that the globals are bad. It's possible they've added new a new way to do this and we haven't seen it yet.

We should file a bug or check up on the existing one (Eric believes a bug has already been filed).

[Alp Toker]

Comment on attachment 19125 [details]
Proposed fix

Patch landed in r30236. Will keep the bug open at least for a few days to see how things pan out. Maybe we can come up with a more complete solution.

[Alp Toker]

Guess we can close this.