[libxml2] Rework entity boundary checks
- From: Nick Wellnhofer <nwellnhof src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [libxml2] Rework entity boundary checks
- Date: Sat, 17 Jun 2017 15:32:41 +0000 (UTC)
commit 5f440d8cadea9f0d87fd3849366445029d47f528
Author: Nick Wellnhofer <wellnhofer aevum de>
Date: Mon Jun 12 14:32:34 2017 +0200
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 | 18 ++----
result/valid/t8.xml.err | 16 +++--
result/valid/t8.xml.err.rdr | 14 ++++-
result/valid/t8a.xml.err | 16 +++--
result/valid/t8a.xml.err.rdr | 14 ++++-
test/errors/754946.xml | 5 +-
9 files changed, 148 insertions(+), 94 deletions(-)
---
diff --git a/parser.c b/parser.c
index 862f4fb..30a41dd 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 c03e35b..4c19526 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<lbbbbbbbbbbbbbbbbbbb_
^
-Entity: line 1: parser error : DOCTYPE improperly terminated
- %SYSTEM;
- ^
-Entity: line 1:
-A<lbbbbbbbbbbbbbbbbbbb_
-^
-Entity: line 1: parser error : Start tag expected, '<' not found
- %SYSTEM;
- ^
-Entity: line 1:
-A<lbbbbbbbbbbbbbbbbbbb_
-^
+./test/errors/754946.xml:4: parser error : internal error: xmlParseInternalSubset: error detected in Markup
declaration
+
+ <![
+ ^
+./test/errors/754946.xml:4: parser error : DOCTYPE improperly terminated
+ <![
+ ^
+./test/errors/754946.xml:4: parser error : StartTag: invalid element name
+ <![
+ ^
+./test/errors/754946.xml:4: parser error : Extra content at the end of the document
+ <![
+ ^
diff --git a/result/errors/754946.xml.str b/result/errors/754946.xml.str
index 3b748cc..49395b6 100644
--- a/result/errors/754946.xml.str
+++ b/result/errors/754946.xml.str
@@ -1,4 +1,4 @@
./test/errors/754946.xml:1: parser error : Extra content at the end of the document
-<!DOCTYPEA[<!ENTITY %
- ^
+<!DOCTYPE A [
+ ^
./test/errors/754946.xml : failed to parse
diff --git a/result/errors10/781205.xml.err b/result/errors10/781205.xml.err
index da15c3f..f6395d6 100644
--- a/result/errors10/781205.xml.err
+++ b/result/errors10/781205.xml.err
@@ -5,17 +5,13 @@ Entity: line 1: parser error : internal error: xmlParseInternalSubset: error det
Entity: line 1:
<:0000
^
-Entity: line 1: parser error : DOCTYPE improperly terminated
- %a;
- ^
-Entity: line 1:
-<:0000
+./test/errors10/781205.xml:4: parser error : internal error: xmlParseInternalSubset: error detected in
Markup declaration
+
+
^
-namespace error : Failed to parse QName ':0000'
- %a;
- ^
-<:0000
- ^
-./test/errors10/781205.xml:4: parser error : Couldn't find end of Start Tag :0000 line 1
+./test/errors10/781205.xml:4: parser error : DOCTYPE improperly terminated
+
+^
+./test/errors10/781205.xml:4: parser error : Start tag expected, '<' not found
^
diff --git a/result/valid/t8.xml.err b/result/valid/t8.xml.err
index 1a3c006..bfe5314 100644
--- a/result/valid/t8.xml.err
+++ b/result/valid/t8.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 root (middle) >
+<!ELEMENT middle (test) >
^
-Entity: line 1: parser error : Start tag expected, '<' not found
- %defroot;
+Entity: line 1: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration
+
+ %deftest;
^
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 c198a16..3b2cd26 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 1a3c006..bfe5314 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 root (middle) >
+<!ELEMENT middle (test) >
^
-Entity: line 1: parser error : Start tag expected, '<' not found
- %defroot;
+Entity: line 1: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration
+
+ %deftest;
^
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 b6bdcbe..d1bd92e 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 6b5f9b0..edeab32 100644
--- a/test/errors/754946.xml
+++ b/test/errors/754946.xml
@@ -1 +1,4 @@
-<!DOCTYPEA[<!ENTITY %SYSTEM "A<lbbbbbbbbbbbbbbbbbbb_">%SYSTEM;<![
\ No newline at end of file
+<!DOCTYPE A [
+ <!ENTITY % SYSTEM "A<lbbbbbbbbbbbbbbbbbbb_">
+ %SYSTEM;
+ <![
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
