4:12 p.m.
* 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.
---
diff in v2 - actually include the changes to src/vbox/vbox_templ.c
.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 +-
src/vbox/vbox_tmpl.c | 46 +++++++++++++++++++++++++-------------
tools/virsh.c | 2 +-
8 files changed, 61 insertions(+), 26 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/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index ddbca97..f0ba36f 100644
--- 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 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