After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 638618 - global structured error handler data overrides sax context error handler data
global structured error handler data overrides sax context error handler data
Status: RESOLVED FIXED
Product: libxml2
Classification: Platform
Component: general
git master
Other All
: Normal major
: ---
Assigned To: Daniel Veillard
libxml QA maintainers
Depends on:
Blocks:
 
 
Reported: 2011-01-04 00:54 UTC by Dmitry V. Levin
Modified: 2011-02-24 17:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
testcase (1.49 KB, application/octet-stream)
2011-01-04 00:54 UTC, Dmitry V. Levin
  Details
[PATCH 1/4] __xmlRaiseError: do cheap code check early (994 bytes, patch)
2011-01-04 01:03 UTC, Dmitry V. Levin
none Details | Review
[PATCH 2/4] __xmlRaiseError: remove redundant schannel initialization (928 bytes, patch)
2011-01-04 01:04 UTC, Dmitry V. Levin
none Details | Review
[PATCH 3/4] __xmlRaiseError: fix the structured callback channel's data initialization (1017 bytes, patch)
2011-01-04 01:05 UTC, Dmitry V. Levin
none Details | Review
[PATCH 4/4] __xmlRaiseError: fix use of the structured callback channel (1.48 KB, patch)
2011-01-04 01:06 UTC, Dmitry V. Levin
none Details | Review
[PATCH 5/4] __xmlRaiseError: cleanup initialization of the old callback channel (1.35 KB, patch)
2011-02-24 17:08 UTC, Dmitry V. Levin
none Details | Review
[PATCH 6/4] __xmlRaiseError: fix initialization of the old callback channel and its data (1.56 KB, patch)
2011-02-24 17:08 UTC, Dmitry V. Levin
none Details | Review
[PATCH 7/4] New test for structured error handlers (5.30 KB, patch)
2011-02-24 17:09 UTC, Dmitry V. Levin
none Details | Review

Description Dmitry V. Levin 2011-01-04 00:54:02 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.
Comment 1 Dmitry V. Levin 2011-01-04 01:03:51 UTC
Created attachment 177447 [details] [review]
[PATCH 1/4] __xmlRaiseError: do cheap code check early
Comment 2 Dmitry V. Levin 2011-01-04 01:04:47 UTC
Created attachment 177448 [details] [review]
[PATCH 2/4] __xmlRaiseError: remove redundant schannel initialization
Comment 3 Dmitry V. Levin 2011-01-04 01:05:24 UTC
Created attachment 177449 [details] [review]
[PATCH 3/4] __xmlRaiseError: fix the structured callback channel's data initialization
Comment 4 Dmitry V. Levin 2011-01-04 01:06:23 UTC
Created attachment 177450 [details] [review]
[PATCH 4/4] __xmlRaiseError: fix use of the structured callback channel
Comment 5 Dmitry V. Levin 2011-01-05 14:45:18 UTC
I've been reported that this bug breaks libxml-ruby.
Comment 6 Michael Shigorin 2011-01-08 11:31:47 UTC
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.
Comment 7 Dmitry V. Levin 2011-01-12 18:10:47 UTC
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
Comment 8 Daniel Veillard 2011-02-23 14:56:37 UTC
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
Comment 9 Dmitry V. Levin 2011-02-23 15:07:53 UTC
(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.
Comment 10 Daniel Veillard 2011-02-24 03:31:03 UTC
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
Comment 11 Dmitry V. Levin 2011-02-24 16:40:33 UTC
(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
Comment 12 Dmitry V. Levin 2011-02-24 17:08:07 UTC
Created attachment 181850 [details] [review]
[PATCH 5/4] __xmlRaiseError: cleanup initialization of the old callback channel
Comment 13 Dmitry V. Levin 2011-02-24 17:08:44 UTC
Created attachment 181851 [details] [review]
[PATCH 6/4] __xmlRaiseError: fix initialization of the old callback channel and its data
Comment 14 Dmitry V. Levin 2011-02-24 17:09:26 UTC
Created attachment 181852 [details] [review]
[PATCH 7/4] New test for structured error handlers
Comment 15 Dmitry V. Levin 2011-02-24 17:21:12 UTC
(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.