Re: [xml] [PATCH] fix memory leak when xmlRegStatePush failed

[Nick Wellnhofer via xml]

On 12/01/2021 10:42, zhuyan (M) wrote:


In the function xmlRegStatePush, if xmlMalloc or xmlRealloc fails,


Yes, there are many issues that arise from poor handling of malloc failures. 
Fortunately, similar issues can be found quite effectively by changing the 
fuzzers to inject malloc failures. I already started to address these errors 
in a more systematic way, but I want to hold off further commits until after 
the next release.


Note that in this particular case, it is easier to make static function 
xmlRegStatePush free the 'to' state on error.


Nick
___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml

[xml] [PATCH] fix memory leak when xmlRegStatePush failed

[zhuyan (M)]

In the function xmlRegStatePush, if xmlMalloc or xmlRealloc fails, the 
parameter to will not be added to ctxt->states and will not be managed. If 
xmlRegStatePush does not determine the return value, the input parameter to 
will not be released.

Signed-off-by: Qing Wu 
Signed-off-by: Yan Zhu 
---
 xmlregexp.c | 75 -
 1 file changed, 60 insertions(+), 15 deletions(-)

diff --git a/xmlregexp.c b/xmlregexp.c
index 40dabb20..c6c93316 100644
--- a/xmlregexp.c
+++ b/xmlregexp.c
@@ -1498,7 +1498,10 @@ xmlFAGenerateAllTransition(xmlRegParserCtxtPtr ctxt,
   int lax) {
 if (to == NULL) {
to = xmlRegNewState(ctxt);
-   xmlRegStatePush(ctxt, to);
+   if(xmlRegStatePush(ctxt, to) < 0){
+   xmlRegFreeState(to);
+   return;
+   }
ctxt->state = to;
 }
 if (lax)
@@ -1519,7 +1522,10 @@ xmlFAGenerateEpsilonTransition(xmlRegParserCtxtPtr ctxt,
   xmlRegStatePtr from, xmlRegStatePtr to) {
 if (to == NULL) {
to = xmlRegNewState(ctxt);
-   xmlRegStatePush(ctxt, to);
+   if(xmlRegStatePush(ctxt, to) < 0){
+   xmlRegFreeState(to);
+   return;
+   }
ctxt->state = to;
 }
 xmlRegStateAddTrans(ctxt, from, NULL, to, -1, -1); @@ -1538,7 +1544,10 @@ 
xmlFAGenerateCountedEpsilonTransition(xmlRegParserCtxtPtr ctxt,
xmlRegStatePtr from, xmlRegStatePtr to, int counter) {
 if (to == NULL) {
to = xmlRegNewState(ctxt);
-   xmlRegStatePush(ctxt, to);
+   if(xmlRegStatePush(ctxt, to) < 0){
+   xmlRegFreeState(to);
+   return;
+   }
ctxt->state = to;
 }
 xmlRegStateAddTrans(ctxt, from, NULL, to, counter, -1); @@ -1557,7 
+1566,10 @@ xmlFAGenerateCountedTransition(xmlRegParserCtxtPtr ctxt,
xmlRegStatePtr from, xmlRegStatePtr to, int counter) {
 if (to == NULL) {
to = xmlRegNewState(ctxt);
-   xmlRegStatePush(ctxt, to);
+   if(xmlRegStatePush(ctxt, to) < 0){
+   xmlRegFreeState(to);
+   return;
+   }
ctxt->state = to;
 }
 xmlRegStateAddTrans(ctxt, from, NULL, to, -1, counter); @@ -1600,7 
+1612,10 @@ xmlFAGenerateTransitions(xmlRegParserCtxtPtr ctxt, xmlRegStatePtr 
from,
} else if ((to == NULL) && (atom->quant != XML_REGEXP_QUANT_RANGE) &&
   (atom->quant != XML_REGEXP_QUANT_ONCE)) {
to = xmlRegNewState(ctxt);
-   xmlRegStatePush(ctxt, to);
+   if(xmlRegStatePush(ctxt, to) < 0){
+   xmlRegFreeState(to);
+   return(-1);
+   }
ctxt->state = to;
xmlFAGenerateEpsilonTransition(ctxt, atom->stop, to);  #endif @@ 
-1641,7 +1656,10 @@ xmlFAGenerateTransitions(xmlRegParserCtxtPtr ctxt, 
xmlRegStatePtr from,
newstate = to;
} else {
newstate = xmlRegNewState(ctxt);
-   xmlRegStatePush(ctxt, newstate);
+   if(xmlRegStatePush(ctxt, newstate) < 0){
+   xmlRegFreeState(newstate);
+   return(-1);
+   }
}
 
/*
@@ -1723,7 +1741,10 @@ xmlFAGenerateTransitions(xmlRegParserCtxtPtr ctxt, 
xmlRegStatePtr from,
if (to == NULL) {
to = xmlRegNewState(ctxt);
if (to != NULL)
-   xmlRegStatePush(ctxt, to);
+   if(xmlRegStatePush(ctxt, to) < 0) {
+   xmlRegFreeState(to);
+   return(-1);
+   }
else {
return(-1);
}
@@ -1736,7 +1757,10 @@ xmlFAGenerateTransitions(xmlRegParserCtxtPtr ctxt, 
xmlRegStatePtr from,
 if (to == NULL) {
to = xmlRegNewState(ctxt);
if (to != NULL)
-   xmlRegStatePush(ctxt, to);
+   if(xmlRegStatePush(ctxt, to) < 0) {
+   xmlRegFreeState(to);
+   return(-1);
+   }
else {
return(-1);
}
@@ -1753,7 +1777,10 @@ xmlFAGenerateTransitions(xmlRegParserCtxtPtr ctxt, 
xmlRegStatePtr from,
 
tmp = xmlRegNewState(ctxt);
if (tmp != NULL)
-   xmlRegStatePush(ctxt, tmp);
+   if(xmlRegStatePush(ctxt, tmp) < 0) {
+   xmlRegFreeState(tmp);
+   return(-1);
+   }
else {
return(-1);
}
@@ -5556,7 +5583,10 @@ xmlRegexpCompile(const xmlChar *regexp) {
 /* initialize the parser */
 ctxt->end = NULL;
 ctxt->start = ctxt->state = xmlRegNewState(ctxt);
-xmlRegStatePush(ctxt, ctxt->start);
+if(xmlRegStatePush(ctxt, ctxt->start) < 0){
+   xmlRegFreeState(ctxt->start);
+   return(NULL);
+}
 
 /* parse the expression building an automata */
 xmlFAParseRegExp(ctxt, 1);
@@ -6014,7 +6044,10 @@ xmlAutomataNewCountTrans2(xmlAutomataPtr am,