On Tue, May 12, 2015 at 10:07:21AM +0200, Ján Tomko wrote: > On Mon, May 11, 2015 at 06:17:26PM +0200, Pavel Hrdina wrote: > > There is a lot of places, were it's pretty ease for user to enter some > > characters that we need to escape to create a valid XML description. > > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1197580 > > > > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx> > > --- > > > > Previous version: > > https://www.redhat.com/archives/libvir-list/2015-May/msg00110.html > > > > I've went through all the usage of virBufferAsprintf and hopefully I didn't miss > > any wrong usage. > > > > src/conf/capabilities.c | 6 ++-- > > src/conf/cpu_conf.c | 6 ++-- > > src/conf/domain_capabilities.c | 2 +- > > src/conf/domain_conf.c | 63 +++++++++++++++++++++--------------------- > > src/conf/network_conf.c | 17 ++++++------ > > src/conf/node_device_conf.c | 4 +-- > > 6 files changed, 49 insertions(+), 49 deletions(-) > > > > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c > > index c43bfb3..6bd06e5 100644 > > --- a/src/conf/capabilities.c > > +++ b/src/conf/capabilities.c > > @@ -682,9 +682,9 @@ virCapabilitiesDomainDataLookupInternal(virCapsPtr caps, > > virBufferAsprintf(&buf, "domaintype=%s ", > > virDomainVirtTypeToString(domaintype)); > > if (emulator) > > - virBufferAsprintf(&buf, "emulator=%s ", emulator); > > + virBufferEscapeString(&buf, "emulator=%s ", emulator); > > virBufferEscapeString is a no-op if the string is NULL, the if is not > necessary. > > > if (machinetype) > > - virBufferAsprintf(&buf, "machine=%s ", machinetype); > > + virBufferEscapeString(&buf, "machine=%s ", machinetype); > > if (virBufferCurrentContent(&buf) && > > !virBufferCurrentContent(&buf)[0]) > > virBufferAsprintf(&buf, "%s", _("any configuration")); > > @@ -903,7 +903,7 @@ virCapabilitiesFormatXML(virCapsPtr caps) > > virBufferAdjustIndent(&buf, 2); > > for (i = 0; i < caps->host.nmigrateTrans; i++) { > > virBufferAsprintf(&buf, "<uri_transport>%s</uri_transport>\n", > > - caps->host.migrateTrans[i]); > > + caps->host.migrateTrans[i]); > > Unrelated whitespace change. > > > } > > virBufferAdjustIndent(&buf, -2); > > virBufferAddLit(&buf, "</uri_transports>\n"); > > diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c > > index e959ecc..1ba1d82 100644 > > --- a/src/conf/cpu_conf.c > > +++ b/src/conf/cpu_conf.c > > @@ -544,17 +544,17 @@ virCPUDefFormatBuf(virBufferPtr buf, > > } > > virBufferAsprintf(buf, " fallback='%s'", fallback); > > if (def->vendor_id) > > - virBufferAsprintf(buf, " vendor_id='%s'", def->vendor_id); > > + virBufferEscapeString(buf, " vendor_id='%s'", def->vendor_id); > > } > > if (formatModel && def->model) { > > - virBufferAsprintf(buf, ">%s</model>\n", def->model); > > + virBufferEscapeString(buf, ">%s</model>\n", def->model); > > We need to keep check for non-NULL string in the if here. > > > } else { > > virBufferAddLit(buf, "/>\n"); > > } > > } > > > > if (formatModel && def->vendor) > > - virBufferAsprintf(buf, "<vendor>%s</vendor>\n", def->vendor); > > + virBufferEscapeString(buf, "<vendor>%s</vendor>\n", def->vendor); > > > > But not here. > > > if (def->sockets && def->cores && def->threads) { > > virBufferAddLit(buf, "<topology"); > > diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c > > index 7c59912..0e32f52 100644 > > --- a/src/conf/domain_capabilities.c > > +++ b/src/conf/domain_capabilities.c > > @@ -272,7 +272,7 @@ virDomainCapsFormatInternal(virBufferPtr buf, > > virBufferAddLit(buf, "<domainCapabilities>\n"); > > virBufferAdjustIndent(buf, 2); > > > > - virBufferAsprintf(buf, "<path>%s</path>\n", caps->path); > > + virBufferEscapeString(buf, "<path>%s</path>\n", caps->path); > > virBufferAsprintf(buf, "<domain>%s</domain>\n", virttype_str); > > virBufferAsprintf(buf, "<machine>%s</machine>\n", caps->machine); > > virBufferAsprintf(buf, "<arch>%s</arch>\n", arch_str); > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > > index e3e0f63..4c3f46f 100644 > > --- a/src/conf/domain_conf.c > > +++ b/src/conf/domain_conf.c > > @@ -3724,7 +3724,7 @@ virDomainDeviceInfoFormat(virBufferPtr buf, > > virBufferAsprintf(buf, " bar='%s'", rombar); > > } > > if (info->romfile) > > - virBufferAsprintf(buf, " file='%s'", info->romfile); > > + virBufferEscapeString(buf, " file='%s'", info->romfile); > > virBufferAddLit(buf, "/>\n"); > > } > > > > @@ -17712,7 +17712,7 @@ virSecurityDeviceLabelDefFormat(virBufferPtr buf, > > virBufferAddLit(buf, "<seclabel"); > > > > if (def->model) > > - virBufferAsprintf(buf, " model='%s'", def->model); > > + virBufferEscapeString(buf, " model='%s'", def->model); > > > > if (def->labelskip) > > virBufferAddLit(buf, " labelskip='yes'"); > > @@ -19246,50 +19246,51 @@ virDomainChrSourceDefFormat(virBufferPtr buf, > > break; > > > > case VIR_DOMAIN_CHR_TYPE_NMDM: > > - virBufferAsprintf(buf, "<source master='%s' slave='%s'/>\n", > > - def->data.nmdm.master, > > - def->data.nmdm.slave); > > + virBufferEscapeString(buf, "<source master='%s' ", > > + def->data.nmdm.master); > > + virBufferEscapeString(buf, "slave='%s'/>\n", def->data.nmdm.slave); > > break; > > > > case VIR_DOMAIN_CHR_TYPE_UDP: > > if (def->data.udp.bindService && > > def->data.udp.bindHost) { > > - virBufferAsprintf(buf, > > - "<source mode='bind' host='%s' " > > - "service='%s'/>\n", > > - def->data.udp.bindHost, > > - def->data.udp.bindService); > > + virBufferEscapeString(buf, > > + "<source mode='bind' host='%s' ", > > + def->data.udp.bindHost); > > + virBufferEscapeString(buf, > > + "service='%s'/>\n", > > + def->data.udp.bindService); > > } else if (def->data.udp.bindHost) { > > - virBufferAsprintf(buf, "<source mode='bind' host='%s'/>\n", > > - def->data.udp.bindHost); > > + virBufferEscapeString(buf, "<source mode='bind' host='%s'/>\n", > > + def->data.udp.bindHost); > > } else if (def->data.udp.bindService) { > > - virBufferAsprintf(buf, "<source mode='bind' service='%s'/>\n", > > - def->data.udp.bindService); > > + virBufferEscapeString(buf, "<source mode='bind' service='%s'/>\n", > > + def->data.udp.bindService); > > } > > > > if (def->data.udp.connectService && > > def->data.udp.connectHost) { > > - virBufferAsprintf(buf, > > - "<source mode='connect' host='%s' " > > - "service='%s'/>\n", > > - def->data.udp.connectHost, > > - def->data.udp.connectService); > > + virBufferEscapeString(buf, > > + "<source mode='connect' host='%s' ", > > + def->data.udp.connectHost); > > + virBufferEscapeString(buf, > > + "service='%s'/>\n", > > + def->data.udp.connectService); > > } else if (def->data.udp.connectHost) { > > - virBufferAsprintf(buf, "<source mode='connect' host='%s'/>\n", > > - def->data.udp.connectHost); > > + virBufferEscapeString(buf, "<source mode='connect' host='%s'/>\n", > > + def->data.udp.connectHost); > > } else if (def->data.udp.connectService) { > > - virBufferAsprintf(buf, > > - "<source mode='connect' service='%s'/>\n", > > - def->data.udp.connectService); > > + virBufferEscapeString(buf, > > + "<source mode='connect' service='%s'/>\n", > > + def->data.udp.connectService); > > } > > break; > > > > What a strange way to format the attributes. > > > case VIR_DOMAIN_CHR_TYPE_TCP: > > - virBufferAsprintf(buf, > > - "<source mode='%s' host='%s' service='%s'/>\n", > > - def->data.tcp.listen ? "bind" : "connect", > > - def->data.tcp.host, > > - def->data.tcp.service); > > + virBufferAsprintf(buf, "<source mode='%s' ", > > + def->data.tcp.listen ? "bind" : "connect"); > > + virBufferEscapeString(buf, "host='%s' ", def->data.tcp.host); > > + virBufferEscapeString(buf, "service='%s'/>\n", def->data.tcp.service); > > virBufferAsprintf(buf, "<protocol type='%s'/>\n", > > virDomainChrTcpProtocolTypeToString( > > def->data.tcp.protocol)); > > @@ -19303,8 +19304,8 @@ virDomainChrSourceDefFormat(virBufferPtr buf, > > break; > > > > case VIR_DOMAIN_CHR_TYPE_SPICEPORT: > > - virBufferAsprintf(buf, "<source channel='%s'/>\n", > > - def->data.spiceport.channel); > > + virBufferEscapeString(buf, "<source channel='%s'/>\n", > > + def->data.spiceport.channel); > > break; > > > > } > > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c > > index 5b734f2..aa607a8 100644 > > --- a/src/conf/network_conf.c > > +++ b/src/conf/network_conf.c > > @@ -2422,21 +2422,20 @@ virNetworkDNSDefFormat(virBufferPtr buf, > > } > > > > for (i = 0; i < def->ntxts; i++) { > > - virBufferAsprintf(buf, "<txt name='%s' value='%s'/>\n", > > - def->txts[i].name, > > - def->txts[i].value); > > + virBufferEscapeString(buf, "<txt name='%s' ", def->txts[i].name); > > + virBufferEscapeString(buf, "value='%s'/>\n", def->txts[i].value); > > } > > > > for (i = 0; i < def->nsrvs; i++) { > > if (def->srvs[i].service && def->srvs[i].protocol) { > > - virBufferAsprintf(buf, "<srv service='%s' protocol='%s'", > > - def->srvs[i].service, > > - def->srvs[i].protocol); > > + virBufferEscapeString(buf, "<srv service='%s' ", > > + def->srvs[i].service); > > + virBufferEscapeString(buf, "protocol='%s'", def->srvs[i].protocol); > > > > if (def->srvs[i].domain) > > virBufferAsprintf(buf, " domain='%s'", def->srvs[i].domain); > > Can the domain contain anything we need to escape? > > > if (def->srvs[i].target) > > - virBufferAsprintf(buf, " target='%s'", def->srvs[i].target); > > + virBufferEscapeString(buf, " target='%s'", def->srvs[i].target); > > Redundant if. > > > if (def->srvs[i].port) > > virBufferAsprintf(buf, " port='%d'", def->srvs[i].port); > > if (def->srvs[i].priority) > > @@ -2455,8 +2454,8 @@ virNetworkDNSDefFormat(virBufferPtr buf, > > virBufferAsprintf(buf, "<host ip='%s'>\n", ip); > > virBufferAdjustIndent(buf, 2); > > for (j = 0; j < def->hosts[i].nnames; j++) > > - virBufferAsprintf(buf, "<hostname>%s</hostname>\n", > > - def->hosts[i].names[j]); > > + virBufferEscapeString(buf, "<hostname>%s</hostname>\n", > > + def->hosts[i].names[j]); > > > > virBufferAdjustIndent(buf, -2); > > virBufferAddLit(buf, "</host>\n"); > > diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > > index a286847..feae3d4 100644 > > --- a/src/conf/node_device_conf.c > > +++ b/src/conf/node_device_conf.c > > @@ -514,8 +514,8 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def) > > virBufferEscapeString(&buf, "<vendor>%s</vendor>\n", > > data->storage.vendor); > > if (data->storage.serial) > > - virBufferAsprintf(&buf, "<serial>%s</serial>\n", > > - data->storage.serial); > > + virBufferEscapeString(&buf, "<serial>%s</serial>\n", > > + data->storage.serial); > > if (data->storage.flags & VIR_NODE_DEV_CAP_STORAGE_REMOVABLE) { > > int avl = data->storage.flags & > > VIR_NODE_DEV_CAP_STORAGE_REMOVABLE_MEDIA_AVAILABLE; > > ACK with: > * removed checks for non-NULL string if it's the only condition > * def->vendor removed from if (formatModel && def->vendor) As we discussed, I'll keep the condition unmodified. > * def->srvs[i].domain escaped > * the whitespace change moved to a separate commit > > Jan Thanks for review, I'll push it shortly with the domain escaped and the whitespace fix in separate commit. Pavel -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list