[libvirt] [PATCH 0/4] Fix a pair of crashes in DNS XML parsing
*** BLUAAARRRGHB HERE *** Ján Tomko (4): Set the number of elements to 0 in virNetwork*Clear Don't check validity of missing attributes in DNS SRV XML Remove double space in error messages Remove redundant free in virNetworkDNSHostDefParseXML src/conf/network_conf.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) -- 1.8.1.5
Leaving it at -1 causes invalid free in virNetworkDefUpdateDNSHost if virNetworkDNSHostDefParseXML fails and virNetworkDNSHostDefClear gets called twice. --- src/conf/network_conf.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d616e12..e3e0e89 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -136,6 +136,7 @@ virNetworkIpDefClear(virNetworkIpDefPtr def) while (def->nhosts--) virNetworkDHCPHostDefClear(&def->hosts[def->nhosts]); + def->nhosts = 0; VIR_FREE(def->hosts); VIR_FREE(def->tftproot); @@ -161,6 +162,7 @@ virNetworkDNSHostDefClear(virNetworkDNSHostDefPtr def) while (def->nnames--) VIR_FREE(def->names[def->nnames]); VIR_FREE(def->names); + def->nnames = 0; } static void @@ -190,6 +192,7 @@ virNetworkDNSDefClear(virNetworkDNSDefPtr def) virNetworkDNSSrvDefClear(&def->srvs[def->nsrvs]); VIR_FREE(def->srvs); } + def->ntxts = def->nhosts = def->nsrvs = 0; } static void @@ -206,6 +209,7 @@ virNetworkForwardDefClear(virNetworkForwardDefPtr def) virNetworkForwardIfDefClear(&def->ifs[i]); } VIR_FREE(def->ifs); + def->nifs = def->npfs = 0; } void -- 1.8.1.5
On 26.07.2013 12:28, Ján Tomko wrote:
Leaving it at -1 causes invalid free in virNetworkDefUpdateDNSHost if virNetworkDNSHostDefParseXML fails and virNetworkDNSHostDefClear gets called twice. --- src/conf/network_conf.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d616e12..e3e0e89 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -136,6 +136,7 @@ virNetworkIpDefClear(virNetworkIpDefPtr def)
while (def->nhosts--) virNetworkDHCPHostDefClear(&def->hosts[def->nhosts]); + def->nhosts = 0;
I think we need a sligthly different approach here. What if def->nhost is 0 before entering the while() loop? It gets decreased to -1 and ... We need: while (def->nhosts) virNetworkDHCPHostDefCelar( .... [--def->nhosts]); or just use for (i = 0; i < def->nhosts; i++) theFunction( ...[i]); Michal
Decrementing it when it was already 0 causes an invalid free in virNetworkDefUpdateDNSHost if virNetworkDNSHostDefParseXML fails and virNetworkDNSHostDefClear gets called twice. virNetworkForwardDefClear left the number untouched even if it freed all the elements. --- src/conf/network_conf.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d616e12..490b04d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -134,8 +134,8 @@ virNetworkIpDefClear(virNetworkIpDefPtr def) VIR_FREE(def->family); VIR_FREE(def->ranges); - while (def->nhosts--) - virNetworkDHCPHostDefClear(&def->hosts[def->nhosts]); + while (def->nhosts) + virNetworkDHCPHostDefClear(&def->hosts[--def->nhosts]); VIR_FREE(def->hosts); VIR_FREE(def->tftproot); @@ -158,8 +158,8 @@ virNetworkDNSTxtDefClear(virNetworkDNSTxtDefPtr def) static void virNetworkDNSHostDefClear(virNetworkDNSHostDefPtr def) { - while (def->nnames--) - VIR_FREE(def->names[def->nnames]); + while (def->nnames) + VIR_FREE(def->names[--def->nnames]); VIR_FREE(def->names); } @@ -176,18 +176,18 @@ static void virNetworkDNSDefClear(virNetworkDNSDefPtr def) { if (def->txts) { - while (def->ntxts--) - virNetworkDNSTxtDefClear(&def->txts[def->ntxts]); + while (def->ntxts) + virNetworkDNSTxtDefClear(&def->txts[--def->ntxts]); VIR_FREE(def->txts); } if (def->hosts) { - while (def->nhosts--) - virNetworkDNSHostDefClear(&def->hosts[def->nhosts]); + while (def->nhosts) + virNetworkDNSHostDefClear(&def->hosts[--def->nhosts]); VIR_FREE(def->hosts); } if (def->srvs) { - while (def->nsrvs--) - virNetworkDNSSrvDefClear(&def->srvs[def->nsrvs]); + while (def->nsrvs) + virNetworkDNSSrvDefClear(&def->srvs[--def->nsrvs]); VIR_FREE(def->srvs); } } @@ -206,6 +206,7 @@ virNetworkForwardDefClear(virNetworkForwardDefPtr def) virNetworkForwardIfDefClear(&def->ifs[i]); } VIR_FREE(def->ifs); + def->nifs = def->npfs = 0; } void -- 1.8.1.5
On 26.07.2013 12:54, Ján Tomko wrote:
Decrementing it when it was already 0 causes an invalid free in virNetworkDefUpdateDNSHost if virNetworkDNSHostDefParseXML fails and virNetworkDNSHostDefClear gets called twice.
virNetworkForwardDefClear left the number untouched even if it freed all the elements. --- src/conf/network_conf.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d616e12..490b04d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -134,8 +134,8 @@ virNetworkIpDefClear(virNetworkIpDefPtr def) VIR_FREE(def->family); VIR_FREE(def->ranges);
- while (def->nhosts--) - virNetworkDHCPHostDefClear(&def->hosts[def->nhosts]); + while (def->nhosts) + virNetworkDHCPHostDefClear(&def->hosts[--def->nhosts]);
VIR_FREE(def->hosts); VIR_FREE(def->tftproot); @@ -158,8 +158,8 @@ virNetworkDNSTxtDefClear(virNetworkDNSTxtDefPtr def) static void virNetworkDNSHostDefClear(virNetworkDNSHostDefPtr def) { - while (def->nnames--) - VIR_FREE(def->names[def->nnames]); + while (def->nnames) + VIR_FREE(def->names[--def->nnames]); VIR_FREE(def->names); }
@@ -176,18 +176,18 @@ static void virNetworkDNSDefClear(virNetworkDNSDefPtr def) { if (def->txts) { - while (def->ntxts--) - virNetworkDNSTxtDefClear(&def->txts[def->ntxts]); + while (def->ntxts) + virNetworkDNSTxtDefClear(&def->txts[--def->ntxts]); VIR_FREE(def->txts); } if (def->hosts) { - while (def->nhosts--) - virNetworkDNSHostDefClear(&def->hosts[def->nhosts]); + while (def->nhosts) + virNetworkDNSHostDefClear(&def->hosts[--def->nhosts]); VIR_FREE(def->hosts); } if (def->srvs) { - while (def->nsrvs--) - virNetworkDNSSrvDefClear(&def->srvs[def->nsrvs]); + while (def->nsrvs) + virNetworkDNSSrvDefClear(&def->srvs[--def->nsrvs]); VIR_FREE(def->srvs); } } @@ -206,6 +206,7 @@ virNetworkForwardDefClear(virNetworkForwardDefPtr def) virNetworkForwardIfDefClear(&def->ifs[i]); } VIR_FREE(def->ifs); + def->nifs = def->npfs = 0; }
void
ACK Michal
On 07/26/2013 01:00 PM, Michal Privoznik wrote:
On 26.07.2013 12:54, Ján Tomko wrote:
Decrementing it when it was already 0 causes an invalid free in virNetworkDefUpdateDNSHost if virNetworkDNSHostDefParseXML fails and virNetworkDNSHostDefClear gets called twice.
virNetworkForwardDefClear left the number untouched even if it freed all the elements. --- src/conf/network_conf.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
ACK
Michal
Thanks, I've pushed the series. Jan
This fixes a crash if one of them is missing. https://bugzilla.redhat.com/show_bug.cgi?id=988718 --- src/conf/network_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index e3e0e89..5faac92 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -923,7 +923,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName, " of network %s"), networkName); goto error; } - if (strlen(def->service) > DNS_RECORD_LENGTH_SRV) { + if (def->service && strlen(def->service) > DNS_RECORD_LENGTH_SRV) { virReportError(VIR_ERR_XML_DETAIL, _("Service name '%s' in network %s is too long, limit is %d bytes"), def->service, networkName, DNS_RECORD_LENGTH_SRV); @@ -939,7 +939,8 @@ virNetworkDNSSrvDefParseXML(const char *networkName, } /* Check whether protocol value is the supported one */ - if (STRNEQ(def->protocol, "tcp") && (STRNEQ(def->protocol, "udp"))) { + if (def->protocol && STRNEQ(def->protocol, "tcp") && + (STRNEQ(def->protocol, "udp"))) { virReportError(VIR_ERR_XML_DETAIL, _("Invalid protocol attribute value '%s' " " in DNS SRV record of network %s"), -- 1.8.1.5
On 26.07.2013 12:28, Ján Tomko wrote:
This fixes a crash if one of them is missing.
https://bugzilla.redhat.com/show_bug.cgi?id=988718 --- src/conf/network_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index e3e0e89..5faac92 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -923,7 +923,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName, " of network %s"), networkName); goto error; } - if (strlen(def->service) > DNS_RECORD_LENGTH_SRV) { + if (def->service && strlen(def->service) > DNS_RECORD_LENGTH_SRV) { virReportError(VIR_ERR_XML_DETAIL, _("Service name '%s' in network %s is too long, limit is %d bytes"), def->service, networkName, DNS_RECORD_LENGTH_SRV); @@ -939,7 +939,8 @@ virNetworkDNSSrvDefParseXML(const char *networkName, }
/* Check whether protocol value is the supported one */ - if (STRNEQ(def->protocol, "tcp") && (STRNEQ(def->protocol, "udp"))) { + if (def->protocol && STRNEQ(def->protocol, "tcp") && + (STRNEQ(def->protocol, "udp"))) { virReportError(VIR_ERR_XML_DETAIL, _("Invalid protocol attribute value '%s' " " in DNS SRV record of network %s"),
ACK Michal
--- src/conf/network_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 5faac92..2561c35 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -920,7 +920,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName, if (!(def->service = virXMLPropString(node, "service")) && !partialOkay) { virReportError(VIR_ERR_XML_DETAIL, _("Missing required service attribute in DNS SRV record " - " of network %s"), networkName); + "of network %s"), networkName); goto error; } if (def->service && strlen(def->service) > DNS_RECORD_LENGTH_SRV) { @@ -943,7 +943,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName, (STRNEQ(def->protocol, "udp"))) { virReportError(VIR_ERR_XML_DETAIL, _("Invalid protocol attribute value '%s' " - " in DNS SRV record of network %s"), + "in DNS SRV record of network %s"), def->protocol, networkName); goto error; } -- 1.8.1.5
On 26.07.2013 12:28, Ján Tomko wrote:
--- src/conf/network_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 5faac92..2561c35 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -920,7 +920,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName, if (!(def->service = virXMLPropString(node, "service")) && !partialOkay) { virReportError(VIR_ERR_XML_DETAIL, _("Missing required service attribute in DNS SRV record " - " of network %s"), networkName); + "of network %s"), networkName); goto error; } if (def->service && strlen(def->service) > DNS_RECORD_LENGTH_SRV) { @@ -943,7 +943,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName, (STRNEQ(def->protocol, "udp"))) { virReportError(VIR_ERR_XML_DETAIL, _("Invalid protocol attribute value '%s' " - " in DNS SRV record of network %s"), + "in DNS SRV record of network %s"), def->protocol, networkName); goto error; }
ACK Michal
ip has to be NULL at this point. --- src/conf/network_conf.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 2561c35..2040d26 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -859,7 +859,6 @@ virNetworkDNSHostDefParseXML(const char *networkName, virReportError(VIR_ERR_XML_DETAIL, _("Missing IP address in network '%s' DNS HOST record"), networkName); - VIR_FREE(ip); goto error; } -- 1.8.1.5
On 26.07.2013 12:28, Ján Tomko wrote:
ip has to be NULL at this point. --- src/conf/network_conf.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 2561c35..2040d26 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -859,7 +859,6 @@ virNetworkDNSHostDefParseXML(const char *networkName, virReportError(VIR_ERR_XML_DETAIL, _("Missing IP address in network '%s' DNS HOST record"), networkName); - VIR_FREE(ip); goto error; }
ACK Increasing context size would show the line just above these where we can clearly see without need to open the network_conf.c that @ip has to be NULL to take this path. Nevermind. Michal
On Fri, Jul 26, 2013 at 12:28:20PM +0200, Ján Tomko wrote:
*** BLUAAARRRGHB HERE ***
Ján Tomko (4): Set the number of elements to 0 in virNetwork*Clear Don't check validity of missing attributes in DNS SRV XML Remove double space in error messages Remove redundant free in virNetworkDNSHostDefParseXML
src/conf/network_conf.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
IMHO you should be including one or more test data files to demonstrate the crash & fix. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/26/2013 01:18 PM, Daniel P. Berrange wrote:
On Fri, Jul 26, 2013 at 12:28:20PM +0200, Ján Tomko wrote:
Ján Tomko (4): Set the number of elements to 0 in virNetwork*Clear Don't check validity of missing attributes in DNS SRV XML Remove double space in error messages Remove redundant free in virNetworkDNSHostDefParseXML
src/conf/network_conf.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
IMHO you should be including one or more test data files to demonstrate the crash & fix.
That can't be done with just data files, a new test for network XML sections is needed. virNetworkDefUpdateSection seems like the right function to test. Jan
participants (3)
-
Daniel P. Berrange -
Ján Tomko -
Michal Privoznik