Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds missing call of endDocument, fixes SR-2301 #2877

Merged
merged 2 commits into from
Sep 13, 2020

Conversation

dhoepfl
Copy link
Contributor

@dhoepfl dhoepfl commented Sep 10, 2020

SR-2301 was mostly fixed by pull request #700 (commit 30b17ed) in 2016.
The fix however still does not call the endDocument callback.
This pull request adds the necessary end of document signaling to SAX.

This pull also fixes several minor (rare) bugs like unchecked error conditions, incorrect handling of small chuncks, etc.

Please note the change in CFXMLInterface: I’m not sure if this is the best place to handle this. The interface is undocumented and returning an error that the end of the document has been reached when you send the last piece of data was unexpected. Even more so since the meaning of the return values is undocumented and the internal (SAX) errors are not mapped to the ErrorCode enum of XMLParser (Leaving this for a different fix).

Fixed: Malloc errors were unchecked.
Fixed: Out of memory when creating context was ignored.
Improved: Parsing from data allocated memory that was never used.
Fixed: Parsing from InputStream ignored the parser return value.
Fixed: Reading the BOM returned an error if the BOM is not returned in
       one chunck from InputStream, e.g. when InputStreams returns one byte
       after the other on read.
Fixed by signaling the final ("terminal") chunk to SAX.
return result
}

// called to start the event-driven parse. Returns YES in the event of a successful parse, and NO in case of error.
open func parse() -> Bool {
return parseFromStream()
XMLParser.setCurrentParser(self)
defer { XMLParser.setCurrentParser(nil) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I start another parser from a parser callback, will this be reentrant correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand the it: No it will not.

On the other side: That’s no difference from the current code. (old: parse → parseFromStream → setCurrentParser → parseData → callback, new: parse → setCurrentParser [→ parseFromStream] → parseData → callback)

@millenomi
Copy link
Contributor

@dhoepfl still interested in shepherding this?

@dhoepfl
Copy link
Contributor Author

dhoepfl commented Sep 11, 2020

@dhoepfl still interested in shepherding this?

I think the reentrance problem you found is valid and should be tracked as a separate issue, because this might result in a major refactoring of XMLParser (beyond what I’m capable of doing). I will try to find time to file an issue for it tomorrow.

@dhoepfl
Copy link
Contributor Author

dhoepfl commented Sep 13, 2020

Created SR-13546 regarding the reentrance problem.

@millenomi
Copy link
Contributor

@swift-ci please rest and merge

@dhoepfl
Copy link
Contributor Author

dhoepfl commented Sep 13, 2020

@swift-ci please rest and merge

What a great typo. Bots deserve a rest, too.

@millenomi
Copy link
Contributor

@swift-ci please test and merge

@millenomi
Copy link
Contributor

Both me and the CI bot needed rest, whoops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants