GNOME Bugzilla – Bug 638618
global structured error handler data overrides sax context error handler data
Last modified: 2011-02-24 17:21:12 UTC
Created attachment 177446 [details] testcase If global structured error handler and its data are set, e.g. via xmlSetStructuredErrorFunc() call, then if xmlParserCtxtPtr->sax->error handler is set, it is called with that data instead of xmlParserCtxtPtr->userData. This is a regeression, most probably due to commit LIBXML2.7.3-88-g1de382e. The issue is somewhat similar to #564217 where handler itself was clobbered.
Created attachment 177447 [details] [review] [PATCH 1/4] __xmlRaiseError: do cheap code check early
Created attachment 177448 [details] [review] [PATCH 2/4] __xmlRaiseError: remove redundant schannel initialization
Created attachment 177449 [details] [review] [PATCH 3/4] __xmlRaiseError: fix the structured callback channel's data initialization
Created attachment 177450 [details] [review] [PATCH 4/4] __xmlRaiseError: fix use of the structured callback channel
I've been reported that this bug breaks libxml-ruby.
Just to give credit where credit is due, the issue was raised by Alexey Frolov (ALT libxml-ruby maintainer) along with a testcase down the road: https://bugzilla.altlinux.org/attachment.cgi?id=4726 Then Alexey Gladkov took the bug and further clarified the testcase: https://bugzilla.altlinux.org/attachment.cgi?id=4732 and after discussion with Sergey Vlasov proposed the patch: https://bugzilla.altlinux.org/attachment.cgi?id=4733 I'm wondering why they aren't mentioned as the bug hunting certainly wasn't a pleasant walk.
I've updated commit messages and added one more fix proposed by Sergey Vlasov. One can browse all these commits at http://git.altlinux.org/people/ldv/packages/?p=libxml2.git;a=shortlog;h=fix-bugzilla.gnome.org-638618 or pull them from branch fix-bugzilla.gnome.org-638618 of git://git.altlinux.org/people/ldv/packages/libxml2.git
Okay, the patchset is nice, all make sense I commented on the mailing list and pushed the 4 patches to git, thanks a lot ! Daniel
(In reply to comment #8) > Okay, the patchset is nice, all make sense I commented on the mailing list and > pushed the 4 patches to git, Thanks, but please read comment #7 once more, and while it's not too late, please pull updated commits mentioned there instead of those attached to this bug report.
Sorry I pushed the patches provided in the bug report. If there are other different patches to be made on top of gnome.org current git please provide them as attachment here, or send them to the xml@gnome.org list, If you mean the following patch http://git.altlinux.org/people/ldv/packages/?p=libxml2.git;a=commitdiff;h=4b023ad2a90ca89e8dec7dd8f4558ff4c94f771a;hp=a6bdd724ab846f0920d0889ee37f81ef42247f7e then I'm not so sure about that one, it's perfectly fine for an application to provide it's own ctxt->sax->error and then when doing this + if (channel != NULL) + data = ctxt->userData; you may override the default behaviour they are relying on. As is and without much testing that patch has the potential to break existing applications error handling. Daniel
(In reply to comment #10) > Sorry I pushed the patches provided in the bug report. It's pity, newer commits had more accurate commit messages. > If there are other > different patches to be made on top of gnome.org current git please > provide them as attachment here, OK
Created attachment 181850 [details] [review] [PATCH 5/4] __xmlRaiseError: cleanup initialization of the old callback channel
Created attachment 181851 [details] [review] [PATCH 6/4] __xmlRaiseError: fix initialization of the old callback channel and its data
Created attachment 181852 [details] [review] [PATCH 7/4] New test for structured error handlers
(In reply to comment #10) > If you mean the following patch > http://git.altlinux.org/people/ldv/packages/?p=libxml2.git;a=commitdiff;h=4b023ad2a90ca89e8dec7dd8f4558ff4c94f771a;hp=a6bdd724ab846f0920d0889ee37f81ef42247f7e > > then I'm not so sure about that one, it's perfectly fine for an application > to provide it's own ctxt->sax->error and then when doing this > + if (channel != NULL) > + data = ctxt->userData; > you may override the default behaviour they are relying on. > As is and without much testing that patch has the potential to break > existing applications error handling. I've attached this patch with more context: http://bugzilla-attachments.gnome.org/attachment.cgi?id=181851 You can see from context that this commit doesn't change behaviour that way. What it essentially does is when channel is NULL and ctxt->sax->error is not set, then there is no use to set data to ctxt->userData. In that case, channel/data should be set to xmlGenericError/xmlGenericErrorContext synchronously.