Skip to content

Commit

Permalink
Don't load external entity from xmlSAX2GetEntity
Browse files Browse the repository at this point in the history
Despite the comment, I can't see a reason why external entities must be
loaded in the SAX handler. For external entities, the handler is
typically first invoked via xmlParseReference which will later load the
entity on its own if it wasn't loaded yet.

The old code also lead to duplicated SAX events which makes it
basically impossible to reuse xmlSAX2GetEntity for a custom SAX parser.
See the change to the expected test output.

Note that xmlSAX2GetEntity was loading the entity via
xmlParseCtxtExternalEntity while xmlParseReference uses
xmlParseExternalEntityPrivate. In the previous commit, the two
functions were merged, trying to compensate for some slight differences
between the two mostly identical implementations.

But the more urgent reason for this change is that xmlParseReference
has the facility to abort early when recursive entities are detected,
avoiding what could practically amount to an infinite loop.

If you want to backport this change, note that the previous three
commits are required as well:

f9ea1a2 Fix copying of entities in xmlParseReference
5c7e0a9 Copy some XMLReader option flags to parser context
1a3e584 Merge code paths loading external entities

Found by OSS-Fuzz.
  • Loading branch information
nwellnhof committed Feb 11, 2020
1 parent 1a3e584 commit eddfbc3
Show file tree
Hide file tree
Showing 2 changed files with 0 additions and 37 deletions.
30 changes: 0 additions & 30 deletions SAX2.c
Original file line number Diff line number Diff line change
Expand Up @@ -590,36 +590,6 @@ xmlSAX2GetEntity(void *ctx, const xmlChar *name)
} else {
ret = xmlGetDocEntity(ctxt->myDoc, name);
}
if ((ret != NULL) &&
((ctxt->validate) || (ctxt->replaceEntities)) &&
(ret->children == NULL) &&
(ret->etype == XML_EXTERNAL_GENERAL_PARSED_ENTITY)) {
int val;

/*
* for validation purposes we really need to fetch and
* parse the external entity
*/
xmlNodePtr children;
unsigned long oldnbent = ctxt->nbentities;

val = xmlParseCtxtExternalEntity(ctxt, ret->URI,
ret->ExternalID, &children);
if (val == 0) {
xmlAddChildList((xmlNodePtr) ret, children);
} else {
xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_PROCESSING,
"Failure to process entity %s\n", name, NULL);
ctxt->validate = 0;
return(NULL);
}
ret->owner = 1;
if (ret->checked == 0) {
ret->checked = (ctxt->nbentities - oldnbent + 1) * 2;
if ((ret->content != NULL) && (xmlStrchr(ret->content, '<')))
ret->checked |= 1;
}
}
return(ret);
}

Expand Down
7 changes: 0 additions & 7 deletions result/noent/ent2.sax2
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,6 @@ SAX.characters(my title, 8)
SAX.endElementNs(title, NULL, NULL)
SAX.characters(
, 1)
SAX.ignorableWhitespace(
, 1)
SAX.startElementNs(title, NULL, NULL, 0, 0, 0)
SAX.characters(my title, 8)
SAX.endElementNs(title, NULL, NULL)
SAX.characters(
, 1)
SAX.characters(
This text is about XML, the, 31)
SAX.getEntity(xml)
Expand Down

0 comments on commit eddfbc3

Please sign in to comment.