On 03/30/2010 10:20 AM, Matthias Bolte wrote: Why skip leading whitespace yourself... when virStrToLong_ui already does the same thing for you? tmp cannot be NULL at this point. > + return -1; > + > + if (virStrToLong_ui(tmp + 1, &tmp, 16, &pci->bus) < 0 || Likewise. Do we care if there is any garbage in *tmp at this point? Oops - virDomainParseLegacyDeviceAddress returns 0, not 3, on success. The other two conversions in this patch looked okay. By the way, eventually your patch series should probably enable sc_prohibit_atoi_atof in cfg.mk's local-checks-to-skip (if you haven't already been experimenting with that). Turning that on will allow 'make syntax-check' to catch all uses of scanf/atoi. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

--- src/conf/nwfilter_conf.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 668918d..24b79a7 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -1226,7 +1226,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED, case DATATYPE_UINT8: storage_ptr = &item->u.u8; - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) { if (int_val >= 0 && int_val <= 0xff) { if (!validator) *(uint8_t *)storage_ptr = int_val; @@ -1240,7 +1240,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED, case DATATYPE_UINT16: storage_ptr = &item->u.u16; - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) { if (int_val >= 0 && int_val <= 0xffff) { if (!validator) *(uint16_t *)storage_ptr = int_val; @@ -1264,7 +1264,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED, case DATATYPE_IPMASK: storage_ptr = &item->u.u8; if (!virNWIPv4AddressParser(prop, &ipaddr)) { - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) { if (int_val >= 0 && int_val <= 32) { if (!validator) *(uint8_t *)storage_ptr = @@ -1319,7 +1319,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED, case DATATYPE_IPV6MASK: storage_ptr = &item->u.u8; if (!virNWIPv6AddressParser(prop, &ipaddr)) { - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) { if (int_val >= 0 && int_val <= 128) { if (!validator) *(uint8_t *)storage_ptr = @@ -1607,7 +1607,7 @@ virNWFilterRuleParse(virConnectPtr conn, ret->priority = MAX_RULE_PRIORITY / 2; if (prio) { - if (sscanf(prio, "%d", (int *)&priority) == 1) { + if (virStrToLong_i(prio, NULL, 10, (int *)&priority) >= 0) { if ((int)priority >= 0 && priority <= MAX_RULE_PRIORITY) ret->priority = priority; } -- 1.6.3.3

2010/3/30 Jim Meyering <jim(a)meyering.net&gt;: Yes, once scanf and atoi are gone, that rule should be activated. Yes, I'm aware of that. In some cases it is necessary to pass a non-NULL value as second parameter to virStrToLong_i in order to have the same behavior as scanf. For example the new virParseVersionNumber function does this in order to ignore the suffix on the VirtualBox version number 3.1.4_OSE. True this needs to be mentioned in the commit message. I think for some scanfs ignoring trailing stuff in the string should just be ignore and in other places trailing stuff should be considered as error. Matthias

virParseVersionString uses virStrToLong_ui instead of sscanf. This also fixes a bug in the UML driver, that always returned 0 as version number. --- src/esx/esx_driver.c | 31 ++++++------------------------- src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 10 ++-------- src/openvz/openvz_conf.c | 16 ++++++++++------ src/uml/uml_conf.h | 2 +- src/uml/uml_driver.c | 17 +++++++++-------- src/util/util.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/util.h | 1 + src/vbox/vbox_tmpl.c | 14 ++++---------- src/xenapi/xenapi_driver.c | 21 ++++++++++----------- 10 files changed, 81 insertions(+), 69 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index bbe8a51..f745f9a 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -684,36 +684,17 @@ static int esxGetVersion(virConnectPtr conn, unsigned long *version) { esxPrivate *priv = conn->privateData; - char *temp; - unsigned int major, minor, release; - temp = (char *)priv->host->service->about->version; - - /* Expecting 'major.minor.release' format */ - if (virStrToLong_ui(temp, &temp, 10, &major) < 0 || temp == NULL || - *temp != '.') { - goto failure; - } - - if (virStrToLong_ui(temp + 1, &temp, 10, &minor) < 0 || temp == NULL || - *temp != '.') { - goto failure; - } + if (virParseVersionString(priv->host->service->about->version, + version) < 0) { + ESX_ERROR(VIR_ERR_INTERNAL_ERROR, + "Could not parse version number from '%s'", + priv->host->service->about->version); - if (virStrToLong_ui(temp + 1, NULL, 10, &release) < 0) { - goto failure; + return -1; } - *version = 1000000 * major + 1000 * minor + release; - return 0; - - failure: - ESX_ERROR(VIR_ERR_INTERNAL_ERROR, - "Expecting version to match 'major.minor.release', but got '%s'", - priv->host->service->about->version); - - return -1; } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cc943f8..edb23c2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -648,6 +648,7 @@ virFilePid; virFileReadPid; virFileLinkPointsTo; virParseNumber; +virParseVersionString; virPipeReadUntilEOF; virAsprintf; virRun; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 7ebc7ae..9caefa1 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1951,20 +1951,14 @@ lxcActive(void) { static int lxcVersion(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned long *version) { struct utsname ver; - int maj; - int min; - int rev; uname(&ver); - if (sscanf(ver.release, "%i.%i.%i", &maj, &min, &rev) != 3) { - lxcError(VIR_ERR_INTERNAL_ERROR, - _("Unknown release: %s"), ver.release); + if (virParseVersionString(ver.release, version) < 0) { + lxcError(VIR_ERR_INTERNAL_ERROR, _("Unknown release: %s"), ver.release); return -1; } - *version = (maj * 1000 * 1000) + (min * 1000) + rev; - return 0; } diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 3713a45..a580ff0 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -78,8 +78,8 @@ openvzExtractVersionInfo(const char *cmd, int *retversion) pid_t child; int newstdout = -1; int ret = -1, status; - unsigned int major, minor, micro; - unsigned int version; + unsigned long version; + char *tmp; if (retversion) *retversion = 0; @@ -93,12 +93,16 @@ openvzExtractVersionInfo(const char *cmd, int *retversion) if (len < 0) goto cleanup2; - if (sscanf(help, "vzctl version %u.%u.%u", - &major, &minor, &micro) != 3) { + tmp = help; + + /* expected format: vzctl version <major>.<minor>.<micro> */ + if (! STRPREFIX(tmp, "vzctl version ")) goto cleanup2; - } - version = (major * 1000 * 1000) + (minor * 1000) + micro; + tmp += 14; /* = strlen("vzctl version ") */ + + if (virParseVersionString(tmp, &version) < 0) + goto cleanup2; if (retversion) *retversion = version; diff --git a/src/uml/uml_conf.h b/src/uml/uml_conf.h index 70c3f7d..4d434e1 100644 --- a/src/uml/uml_conf.h +++ b/src/uml/uml_conf.h @@ -44,7 +44,7 @@ struct uml_driver { int privileged; - unsigned int umlVersion; + unsigned long umlVersion; int nextvmid; virDomainObjList domains; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 835e5d4..08fbf93 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1225,17 +1225,18 @@ cleanup: static int umlGetVersion(virConnectPtr conn, unsigned long *version) { struct uml_driver *driver = conn->privateData; struct utsname ut; - int major, minor, micro; int ret = -1; - uname(&ut); - umlDriverLock(driver); - if (sscanf(ut.release, "%u.%u.%u", - &major, &minor, &micro) != 3) { - umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot parse version %s"), ut.release); - goto cleanup; + + if (driver->umlVersion == 0) { + uname(&ut); + + if (virParseVersionString(ut.release, &driver->umlVersion) < 0) { + umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("cannot parse version %s"), ut.release); + goto cleanup; + } } *version = driver->umlVersion; diff --git a/src/util/util.c b/src/util/util.c index 62dc5f1..a669d17 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2074,6 +2074,43 @@ virParseNumber(const char **str) return (ret); } + +/** + * virParseVersionString: + * @str: const char pointer to the version string + * @version: unsigned long pointer to output the version number + * + * Parse an unsigned version number from a version string. Expecting + * 'major.minor.micro' format, ignoring an optional suffix. + * + * The major, minor and micro numbers are encoded into a single version number: + * + * 1000000 * major + 1000 * minor + micro + * + * Returns the 0 for success, -1 for error. + */ +int +virParseVersionString(const char *str, unsigned long *version) +{ + unsigned int major, minor, micro; + char *tmp = (char *)str; + + if (virStrToLong_ui(tmp, &tmp, 10, &major) < 0 || tmp == NULL || + *tmp != '.') + return -1; + + if (virStrToLong_ui(tmp + 1, &tmp, 10, &minor) < 0 || tmp == NULL || + *tmp != '.') + return -1; + + if (virStrToLong_ui(tmp + 1, &tmp, 10, &micro) < 0) + return -1; + + *version = 1000000 * major + 1000 * minor + micro; + + return 0; +} + /** * virAsprintf * diff --git a/src/util/util.h b/src/util/util.h index 24dfbfc..c256117 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -189,6 +189,7 @@ int virMacAddrCompare (const char *mac1, const char *mac2); void virSkipSpaces(const char **str); int virParseNumber(const char **str); +int virParseVersionString(const char *str, unsigned long *version); int virAsprintf(char **strp, const char *fmt, ...) ATTRIBUTE_FMT_PRINTF(2, 3); char *virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) ATTRIBUTE_RETURN_CHECK; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index f7a9b9f..05b075f 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -157,7 +157,7 @@ if (strUtf16) {\ typedef struct { virMutex lock; - int version; + unsigned long version; virCapsPtr caps; @@ -713,10 +713,7 @@ cleanup: } static int vboxExtractVersion(virConnectPtr conn, vboxGlobalData *data) { - unsigned int major = 0; - unsigned int minor = 0; - unsigned int micro = 0; - int ret = -1; + int ret = -1; PRUnichar *versionUtf16 = NULL; nsresult rc; @@ -729,20 +726,17 @@ static int vboxExtractVersion(virConnectPtr conn, vboxGlobalData *data) { VBOX_UTF16_TO_UTF8(versionUtf16, &vboxVersion); - if (sscanf(vboxVersion, "%u.%u.%u", &major, &minor, &micro) == 3) + if (virParseVersionString(vboxVersion, &data->version) >= 0) ret = 0; VBOX_UTF8_FREE(vboxVersion); VBOX_COM_UNALLOC_MEM(versionUtf16); - } else { - ret = -1; } - data->version = (major * 1000 * 1000) + (minor * 1000) + micro; - if (ret != 0) vboxError(conn, VIR_ERR_INTERNAL_ERROR,"%s", "Cound not extract VirtualBox version"); + return ret; } diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 6e1183d..7988601 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -253,9 +253,8 @@ xenapiGetVersion (virConnectPtr conn, unsigned long *hvVer) xen_host host; xen_session *session = ((struct _xenapiPrivate *)(conn->privateData))->session; xen_string_string_map *result = NULL; - int i; + int i, ret = -1; char *version = NULL; - unsigned long major = 0, minor = 0, release = 0; if (!(xen_session_get_this_host(session, &host, session))) { xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, NULL); return -1; @@ -278,17 +277,17 @@ xenapiGetVersion (virConnectPtr conn, unsigned long *hvVer) } } if (version) { - if (sscanf(version, "%ld.%ld.%ld", &major, &minor, &release) != 3) { - xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, "Couldn't get version info"); - xen_string_string_map_free(result); - VIR_FREE(version); - return -1; - } - *hvVer = major * 1000000 + minor * 1000 + release; - VIR_FREE(version); + if (virParseVersionString(version, hvVer) < 0) + xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, + "Couldn't parse version info"); + else + ret = 0; xen_string_string_map_free(result); - return 0; + VIR_FREE(version); + return ret; } + xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, + "Couldn't get version info"); } return -1; } -- 1.6.3.3

Eric Blake wrote: Yes, definitely. Good catch. I added it to the list here: diff --git a/cfg.mk b/cfg.mk index 9b8ee00..45da56a 100644 --- a/cfg.mk +++ b/cfg.mk @@ -215,6 +215,7 @@ msg_gen_function += virXenInotifyError msg_gen_function += virXenStoreError msg_gen_function += virXendError msg_gen_function += vshCloseLogFile +msg_gen_function += xenapiSessionErrorHandler msg_gen_function += xenUnifiedError msg_gen_function += xenXMError Then ran "make syntax-check" and get 53 new violations. BTW, I'm seeing a lot of these, too: nwfilter/nwfilter_ebiptables_driver.c:193: warning: format not a string literal and no format arguments [-Wformat-security] nwfilter/nwfilter_ebiptables_driver.c:204: warning: format not a string literal and no format arguments [-Wformat-security] nwfilter/nwfilter_ebiptables_dr...

--- src/vbox/vbox_tmpl.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 05b075f..07996d4 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -598,7 +598,9 @@ static int PRUnicharToInt(PRUnichar *strUtf16) { if (!strUtf8) return -1; - ret = atoi(strUtf8); + if (virStrToLong_i(strUtf8, NULL, 10, &ret) < 0) + return -1; + g_pVBoxGlobalData->pFuncs->pfnUtf8Free(strUtf8); return ret; -- 1.6.3.3

2010/3/31 Eric Blake <eblake(a)redhat.com&gt;: Good catch. It seems that I missed many details in this series :( Matthias

On 03/30/2010 10:20 AM, Matthias Bolte wrote: ACK. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

--- src/xenapi/xenapi_utils.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index 697ad39..790eebf 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -309,7 +309,8 @@ getCpuBitMapfromString(char *mask, unsigned char *cpumap, int maplen) bzero(cpumap, maplen); num = strtok_r(mask, ",", &bp); while (num != NULL) { - if (sscanf(num, "%d", &pos) != 1) + num += strspn(num, " \t"); + if (virStrToLong_i(num, NULL, 10, &pos) < 0) return; if (pos < 0 || pos > max_bits - 1) VIR_WARN ("number in str %d exceeds cpumap's max bits %d", pos, max_bits); -- 1.6.3.3

--- src/xen/xend_internal.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 0649d23..c910439 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -4683,7 +4683,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, else if ((p = strrchr (uri, ':')) != NULL) { /* "hostname:port" */ int port_nr, n; - if (sscanf (p+1, "%d", &port_nr) != 1) { + if (virStrToLong_i(p+1, NULL, 10, &port_nr) != 1) { virXendError (conn, VIR_ERR_INVALID_ARG, "%s", _("xenDaemonDomainMigrate: invalid port number")); return -1; -- 1.6.3.3

This also fixes a problem with MinGW's GCC on Windows. GCC complained bout the L modifier being unknown. --- src/util/pci.c | 59 +++++++++++++++++++++++++++++++++++++++++++------------ 1 files changed, 46 insertions(+), 13 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index 99ec22a..6aa2fe0 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -282,15 +282,26 @@ pciIterDevices(pciIterPredicate predicate, } while ((entry = readdir(dir))) { - unsigned domain, bus, slot, function; + unsigned int domain, bus, slot, function; pciDevice *check; + char *tmp; /* Ignore '.' and '..' */ if (entry->d_name[0] == '.') continue; - if (sscanf(entry->d_name, "%x:%x:%x.%x", - &domain, &bus, &slot, &function) < 4) { + /* expected format: <domain>:<bus>:<slot>.<function> */ + if (/* domain */ + virStrToLong_ui(entry->d_name, &tmp, 16, &domain) < 0 || + tmp == NULL || *tmp != ':' || + /* bus */ + virStrToLong_ui(tmp + 1, &tmp, 16, &bus) < 0 || + tmp == NULL || *tmp != ':' || + /* slot */ + virStrToLong_ui(tmp + 1, &tmp, 16, &slot) < 0 || + tmp == NULL || *tmp != '.' || + /* function */ + virStrToLong_ui(tmp + 1, NULL, 16, &function) < 0) { VIR_WARN("Unusual entry in " PCI_SYSFS "devices: %s", entry->d_name); continue; } @@ -914,10 +925,9 @@ pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher) FILE *fp; char line[160]; unsigned long long start, end; - int consumed; char *rest; - unsigned long long domain; - int bus, slot, function; + char *tmp; + unsigned int domain, bus, slot, function; int in_matching_device; int ret; size_t match_depth; @@ -945,10 +955,18 @@ pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher) * of these situations */ if (in_matching_device && (strspn(line, " ") == (match_depth + 2))) { - if (sscanf(line, "%Lx-%Lx : %n", &start, &end, &consumed) != 2) + tmp = line + strspn(line, " "); + + /* expected format: <start>-<end> : <suffix> */ + if (/* start */ + virStrToLong_ull(tmp, &tmp, 16, &start) < 0 || + tmp == NULL || *tmp != '-' || + /* end */ + virStrToLong_ull(tmp + 1, &tmp, 16, &end) < 0 || + tmp == NULL || ! STRPREFIX(tmp, " : ")) continue; - rest = line + consumed; + rest = tmp + 3; /* = strlen(" : ") */ if (STRPREFIX(rest, matcher)) { ret = 1; break; @@ -956,11 +974,26 @@ pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher) } else { in_matching_device = 0; - if (sscanf(line, "%Lx-%Lx : %n", &start, &end, &consumed) != 2) - continue; - - rest = line + consumed; - if (sscanf(rest, "%Lx:%x:%x.%x", &domain, &bus, &slot, &function) != 4) + tmp = line + strspn(line, " "); + + /* expected format: <start>-<end> : <domain>:<bus>:<slot>.<function> */ + if (/* start */ + virStrToLong_ull(tmp, &tmp, 16, &start) < 0 || + tmp == NULL || *tmp != '-' || + /* end */ + virStrToLong_ull(tmp + 1, &tmp, 16, &end) < 0 || + tmp == NULL || ! STRPREFIX(tmp, " : ") || + /* domain */ + virStrToLong_ui(tmp + 3, &tmp, 16, &domain) < 0 || + tmp == NULL || *tmp != ':' || + /* bus */ + virStrToLong_ui(tmp + 1, &tmp, 16, &bus) < 0 || + tmp == NULL || *tmp != ':' || + /* slot */ + virStrToLong_ui(tmp + 1, &tmp, 16, &slot) < 0 || + tmp == NULL || *tmp != '.' || + /* function */ + virStrToLong_ui(tmp + 1, &tmp, 16, &function) < 0) continue; if (domain != dev->domain || bus != dev->bus || slot != dev->slot || -- 1.6.3.3