[libvirt] [PATCH] maint: don't permit format strings without %

[Daniel P. Berrange]

On Mon, Jul 23, 2012 at 02:37:42PM -0600, Eric Blake wrote:
> Any time we have a string with no % passed through gettext, a
> translator can inject a % to cause a stack overread.  When there
> is nothing to format, it's easier to ask for a string that cannot
> be used as a formatter, by using a trivial "%s" format instead.
> 
> In the past, we have used --disable-nls to catch some of the
> offenders, but that doesn't get run very often, and many more
> uses have crept in.  Syntax check to the rescue!

Also with current GCC, even using '--disable-nls' doesn't produce
any errors in this regard, hence why we have so many problems.

> 
> The syntax check can catch uses such as
> virReportError(code,
>                _("split "
>                  "string"));
> by using a sed script to fold context lines into one pattern
> space before checking for a string without %.
> 
> This patch is just mechanical insertion of %s; there are probably
> several messages touched by this patch where we would be better
> off giving the user more information than a fixed string.
> 
> * cfg.mk (sc_prohibit_diagnostic_without_format): New rule.
> * src/datatypes.c (virUnrefConnect, virGetDomain)
> (virUnrefDomain, virGetNetwork, virUnrefNetwork, virGetInterface)
> (virUnrefInterface, virGetStoragePool, virUnrefStoragePool)
> (virGetStorageVol, virUnrefStorageVol, virGetNodeDevice)
> (virGetSecret, virUnrefSecret, virGetNWFilter, virUnrefNWFilter)
> (virGetDomainSnapshot, virUnrefDomainSnapshot): Add %s wrapper.
> * src/lxc/lxc_driver.c (lxcDomainSetBlkioParameters)
> (lxcDomainGetBlkioParameters): Likewise.
> * src/conf/domain_conf.c (virSecurityDeviceLabelDefParseXML)
> (virDomainDiskDefParseXML, virDomainGraphicsDefParseXML):
> Likewise.
> * src/conf/network_conf.c (virNetworkDNSHostsDefParseXML)
> (virNetworkDefParseXML): Likewise.
> * src/conf/nwfilter_conf.c (virNWFilterIsValidChainName):
> Likewise.
> * src/conf/nwfilter_params.c (virNWFilterVarValueCreateSimple)
> (virNWFilterVarAccessParse): Likewise.
> * src/libvirt.c (virDomainSave, virDomainSaveFlags)
> (virDomainRestore, virDomainRestoreFlags)
> (virDomainSaveImageGetXMLDesc, virDomainSaveImageDefineXML)
> (virDomainCoreDump, virDomainGetXMLDesc)
> (virDomainMigrateVersion1, virDomainMigrateVersion2)
> (virDomainMigrateVersion3, virDomainMigrate, virDomainMigrate2)
> (virStreamSendAll, virStreamRecvAll)
> (virDomainSnapshotGetXMLDesc): Likewise.
> * src/nwfilter/nwfilter_dhcpsnoop.c (virNWFilterSnoopReqLeaseDel)
> (virNWFilterDHCPSnoopReq): Likewise.
> * src/openvz/openvz_driver.c (openvzUpdateDevice): Likewise.
> * src/openvz/openvz_util.c (openvzKBPerPages): Likewise.
> * src/qemu/qemu_cgroup.c (qemuSetupCgroup): Likewise.
> * src/qemu/qemu_command.c (qemuBuildHubDevStr, qemuBuildChrChardevStr)
> (qemuBuildCommandLine): Likewise.
> * src/qemu/qemu_driver.c (qemuDomainGetPercpuStats): Likewise.
> * src/qemu/qemu_hotplug.c (qemuDomainAttachNetDevice): Likewise.
> * src/rpc/virnetsaslcontext.c (virNetSASLSessionGetIdentity):
> Likewise.
> * src/rpc/virnetsocket.c (virNetSocketNewConnectUNIX)
> (virNetSocketSendFD, virNetSocketRecvFD): Likewise.
> * src/storage/storage_backend_disk.c
> (virStorageBackendDiskBuildPool): Likewise.
> * src/storage/storage_backend_fs.c
> (virStorageBackendFileSystemProbe)
> (virStorageBackendFileSystemBuild): Likewise.
> * src/storage/storage_backend_rbd.c
> (virStorageBackendRBDOpenRADOSConn): Likewise.
> * src/storage/storage_driver.c (storageVolumeResize): Likewise.
> * src/test/test_driver.c (testInterfaceChangeBegin)
> (testInterfaceChangeCommit, testInterfaceChangeRollback):
> Likewise.
> * src/vbox/vbox_tmpl.c (vboxListAllDomains): Likewise.
> * src/xenxs/xen_sxpr.c (xenFormatSxprDisk, xenFormatSxpr):
> Likewise.
> * src/xenxs/xen_xm.c (xenXMConfigGetUUID, xenFormatXMDisk)
> (xenFormatXM): Likewise.
> ---
> 
> danpb asked me on IRC if it was possible to automate the checking
> for our recent %s additions.  Of course, my answer was yes, before
> I knew it would take me three hours to get here...
> 
>  cfg.mk                             |   15 ++++++++++++-
>  src/conf/domain_conf.c             |   22 +++++++++----------
>  src/conf/network_conf.c            |    8 +++----
>  src/conf/nwfilter_conf.c           |    4 ++--
>  src/conf/nwfilter_params.c         |    8 +++----
>  src/datatypes.c                    |   41 +++++++++++++++++++-----------------
>  src/libvirt.c                      |   34 +++++++++++++++---------------
>  src/lxc/lxc_driver.c               |    6 ++++--
>  src/nwfilter/nwfilter_dhcpsnoop.c  |    5 +++--
>  src/openvz/openvz_driver.c         |    2 +-
>  src/openvz/openvz_util.c           |    2 +-
>  src/qemu/qemu_cgroup.c             |    4 ++--
>  src/qemu/qemu_command.c            |    8 +++----
>  src/qemu/qemu_driver.c             |    2 +-
>  src/qemu/qemu_hotplug.c            |    2 +-
>  src/rpc/virnetsaslcontext.c        |    4 ++--
>  src/rpc/virnetsocket.c             |    6 +++---
>  src/storage/storage_backend_disk.c |    8 +++----
>  src/storage/storage_backend_fs.c   |    8 +++----
>  src/storage/storage_backend_rbd.c  |    8 +++----
>  src/storage/storage_driver.c       |   10 ++++-----
>  src/test/test_driver.c             |    6 +++---
>  src/vbox/vbox_tmpl.c               |    2 +-
>  src/xenxs/xen_sxpr.c               |    6 +++---
>  src/xenxs/xen_xm.c                 |    8 +++----
>  25 files changed, 124 insertions(+), 105 deletions(-)

ACK


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 :|