[Expat-bugs] [ expat-Bugs-1515266 ] missing check of stopped parser in doContext() 'for' loop

[SourceForge.net]

Bugs item #1515266, was opened at 2006-06-30 14:04
Message generated for change (Comment added) made by kwaclaw
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=110127&aid=1515266&group_id=10127

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: Test Required
Status: Open
>Resolution: Fixed
Priority: 6
Submitted By: Brett Cannon (bcannon)
Assigned to: Fred L. Drake, Jr. (fdrake)
Summary: missing check of stopped parser in doContext() 'for' loop

Initial Comment:
In Expat 2.0.0, in expat.c:doConvert() there is a 'for'
loop for the XML_TOK_DATA_CHARS case.  There is
unfortunately no check in that loop whether the parser
was stopped during that call because of an error.

This was discovered in Python
(Lib/test/crashers/xml_parsers.py) because pyexpat,
upon error where there is no error return code like
with characterDataHandlers, sets all handlers to 0,
sets parsingStatus to XML_FINISHED, and sets errorCode.
 This leads to a segfault if the 'for' loop goes around
again because parser->m_characterDataHandler is set to 0.

A simple check if the parser is stopped fixes the
problem.  I have attached a simple patch that just
breaks out of the loop and lets execution fall through
to the bottom of the 'switch' statement.  I don't know
if returning errorCode directly would be better or if
checking for XML_SUSPENDED is also desirable.

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

>Comment By: Karl Waclawek (kwaclaw)
Date: 2006-07-05 09:14

Message:
Logged In: YES 
user_id=290026

Applied the patch for bug # 1515600 which solves this issue
as well. Removed the check for XML_FINISHED/XML_SUSPENDED.
We could discuss special treatment of XML_FINISHED, but if
one is clearing all handlers anyway, then special treatment
of XML_FINISHED is not necessary.

For Fred: I have not re-run the test cases. Please do so and
close the issue if successful.



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

Comment By: Karl Waclawek (kwaclaw)
Date: 2006-07-04 09:37

Message:
Logged In: YES 
user_id=290026

I am re-opening this issue because in the case of a
suspended parser, breaking out of the inner loop in
XML_TOK_DATA_CHARS means that character call-backs are
missed when resuming the parser. We should let the inner
loop finish reporting all characters.

The documentation already states that after calling
XML_StopParser() there may still be a few call-backs that
would otherwise be missed, so this would not be new
behaviour, but consistent with existing behaviour.

The solution to the problem described is the same as
suggested for bug # 1515600 (Segfault after removing
character data handler). Just put the NULL check for the
character data handler inside the internal loop.

Btw, the same problem exists in the doCdataSection()
function. I'll attach a patch suggestion to bug # 1515600.

We might decide to treat XML_FINISHED different from
XML_SUSPENDED such that no other call-backs will happen, but
in that case we need to review all the other places where
this would need to be done as well (and update the
documentation, of course).

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

Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2006-07-01 11:32

Message:
Logged In: YES 
user_id=3066

Confirmed that the suspend behavior parallels the abort
behavior Brett's patch fixed; fixed and added a regression
test in lib/xmlparse.c 1.155 and tests/runtests.c 1.66.

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

Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2006-07-01 11:02

Message:
Logged In: YES 
user_id=3066

Added a regression test in tests/runtests.c revision 1.65.

Closing this report.

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

Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2006-07-01 00:00

Message:
Logged In: YES 
user_id=3066

That seems fine, but can be done faster within the Expat
implementation.  I've committed the simplified patch as
lib/xmlparse.c revision 1.154.

I'll have a test case committed tomorrow as well.  Leaving
this report open for now since I need to finish up the test
case.

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

Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2006-06-30 14:40

Message:
Logged In: YES 
user_id=3066

The Python folks need this dealt with before Python 2.5, so 
I'll try and take a look at it this weekend if no one beats 
me to it.

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

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