maint: avoid remaining sprintf uses

* cfg.mk (sc_prohibit_sprintf): New rule.
(sc_prohibit_asprintf): Avoid false positives.
* docs/hacking.html.in (Printf-style functions): Document the
policy.
* HACKING: Regenerate.
* .x-sc_prohibit_sprintf: New exemptions.
* Makefile.am (syntax_check_exceptions): Ship new file.
* src/vbox/vbox_tmpl.c (vboxStartMachine, vboxAttachUSB): Use
virAsprintf instead.
* src/uml/uml_driver.c (umlOpenMonitor): Use snprintf instead.
* tools/virsh.c (cmdDetachInterface): Likewise.
* src/security/security_selinux.c (SELinuxGenSecurityLabel):
Likewise.
* src/openvz/openvz_driver.c (openvzDomainDefineCmd): Likewise,
and ensure large enough buffer.

diff --git a/.x-sc_prohibit_sprintf b/.x-sc_prohibit_sprintf
new file mode 100644 (file)
index 0000000..a67f51c
--- /dev/null
+++ b/.x-sc_prohibit_sprintf
@@ -0,0 +1,4 @@
+^docs/
+^po/
+^ChangeLog
+^HACKING

diff --git a/HACKING b/HACKING
index 2711ea1e082353e1a3a042452547af084d735e53..17ad34479a099d91a28ac299d6a0b079b227b8bd 100644 (file)
--- a/HACKING
+++ b/HACKING
@@ -538,6 +538,12 @@ virAsprintf, in util.h:
 This makes it so gcc's -Wformat and -Wformat-security options can do their
 jobs and cross-check format strings with the number and types of arguments.
 
+When printing to a string, consider using virBuffer for incremental
+allocations, virAsprintf for a one-shot allocation, and snprintf for
+fixed-width buffers. Do not use sprintf, even if you can prove the buffer
+won't overflow, since gnulib does not provide the same portability guarantees
+for sprintf as it does for snprintf.
+
 
 Use of goto
 ===========

diff --git a/Makefile.am b/Makefile.am
index 720dff961d5f96d1e6c1c948900b3c1d752d681c..e88814467da66af2db8f000d8f5f7eae21e41978 100644 (file)
--- a/Makefile.am
+++ b/Makefile.am
@@ -33,6 +33,7 @@ EXTRA_DIST = \
   .x-sc_prohibit_have_config_h \
   .x-sc_prohibit_HAVE_MBRTOWC \
   .x-sc_prohibit_nonreentrant \
+  .x-sc_prohibit_sprintf \
   .x-sc_prohibit_strcmp \
   .x-sc_prohibit_strcmp_and_strncmp \
   .x-sc_prohibit_strncpy \

diff --git a/cfg.mk b/cfg.mk
index 9d5fa76f1c2d59fe6f68785d6575098700c87db9..286b9022c3d1da37e6e9edb74cee73d800138131 100644 (file)
--- a/cfg.mk
+++ b/cfg.mk
@@ -238,10 +238,17 @@ sc_prohibit_strcmp_and_strncmp:
        halt='use STREQ() in place of the above uses of str[n]cmp'      \
          $(_sc_search_regexp)
 
-# Use virAsprintf rather than a'sprintf since *strp is undefined on error.
+# Use virAsprintf rather than as'printf since *strp is undefined on error.
 sc_prohibit_asprintf:
-       @prohibit='\<[a]sprintf\>'                                      \
-       halt='use virAsprintf, not a'sprintf                            \
+       @prohibit='\<a[s]printf\>'                                      \
+       halt='use virAsprintf, not as'printf                            \
+         $(_sc_search_regexp)
+
+# Use snprintf rather than s'printf, even if buffer is provably large enough,
+# since gnulib has more guarantees for snprintf portability
+sc_prohibit_sprintf:
+       @prohibit='\<[s]printf\>'                                       \
+       halt='use snprintf, not s'printf                                \
          $(_sc_search_regexp)
 
 sc_prohibit_strncpy:

diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index c42654e0b7e47611505dda72b88829b8e9d1bd7c..e1b51856c74a48572b42e3b87e5f27704d7d8650 100644 (file)
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -649,6 +649,15 @@
       of arguments.
     </p>
 
+    <p>
+      When printing to a string, consider using virBuffer for
+      incremental allocations, virAsprintf for a one-shot allocation,
+      and snprintf for fixed-width buffers.  Do not use sprintf, even
+      if you can prove the buffer won't overflow, since gnulib does
+      not provide the same portability guarantees for sprintf as it
+      does for snprintf.
+    </p>
+
     <h2><a name="goto">Use of goto</a></h2>
 
     <p>

diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index d9489083fd39c019e86eb4c01da6d42c0efc7775..796369534322b2344cce8e55062af1e67d19dc5e 100644 (file)
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -58,6 +58,7 @@
 #include "memory.h"
 #include "bridge.h"
 #include "files.h"
+#include "intprops.h"
 
 #define VIR_FROM_THIS VIR_FROM_OPENVZ
 
@@ -104,7 +105,7 @@ openvzDomainDefineCmd(const char *args[],
     int narg;
     int veid;
     int max_veid;
-    char str_id[10];
+    char str_id[INT_BUFSIZE_BOUND(max_veid)];
     FILE *fp;
 
     for (narg = 0; narg < maxarg; narg++)
@@ -162,7 +163,7 @@ openvzDomainDefineCmd(const char *args[],
         max_veid++;
     }
 
-    sprintf(str_id, "%d", max_veid);
+    snprintf(str_id, sizeof(str_id), "%d", max_veid);
     ADD_ARG_LIT(str_id);
 
     ADD_ARG_LIT("--name");

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 4aaf34803e140990442693bfb769eb0873a8ec42..936a1a6b9b41d484a18d41b1e9825e347744b648 100644 (file)
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -182,12 +182,12 @@ SELinuxGenSecurityLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED,
         c2 = virRandom(1024);
 
         if ( c1 == c2 ) {
-            sprintf(mcs, "s0:c%d", c1);
+            snprintf(mcs, sizeof(mcs), "s0:c%d", c1);
         } else {
             if ( c1 < c2 )
-                sprintf(mcs, "s0:c%d,c%d", c1, c2);
+                snprintf(mcs, sizeof(mcs), "s0:c%d,c%d", c1, c2);
             else
-                sprintf(mcs, "s0:c%d,c%d", c2, c1);
+                snprintf(mcs, sizeof(mcs), "s0:c%d,c%d", c2, c1);
         }
     } while(mcsAdd(mcs) == -1);
 

diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 7423956f241f3d181f7d44101afcbc3a04423694..ca4e7be695a2cf573bb9bed76bb633ce0807cee6 100644 (file)
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -657,7 +657,8 @@ restat:
     }
 
     memset(addr.sun_path, 0, sizeof addr.sun_path);
-    sprintf(addr.sun_path + 1, "libvirt-uml-%u", vm->pid);
+    snprintf(addr.sun_path + 1, sizeof(addr.sun_path) - 1,
+             "libvirt-uml-%u", vm->pid);
     VIR_DEBUG("Reply address for monitor is '%s'", addr.sun_path+1);
     if (bind(priv->monitor, (struct sockaddr *)&addr, sizeof addr) < 0) {
         virReportSystemError(errno,

diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 78f945cefca84059e86b3e1283295d7a738c0722..a210696a63a9139cd4b14a375d264eb3ca61f497 100644 (file)
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -3228,7 +3228,6 @@ vboxStartMachine(virDomainPtr dom, int i, IMachine *machine, vboxIID *iid)
     PRUnichar *valueDisplayUtf16 = NULL;
     char      *valueDisplayUtf8  = NULL;
     IProgress *progress          = NULL;
-    char displayutf8[32]         = {0};
     PRUnichar *env               = NULL;
     PRUnichar *sessionType       = NULL;
     nsresult rc;
@@ -3308,8 +3307,13 @@ vboxStartMachine(virDomainPtr dom, int i, IMachine *machine, vboxIID *iid)
 
     if (guiPresent) {
         if (guiDisplay) {
-            sprintf(displayutf8, "DISPLAY=%.24s", guiDisplay);
-            VBOX_UTF8_TO_UTF16(displayutf8, &env);
+            char *displayutf8;
+            if (virAsprintf(&displayutf8, "DISPLAY=%s", guiDisplay) < 0)
+                virReportOOMError();
+            else {
+                VBOX_UTF8_TO_UTF16(displayutf8, &env);
+                VIR_FREE(displayutf8);
+            }
             VIR_FREE(guiDisplay);
         }
 
@@ -3318,8 +3322,13 @@ vboxStartMachine(virDomainPtr dom, int i, IMachine *machine, vboxIID *iid)
 
     if (sdlPresent) {
         if (sdlDisplay) {
-            sprintf(displayutf8, "DISPLAY=%.24s", sdlDisplay);
-            VBOX_UTF8_TO_UTF16(displayutf8, &env);
+            char *displayutf8;
+            if (virAsprintf(&displayutf8, "DISPLAY=%s", sdlDisplay) < 0)
+                virReportOOMError();
+            else {
+                VBOX_UTF8_TO_UTF16(displayutf8, &env);
+                VIR_FREE(displayutf8);
+            }
             VIR_FREE(sdlDisplay);
         }
 
@@ -4526,19 +4535,22 @@ vboxAttachUSB(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine)
                     if (def->hostdevs[i]->source.subsys.type ==
                         VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
 
-                        char filtername[11]        = {0};
+                        char *filtername           = NULL;
                         PRUnichar *filternameUtf16 = NULL;
                         IUSBDeviceFilter *filter   = NULL;
 
-                        /* Assuming can't have more then 9999 devices so
-                         * restricting to %04d
+                        /* Zero pad for nice alignment when fewer than 9999
+                         * devices.
                          */
-                        sprintf(filtername, "filter%04d", i);
-                        VBOX_UTF8_TO_UTF16(filtername, &filternameUtf16);
-
-                        USBController->vtbl->CreateDeviceFilter(USBController,
-                                                                filternameUtf16,
-                                                                &filter);
+                        if (virAsprintf(&filtername, "filter%04d", i) < 0) {
+                            virReportOOMError();
+                        } else {
+                            VBOX_UTF8_TO_UTF16(filtername, &filternameUtf16);
+                            VIR_FREE(filtername);
+                            USBController->vtbl->CreateDeviceFilter(USBController,
+                                                                    filternameUtf16,
+                                                                    &filter);
+                        }
                         VBOX_UTF16_FREE(filternameUtf16);
 
                         if (filter &&
@@ -4551,13 +4563,15 @@ vboxAttachUSB(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine)
                             char productId[40]        = {0};
 
                             if (def->hostdevs[i]->source.subsys.u.usb.vendor) {
-                                sprintf(vendorId, "%x", def->hostdevs[i]->source.subsys.u.usb.vendor);
+                                snprintf(vendorId, sizeof(vendorId), "%x",
+                                         def->hostdevs[i]->source.subsys.u.usb.vendor);
                                 VBOX_UTF8_TO_UTF16(vendorId, &vendorIdUtf16);
                                 filter->vtbl->SetVendorId(filter, vendorIdUtf16);
                                 VBOX_UTF16_FREE(vendorIdUtf16);
                             }
                             if (def->hostdevs[i]->source.subsys.u.usb.product) {
-                                sprintf(productId, "%x", def->hostdevs[i]->source.subsys.u.usb.product);
+                                snprintf(productId, sizeof(productId), "%x",
+                                         def->hostdevs[i]->source.subsys.u.usb.product);
                                 VBOX_UTF8_TO_UTF16(productId, &productIdUtf16);
                                 filter->vtbl->SetProductId(filter,
                                                            productIdUtf16);

diff --git a/tools/virsh.c b/tools/virsh.c
index 4ef556a70f9ad0fa296e163992c3623f25ca568f..ae88cc022c05ba0abb5ab5d8da6f54d166393c8d 100644 (file)
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -8468,7 +8468,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
         goto cleanup;
     }
 
-    sprintf(buf, "/domain/devices/interface[@type='%s']", type);
+    snprintf(buf, sizeof(buf), "/domain/devices/interface[@type='%s']", type);
     obj = xmlXPathEval(BAD_CAST buf, ctxt);
     if ((obj == NULL) || (obj->type != XPATH_NODESET) ||
         (obj->nodesetval == NULL) || (obj->nodesetval->nodeNr == 0)) {