[libvirt] [PATCH] maint: avoid remaining sprintf uses

[Eric Blake]

* cfg.mk (sc_prohibit_sprintf): New rule.
(sc_prohibit_asprintf): Avoid false positives.
* docs/hacking.html.in (Printf-style functions): Document the
policy.
* .x-sc_prohibit_sprintf: New exemptions.
* 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.
---
 .x-sc_prohibit_sprintf          |    3 +++
 cfg.mk                          |   13 ++++++++++---
 docs/hacking.html.in            |    9 +++++++++
 src/openvz/openvz_driver.c      |    5 +++--
 src/security/security_selinux.c |    6 +++---
 src/uml/uml_driver.c            |    3 ++-
 tools/virsh.c                   |    2 +-
 7 files changed, 31 insertions(+), 10 deletions(-)
 create mode 100644 .x-sc_prohibit_sprintf

diff --git a/.x-sc_prohibit_sprintf b/.x-sc_prohibit_sprintf
new file mode 100644
index 0000000..0a1f448
--- /dev/null
+++ b/.x-sc_prohibit_sprintf
@@ -0,0 +1,3 @@
+^docs/
+^po/
+ChangeLog
diff --git a/cfg.mk b/cfg.mk
index 16c2ae3..01cada8 100644
--- 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 bd8b443..a79250e 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -602,6 +602,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 2893f69..f799691 100644
--- 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 7dd9b14..5e0e7bb 100644
--- 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 348f299..5b2a553 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -655,7 +655,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/tools/virsh.c b/tools/virsh.c
index cd20d34..053aee7 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -8450,7 +8450,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)) {
-- 
1.7.3.2