From 5f440d8cadea9f0d87fd3849366445029d47f528 Mon Sep 17 00:00:00 2001 From: Nick Wellnhofer Date: Mon, 12 Jun 2017 14:32:34 +0200 Subject: [PATCH] Rework entity boundary checks Make sure to finish all entities in the internal subset. Nevertheless, readd a sanity check in xmlParseStartTag2 that was lost in my previous commit. Also add a sanity check in xmlPopInput. Popping an input unexpectedly was the source of many recent memory bugs. The check doesn't mitigate such issues but helps with diagnosis. Always base entity boundary checks on the input ID, not the input pointer. The pointer could have been reallocated to the old address. Always throw a well-formedness error if a boundary check fails. In a few places, a validity error was thrown. Fix a few error codes and improve indentation. --- parser.c | 130 +++++++++++++++++++++------------ result/errors/754946.xml.err | 25 ++++--- result/errors/754946.xml.str | 4 +- result/errors10/781205.xml.err | 20 ++--- result/valid/t8.xml.err | 20 ++--- result/valid/t8.xml.err.rdr | 14 +++- result/valid/t8a.xml.err | 20 ++--- result/valid/t8a.xml.err.rdr | 14 +++- test/errors/754946.xml | 5 +- 9 files changed, 153 insertions(+), 99 deletions(-) diff --git a/parser.c b/parser.c index 862f4fbb..30a41dd1 100644 --- a/parser.c +++ b/parser.c @@ -2215,6 +2215,10 @@ xmlPopInput(xmlParserCtxtPtr ctxt) { if (xmlParserDebugEntities) xmlGenericError(xmlGenericErrorContext, "Popping input %d\n", ctxt->inputNr); + if ((ctxt->inputNr > 1) && (ctxt->inSubset == 0) && + (ctxt->instate != XML_PARSER_EOF)) + xmlFatalErr(ctxt, XML_ERR_INTERNAL_ERROR, + "Unfinished entity outside the DTD"); xmlFreeInputStream(inputPop(ctxt)); if ((*ctxt->input->cur == 0) && (xmlParserInputGrow(ctxt->input, INPUT_CHUNK) <= 0)) @@ -4837,7 +4841,8 @@ xmlParseCommentComplex(xmlParserCtxtPtr ctxt, xmlChar *buf, } else { if (inputid != ctxt->input->id) { xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, - "Comment doesn't start and stop in the same entity\n"); + "Comment doesn't start and stop in the same" + " entity\n"); } NEXT; if ((ctxt->sax != NULL) && (ctxt->sax->comment != NULL) && @@ -4985,7 +4990,8 @@ get_more: if (in[2] == '>') { if (ctxt->input->id != inputid) { xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, - "comment doesn't start and stop in the same entity\n"); + "comment doesn't start and stop in the" + " same entity\n"); } SKIP(3); if ((ctxt->sax != NULL) && (ctxt->sax->comment != NULL) && @@ -5153,7 +5159,7 @@ xmlParsePI(xmlParserCtxtPtr ctxt) { int count = 0; if ((RAW == '<') && (NXT(1) == '?')) { - xmlParserInputPtr input = ctxt->input; + int inputid = ctxt->input->id; state = ctxt->instate; ctxt->instate = XML_PARSER_PI; /* @@ -5169,9 +5175,10 @@ xmlParsePI(xmlParserCtxtPtr ctxt) { target = xmlParsePITarget(ctxt); if (target != NULL) { if ((RAW == '?') && (NXT(1) == '>')) { - if (input != ctxt->input) { + if (inputid != ctxt->input->id) { xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, - "PI declaration doesn't start and stop in the same entity\n"); + "PI declaration doesn't start and stop in" + " the same entity\n"); } SKIP(2); @@ -5253,9 +5260,10 @@ xmlParsePI(xmlParserCtxtPtr ctxt) { xmlFatalErrMsgStr(ctxt, XML_ERR_PI_NOT_FINISHED, "ParsePI: PI %s never end ...\n", target); } else { - if (input != ctxt->input) { - xmlFatalErrMsg(ctxt, XML_ERR_SPACE_REQUIRED, - "PI declaration doesn't start and stop in the same entity\n"); + if (inputid != ctxt->input->id) { + xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, + "PI declaration doesn't start and stop in" + " the same entity\n"); } SKIP(2); @@ -5311,7 +5319,7 @@ xmlParseNotationDecl(xmlParserCtxtPtr ctxt) { xmlChar *Systemid; if (CMP10(CUR_PTR, '<', '!', 'N', 'O', 'T', 'A', 'T', 'I', 'O', 'N')) { - xmlParserInputPtr input = ctxt->input; + int inputid = ctxt->input->id; SHRINK; SKIP(10); if (!IS_BLANK_CH(CUR)) { @@ -5345,9 +5353,10 @@ xmlParseNotationDecl(xmlParserCtxtPtr ctxt) { SKIP_BLANKS; if (RAW == '>') { - if (input != ctxt->input) { - xmlFatalErrMsg(ctxt, XML_ERR_SPACE_REQUIRED, - "Notation declaration doesn't start and stop in the same entity\n"); + if (inputid != ctxt->input->id) { + xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, + "Notation declaration doesn't start and stop" + " in the same entity\n"); } NEXT; if ((ctxt->sax != NULL) && (!ctxt->disableSAX) && @@ -5395,7 +5404,7 @@ xmlParseEntityDecl(xmlParserCtxtPtr ctxt) { /* GROW; done in the caller */ if (CMP8(CUR_PTR, '<', '!', 'E', 'N', 'T', 'I', 'T', 'Y')) { - xmlParserInputPtr input = ctxt->input; + int inputid = ctxt->input->id; SHRINK; SKIP(8); skipped = SKIP_BLANKS; @@ -5594,9 +5603,10 @@ xmlParseEntityDecl(xmlParserCtxtPtr ctxt) { "xmlParseEntityDecl: entity %s not terminated\n", name); xmlHaltParser(ctxt); } else { - if (input != ctxt->input) { + if (inputid != ctxt->input->id) { xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, - "Entity declaration doesn't start and stop in the same entity\n"); + "Entity declaration doesn't start and stop in" + " the same entity\n"); } NEXT; } @@ -5964,7 +5974,7 @@ xmlParseAttributeListDecl(xmlParserCtxtPtr ctxt) { xmlEnumerationPtr tree; if (CMP9(CUR_PTR, '<', '!', 'A', 'T', 'T', 'L', 'I', 'S', 'T')) { - xmlParserInputPtr input = ctxt->input; + int inputid = ctxt->input->id; SKIP(9); if (!IS_BLANK_CH(CUR)) { @@ -6060,10 +6070,10 @@ xmlParseAttributeListDecl(xmlParserCtxtPtr ctxt) { GROW; } if (RAW == '>') { - if (input != ctxt->input) { - xmlValidityError(ctxt, XML_ERR_ENTITY_BOUNDARY, - "Attribute list declaration doesn't start and stop in the same entity\n", - NULL, NULL); + if (inputid != ctxt->input->id) { + xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, + "Attribute list declaration doesn't start and" + " stop in the same entity\n"); } NEXT; } @@ -6100,10 +6110,10 @@ xmlParseElementMixedContentDecl(xmlParserCtxtPtr ctxt, int inputchk) { SKIP_BLANKS; SHRINK; if (RAW == ')') { - if ((ctxt->validate) && (ctxt->input->id != inputchk)) { - xmlValidityError(ctxt, XML_ERR_ENTITY_BOUNDARY, -"Element content declaration doesn't start and stop in the same entity\n", - NULL, NULL); + if (ctxt->input->id != inputchk) { + xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, + "Element content declaration doesn't start and" + " stop in the same entity\n"); } NEXT; ret = xmlNewDocElementContent(ctxt->myDoc, NULL, XML_ELEMENT_CONTENT_PCDATA); @@ -6159,10 +6169,10 @@ xmlParseElementMixedContentDecl(xmlParserCtxtPtr ctxt, int inputchk) { } if (ret != NULL) ret->ocur = XML_ELEMENT_CONTENT_MULT; - if ((ctxt->validate) && (ctxt->input->id != inputchk)) { - xmlValidityError(ctxt, XML_ERR_ENTITY_BOUNDARY, -"Element content declaration doesn't start and stop in the same entity\n", - NULL, NULL); + if (ctxt->input->id != inputchk) { + xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, + "Element content declaration doesn't start and" + " stop in the same entity\n"); } SKIP(2); } else { @@ -6402,10 +6412,10 @@ xmlParseElementChildrenContentDeclPriv(xmlParserCtxtPtr ctxt, int inputchk, if (last != NULL) last->parent = cur; } - if ((ctxt->validate) && (ctxt->input->id != inputchk)) { - xmlValidityError(ctxt, XML_ERR_ENTITY_BOUNDARY, -"Element content declaration doesn't start and stop in the same entity\n", - NULL, NULL); + if (ctxt->input->id != inputchk) { + xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, + "Element content declaration doesn't start and stop in" + " the same entity\n"); } NEXT; if (RAW == '?') { @@ -6578,7 +6588,7 @@ xmlParseElementDecl(xmlParserCtxtPtr ctxt) { /* GROW; done in the caller */ if (CMP9(CUR_PTR, '<', '!', 'E', 'L', 'E', 'M', 'E', 'N', 'T')) { - xmlParserInputPtr input = ctxt->input; + int inputid = ctxt->input->id; SKIP(9); if (!IS_BLANK_CH(CUR)) { @@ -6644,9 +6654,10 @@ xmlParseElementDecl(xmlParserCtxtPtr ctxt) { xmlFreeDocElementContent(ctxt->myDoc, content); } } else { - if (input != ctxt->input) { + if (inputid != ctxt->input->id) { xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, - "Element declaration doesn't start and stop in the same entity\n"); + "Element declaration doesn't start and stop in" + " the same entity\n"); } NEXT; @@ -6699,9 +6710,9 @@ xmlParseConditionalSections(xmlParserCtxtPtr ctxt) { return; } else { if (ctxt->input->id != id) { - xmlValidityError(ctxt, XML_ERR_ENTITY_BOUNDARY, - "All markup of the conditional section is not in the same entity\n", - NULL, NULL); + xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, + "All markup of the conditional section is not" + " in the same entity\n"); } NEXT; } @@ -6762,9 +6773,9 @@ xmlParseConditionalSections(xmlParserCtxtPtr ctxt) { return; } else { if (ctxt->input->id != id) { - xmlValidityError(ctxt, XML_ERR_ENTITY_BOUNDARY, - "All markup of the conditional section is not in the same entity\n", - NULL, NULL); + xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, + "All markup of the conditional section is not" + " in the same entity\n"); } NEXT; } @@ -6826,9 +6837,9 @@ xmlParseConditionalSections(xmlParserCtxtPtr ctxt) { xmlFatalErr(ctxt, XML_ERR_CONDSEC_NOT_FINISHED, NULL); } else { if (ctxt->input->id != id) { - xmlValidityError(ctxt, XML_ERR_ENTITY_BOUNDARY, - "All markup of the conditional section is not in the same entity\n", - NULL, NULL); + xmlFatalErrMsg(ctxt, XML_ERR_ENTITY_BOUNDARY, + "All markup of the conditional section is not in" + " the same entity\n"); } if ((ctxt-> instate != XML_PARSER_EOF) && ((ctxt->input->cur + 3) <= ctxt->input->end)) @@ -8382,13 +8393,27 @@ xmlParseInternalSubset(xmlParserCtxtPtr ctxt) { /* * Pop-up of finished entities. */ - while ((RAW == 0) && (ctxt->inputNr > 1)) - xmlPopInput(ctxt); + while (ctxt->inputNr > 1) { + if (RAW == 0) { + xmlPopInput(ctxt); + } else if (RAW == ']') { + /* + * Make sure not to return with unfinished entities. + */ + xmlFatalErr(ctxt, XML_ERR_EXT_SUBSET_NOT_FINISHED, NULL); + xmlPopInput(ctxt); + } else { + break; + } + } if ((CUR_PTR == check) && (cons == ctxt->input->consumed)) { xmlFatalErr(ctxt, XML_ERR_INTERNAL_ERROR, "xmlParseInternalSubset: error detected in Markup declaration\n"); - break; + if (ctxt->inputNr > 1) + xmlPopInput(ctxt); + else + break; } } if (RAW == ']') { @@ -9282,7 +9307,7 @@ xmlParseStartTag2(xmlParserCtxtPtr ctxt, const xmlChar **pref, xmlChar *attvalue; const xmlChar **atts = ctxt->atts; int maxatts = ctxt->maxatts; - int nratts, nbatts, nbdef; + int nratts, nbatts, nbdef, inputid; int i, j, nbNs, attval; unsigned long cur; int nsNr = ctxt->nsNr; @@ -9299,6 +9324,7 @@ xmlParseStartTag2(xmlParserCtxtPtr ctxt, const xmlChar **pref, */ SHRINK; cur = ctxt->input->cur - ctxt->input->base; + inputid = ctxt->input->id; nbatts = 0; nratts = 0; nbdef = 0; @@ -9517,6 +9543,13 @@ next_attr: GROW; } + if (ctxt->input->id != inputid) { + xmlFatalErr(ctxt, XML_ERR_INTERNAL_ERROR, + "Unexpected change of input\n"); + localname = NULL; + goto done; + } + /* Reconstruct attribute value pointers. */ for (i = 0, j = 0; j < nratts; i += 5, j++) { if (atts[i+2] != NULL) { @@ -9675,6 +9708,7 @@ next_attr: nsname, 0, NULL, nbatts / 5, nbdef, atts); } +done: /* * Free up attribute allocated strings if needed */ diff --git a/result/errors/754946.xml.err b/result/errors/754946.xml.err index c03e35bf..4c195265 100644 --- a/result/errors/754946.xml.err +++ b/result/errors/754946.xml.err @@ -5,15 +5,16 @@ Entity: line 1: parser error : internal error: xmlParseInternalSubset: error det Entity: line 1: A ^ -Entity: line 1: parser error : DOCTYPE improperly terminated - %defroot; +Entity: line 1: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration + + %defmiddle; + ^ +Entity: line 1: +<!ELEMENT middle (test) > +^ +Entity: line 1: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration + + %deftest; ^ Entity: line 1: -<!ELEMENT root (middle) > -^ -Entity: line 1: parser error : Start tag expected, '<' not found - %defroot; - ^ -Entity: line 1: -<!ELEMENT root (middle) > +<!ELEMENT test (#PCDATA) > ^ diff --git a/result/valid/t8.xml.err.rdr b/result/valid/t8.xml.err.rdr index c198a163..3b2cd262 100644 --- a/result/valid/t8.xml.err.rdr +++ b/result/valid/t8.xml.err.rdr @@ -5,10 +5,18 @@ Entity: line 1: parser error : internal error: xmlParseInternalSubset: error det Entity: line 1: <!ELEMENT root (middle) > ^ -Entity: line 1: parser error : DOCTYPE improperly terminated - %defroot; +Entity: line 1: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration + + %defmiddle; + ^ +Entity: line 1: +<!ELEMENT middle (test) > +^ +Entity: line 1: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration + + %deftest; ^ Entity: line 1: -<!ELEMENT root (middle) > +<!ELEMENT test (#PCDATA) > ^ ./test/valid/t8.xml : failed to parse diff --git a/result/valid/t8a.xml.err b/result/valid/t8a.xml.err index 1a3c006d..bfe53147 100644 --- a/result/valid/t8a.xml.err +++ b/result/valid/t8a.xml.err @@ -5,15 +5,17 @@ Entity: line 1: parser error : internal error: xmlParseInternalSubset: error det Entity: line 1: <!ELEMENT root (middle) > ^ -Entity: line 1: parser error : DOCTYPE improperly terminated - %defroot; +Entity: line 1: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration + + %defmiddle; + ^ +Entity: line 1: +<!ELEMENT middle (test) > +^ +Entity: line 1: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration + + %deftest; ^ Entity: line 1: -<!ELEMENT root (middle) > -^ -Entity: line 1: parser error : Start tag expected, '<' not found - %defroot; - ^ -Entity: line 1: -<!ELEMENT root (middle) > +<!ELEMENT test (#PCDATA) > ^ diff --git a/result/valid/t8a.xml.err.rdr b/result/valid/t8a.xml.err.rdr index b6bdcbe7..d1bd92e0 100644 --- a/result/valid/t8a.xml.err.rdr +++ b/result/valid/t8a.xml.err.rdr @@ -5,10 +5,18 @@ Entity: line 1: parser error : internal error: xmlParseInternalSubset: error det Entity: line 1: <!ELEMENT root (middle) > ^ -Entity: line 1: parser error : DOCTYPE improperly terminated - %defroot; +Entity: line 1: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration + + %defmiddle; + ^ +Entity: line 1: +<!ELEMENT middle (test) > +^ +Entity: line 1: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration + + %deftest; ^ Entity: line 1: -<!ELEMENT root (middle) > +<!ELEMENT test (#PCDATA) > ^ ./test/valid/t8a.xml : failed to parse diff --git a/test/errors/754946.xml b/test/errors/754946.xml index 6b5f9b06..edeab32d 100644 --- a/test/errors/754946.xml +++ b/test/errors/754946.xml @@ -1 +1,4 @@ -%SYSTEM; + %SYSTEM; +