diff --git a/.x-sc_prohibit_sprintf b/.x-sc_prohibit_sprintf new file mode 100644 index 0000000000000000000000000000000000000000..a67f51c0981eb99855a396c1ef7dd66cb68d4bd0 --- /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 --- 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 --- 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 --- 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 c42654e0b7e47611505dda72b88829b8e9d1bd7c..e1b51856c74a48572b42e3b87e5f27704d7d8650 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. +
+diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index d9489083fd39c019e86eb4c01da6d42c0efc7775..796369534322b2344cce8e55062af1e67d19dc5e 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 4aaf34803e140990442693bfb769eb0873a8ec42..936a1a6b9b41d484a18d41b1e9825e347744b648 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 7423956f241f3d181f7d44101afcbc3a04423694..ca4e7be695a2cf573bb9bed76bb633ce0807cee6 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 78f945cefca84059e86b3e1283295d7a738c0722..a210696a63a9139cd4b14a375d264eb3ca61f497 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 4ef556a70f9ad0fa296e163992c3623f25ca568f..ae88cc022c05ba0abb5ab5d8da6f54d166393c8d 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)) {