Re: [PATCH v2] XML: escape strings where we should do it

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]