Re: [xml] [PATCH] Fix xsltExtModuleFunctionLookup() / xsltExtModuleRegisterDynamic() deadlock.

[Nix <nix esperi org uk>]

On 8 Sep 2009, Daniel Veillard spake thusly:

> On Mon, Sep 07, 2009 at 10:50:42PM +0100, Nix wrote:
> > Fix a deadlock when encountering extensions that do not exist on the supported platform at
> > all. Both xsltExtModuleFunctionLookup() and xsltExtModuleRegisterDynamic() take the
> > xsltExtMutex, but the former calls the latter -> deadlock.
> >
> > Dropping the mutex and taking it again is safe, as xsltExtModuleRegisterDynamic() checks that
> > it has not already been called for a particular module. So do that.
>
>   Ah right, that's my fault, I added mutex access to allow concurent
> access but I didn't realize the deadlock. The code is in git but not
> released yet, so hopefully the problem is limited :-)

Yeah: I only noticed 'cos I just built a bleeding-edge-everything system and
noticed KDE's meinproc locking up...

> > diff --git a/libxslt/extensions.c b/libxslt/extensions.c
> > index 3b99de8..a797c39 100644
> > --- a/libxslt/extensions.c
> > +++ b/libxslt/extensions.c
> > @@ -1424,6 +1424,8 @@ xsltExtModuleFunctionLookup(const xmlChar * name, const xmlChar * URI)
> >  
> >      XML_CAST_FPTR(ret) = xmlHashLookup2(xsltFunctionsHash, name, URI);
> >  
> > +    xmlMutexUnlock(xsltExtMutex);
> > +
> >      /* if lookup fails, attempt a dynamic load on supported platforms */
> >      if (NULL == ret) {
> >          if (!xsltExtModuleRegisterDynamic(URI)) {
> > @@ -1432,8 +1434,6 @@ xsltExtModuleFunctionLookup(const xmlChar * name, const xmlChar * URI)
>
> Good patch but the mutext should be taken/released again around the second
> Lookup.

There's a second lookup? ... whoops, quite missed that. It would perhaps
help if there was a comment somewhere saying what resources this mutex
was meant to be protecting: i.e., the xsltModuleHash table. (But perhaps
to one more awake than I this would have been obvious.)

I notice that xsltExtModuleRegisterDynamic()'s doc comment says 'Always
called with xsltExtMutex lock taken': since this is demonstrably untrue
(there is only one such call site, and xsltExtModuleRegisterDynamic()
takes the lock itself), probably this comment-residue should be removed
as well.

>   Applied and commited to Git, please double check if you have a chance,

Looks fine, I'll do a test KDE3 rebuild soon and see if it locks up.