From e8aba782e755760a77a8a35f9583e4e31995cda4 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 18 Aug 2010 17:31:39 -0600 Subject: [PATCH] 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. --- .x-sc_prohibit_sprintf | 4 +++ HACKING | 6 +++++ Makefile.am | 1 + 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 +- 10 files changed, 69 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 0000000000..a67f51c098 --- /dev/null +++ b/.x-sc_prohibit_sprintf @@ -0,0 +1,4 @@ +^docs/ +^po/ +^ChangeLog +^HACKING diff --git a/HACKING b/HACKING index 2711ea1e08..17ad34479a 100644 --- 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 720dff961d..e88814467d 100644 --- 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 9d5fa76f1c..286b9022c3 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='\' \ + 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 c42654e0b7..e1b51856c7 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -649,6 +649,15 @@ 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/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index d9489083fd..7963695343 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 4aaf34803e..936a1a6b9b 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 7423956f24..ca4e7be695 100644 --- 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 78f945cefc..a210696a63 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 4ef556a70f..ae88cc022c 100644 --- 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)) {