[Expat-bugs] [ expat-Bugs-620343 ] segfault: bad API/callback interaction

[SourceForge.net]

Bugs item #620343, was opened at 2002-10-08 12:50
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=110127&aid=620343&group_id=10127

>Category: Documentation
>Group: Test Required
>Status: Open
Resolution: Fixed
Priority: 5
Submitted By: Fred L. Drake, Jr. (fdrake)
Assigned to: Fred L. Drake, Jr. (fdrake)
Summary: segfault: bad API/callback interaction

Initial Comment:
There is a bad interaction between the call to the
start element handler and the Expat API:  After the
presence of the callback is checked, the addBinding()
function can call XML_SetStartElementHandler(parser,
NULL), after which doContent() will still attempt to
call the start element handler.  This results in a call
to the NULL address, resulting in a memory fault.

This is unlikely to be an issue for applications
written entirely in C, but can reasonably happen when a
wrapper library clears callbacks when an error
condition or exception has been detected.

I've attached a patch and a regression test for this case.


----------------------------------------------------------------------

>Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2003-01-28 10:21

Message:
Logged In: YES 
user_id=3066

It probably does.  There should be a bug report on the
documentation, to which I'll respond "we need more tests to
check this", and a year from now we'll decide the lack of
thoses tests won't hold up the next bugfix release.

Or maybe I'll just write thoses tests.  ;-)

For now, re-opening this as a documentation bug (regarding
documented limitations that should no longer exist), and
marked if "Test Required".  Still assigned to me.

----------------------------------------------------------------------

Comment By: Karl Waclawek (kwaclaw)
Date: 2003-01-28 09:05

Message:
Logged In: YES 
user_id=290026

Should my newest fix ("always call storeAtts") not make
the handlkers independent? Therefore, should we not 
remove the documentation item about the "limitation"?

----------------------------------------------------------------------

Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2003-01-28 00:55

Message:
Logged In: YES 
user_id=3066

Since there have been no objections, this is being closed, 
and marked as fixed.

----------------------------------------------------------------------

Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2003-01-21 22:31

Message:
Logged In: YES 
user_id=3066

While I really like the idea that each handler is completely
independent, as the API suggests, I'm not aware of any
real-world use case that requires the namespace callbacks
but not the element boundary callbacks.  Without the use
cases, I think we can live with documented limitations.  As
long as limitations are documented, higher-level layers can
implement the fully-decoupled callbacks model if needed, by
registering "dummy" callbacks as needed to take the place of
application-level callbacks.

Since I've added notes to the documentation about the
limitations relevant to this issue report, I think this is
safe to close if there are no objections in the next day or so.

----------------------------------------------------------------------

Comment By: Karl Waclawek (kwaclaw)
Date: 2003-01-21 14:32

Message:
Logged In: YES 
user_id=290026

The patch passes the OASIS test-suite correctly, but
I am not sure it is worth the extra CPU cycles to make
the namespace declaration handlers truly independent
of the startElementHandler.

Any use cases for that?

----------------------------------------------------------------------

Comment By: Karl Waclawek (kwaclaw)
Date: 2003-01-21 11:59

Message:
Logged In: YES 
user_id=290026

I have attached a patch (ElmHandlers.diff) that calls
storeAtts whenever any of the attribute related handlers
is called, i.e. :
- startElmHandler, 
- startNamespaceDeclHandler, and
- endNamespaceDeclHandler.

The default handler calls are adjusted appropriately.

This patch should make it possible to have the 
namespace declaration handlers called even when 
no startElementHandlers is set.

Please review. Not tested yet.

----------------------------------------------------------------------

Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2003-01-21 11:14

Message:
Logged In: YES 
user_id=3066

Added notes about the limitation in the documentation:
doc/reference.html 1.41

----------------------------------------------------------------------

Comment By: Karl Waclawek (kwaclaw)
Date: 2003-01-21 10:55

Message:
Logged In: YES 
user_id=290026

I had another look and this looks trickier than I thought.
I suggest we add documentation that states that
the start/endNamespaceDeclHandlers will not be called
when the startElementHandler is not set.

I'll post more details in a follow-up.

----------------------------------------------------------------------

Comment By: Karl Waclawek (kwaclaw)
Date: 2002-10-08 13:40

Message:
Logged In: YES 
user_id=290026

I am not *that* eager <g>.
You take care of it.

Thanks!

----------------------------------------------------------------------

Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2002-10-08 13:36

Message:
Logged In: YES 
user_id=3066

Original patch checked in as lib/xmlparse.c 1.92 and
tests/runtests.c 1.36, before I saw Karl's comments.

Karl:  Yes, we should probably fix things so the namespace
decl handlers can be set independently, as the API suggests.
 I can take care of this, unless you beat me to it.  ;-)

Don't know that it warrants a separate issue report, so I'll
leave this one as fixed but open for now.

----------------------------------------------------------------------

Comment By: Karl Waclawek (kwaclaw)
Date: 2002-10-08 13:21

Message:
Logged In: YES 
user_id=290026

Yes, verified. To be precise, addBinding() can call back
to startNamespaceDeclHandler() which then allows
the application to clear the startElementHandler.

One comment: storeAtts() is called like this

  if (startElementHandler) {
    result = storeAtts(parser, ...);
    ...
  }

presumably because the attributes should only be
stored if needed for the startElementHandler.
However, storeAtts does double duty by also
processing namespace declarations and calling
startNamespaceDeclHandler().

So, if that handler is set, should then not storeAtts()
be called like this:

  if (startElementHandler || startNamespaceDeclHandler)
  {
    result = storeAtts(parser, ...);
    ...
  }

And, if true, one should then apply this logic to the
other occasions when storeAtts() is called.

Or does it not make sense to set 
startNamespaceDeclHandler when
startElementHandler is cleared?



----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=110127&aid=620343&group_id=10127