Re: [xml] interaction between xmlSetGenericErrorFunc and xmlSetStructuredErrorFunc can crash (draft patch available)

[Daniel Veillard <veillard redhat com>]

On Mon, Jul 27, 2009 at 08:05:37PM -0700, Wang Lam wrote:
> Hi!  I came across some (unfortunate) behavior in libxml2 (2.7.3
> and head) error handling that I want to run by you for your comments.
> I have a patch that may help one aspect of it, if you are interested.

  Yes, I am, sorry for the late reply, I was really busy lately.


[...]
> // 1
> Based on the API description and mailing-list messages--
>
>       [Re: [xml] libxml error redirection]
>       http://mail.gnome.org/archives/xml/2006-April/msg00090.html
>
>       [Re: [xml] Error handling]
>       http://mail.gnome.org/archives/xml/2005-May/msg00134.html
>
> --this function (xmlSetStructuredErrorFunc) seems to be the preferable
> way to capture errors.

  yes


> (a) xmlGenericError still gets called, which seems surprising and
>     unfortunate in light of the xmlSetStructuredErrorFunc call.

  yes as you foudn out there is still many many calls internally to
xmlGenericError(), that should be cleaned up in some ways !

[...]
> (b) Worse than being invoked, though, the default xmlGenericError
>     crashed (instead of, say, printing to stderr).
>
>     From what I see, xmlGenericErrorFunc and xmlStructuredErrorFunc
>     share a single xmlGenericErrorContext; setting one function
>     overwrites the other function's context.  In the above example
>     code, the default xmlGenericErrorDefaultFunc expects its context
>     to be a FILE * (or NULL, to default to stderr), and my custom
>     xmlStructuredErrorFunc expects a pointer to a writable error-string
>     buffer (for example).
>
>     Locally, I work around the problem by writing both error-handling
>     functions (and setting them both together) so that they can share
>     one context.  It's also possible, though, to create a separate
>     void * xmlStructuredErrorContext so that the two functions don't
>     have to fight over the same state.

  yes, I agree with the diagnostic, I came up to the same conclusion,
this need fixing !

>     With a patch to create xmlStructuredErrorContext, I get a different
>     result from the above example code:

  yes, I think I know what you patch does, add the new variable in
  globals.c, initialize it properly, and use it for the strcutured
  error paths in error.c

> While I'd naturally prefer (a) to be resolved, I don't know whether
> it is easy or not (whether anybody is able to go through the code
> to assert or verify its resolution).  On the other hand, I do have
> a draft patch lying around (relative to libxml head) for (b).

  I'm interested in the b) patch, that would avoid the need for me to
do the exact same thing !

> So, I wonder--
>
> * Are (a) and (b) behaviors that libxml2's maintainers would also
>   like changed, or do they represent desired behavior for the system
>   (e.g., the system is "supposed to" become undefined in // 2)?

  I ideally would like to have both changed, but realistically right now
fixing b) might be sufficient.


> * Should I file a bug entry and submit my draft patch for (b) (to create
>   a new void *xmlStructuredErrorContext)?  (It's about 451 lines, so
>   I don't have it attached to this already-long message, but as you'd
>   quickly see, most of those lines are just side effects of libxml2's
>   internal code generation.)

  yes please or just reply with the attached patch, I expect to do
  libxml2 maintainace today and tomorrow !

> Your thoughts or suggestions would be welcome.  Thanks!

  thanks a lot for the report and details, spot on !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/