From b78e763c1af2ace4c19d2932065a43357e3f5d3e Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Wed, 23 Jan 2019 08:27:37 +0900 Subject: [PATCH] string: spdk_strtol to delegate additional error checking Error check of strtol is left to users of it. But some use cases of strtol in SPDK do not have enough error check yet. For example, strtol returns 0 if there were no digits at all. It should be avoided for each use case to add enough error checking for strtol. Hence spdk_strtol and spdk_strtoll do additional error checking according to the description of manual of strtol. Besides, there is no use case of negative number now, and to keep simplicity, spdk_trtol and spdk_strtoll allows only strings that is positive number or zero. As a result of this policy, callers of them only have to check if the return value is not negative. Subsequent patches will replace atoi to spdk_strtol because atoi does not have error check. Change-Id: If3d549970595e53b1141674e47710fe4dd062bc5 Signed-off-by: Shuhei Matsumoto Reviewed-on: https://review.gerrithub.io/c/441626 Tested-by: SPDK CI Jenkins Reviewed-by: wuzhouhui Reviewed-by: Darek Stojaczyk Reviewed-by: Jim Harris Chandler-Test-Pool: SPDK Automated Test System --- include/spdk/string.h | 28 +++++ lib/bdev/null/bdev_null.c | 5 +- lib/bdev/nvme/bdev_nvme.c | 6 +- lib/bdev/rbd/bdev_rbd.c | 14 ++- lib/conf/conf.c | 4 +- lib/util/string.c | 62 +++++++++++ test/bdev/bdevperf/bdevperf.c | 10 +- test/unit/lib/util/string.c/string_ut.c | 133 +++++++++++++++++++++++- 8 files changed, 245 insertions(+), 17 deletions(-) diff --git a/include/spdk/string.h b/include/spdk/string.h index 901ca23e9..041010e20 100644 --- a/include/spdk/string.h +++ b/include/spdk/string.h @@ -236,6 +236,34 @@ int spdk_parse_capacity(const char *cap_str, uint64_t *cap, bool *has_prefix); */ bool spdk_mem_all_zero(const void *data, size_t size); +/** + * Convert the string in nptr to a long integer value according to the given base. + * + * spdk_strtol() does the additional error checking and allows only strings that + * contains only numbers and is positive number or zero. The caller only has to check + * if the return value is not negative. + * + * \param nptr String containing numbers. + * \param base Base which must be between 2 and 32 inclusive, or be the special value 0. + * + * \return positive number or zero on success, or negative errno on failure. + */ +long int spdk_strtol(const char *nptr, int base); + +/** + * Convert the string in nptr to a long long integer value according to the given base. + * + * spdk_strtoll() does the additional error checking and allows only strings that + * contains only numbers and is positive number or zero. The caller only has to check + * if the return value is not negative. + * + * \param nptr String containing numbers. + * \param base Base which must be between 2 and 32 inclusive, or be the special value 0. + * + * \return positive number or zero on success, or negative errno on failure. + */ +long long int spdk_strtoll(const char *nptr, int base); + #ifdef __cplusplus } #endif diff --git a/lib/bdev/null/bdev_null.c b/lib/bdev/null/bdev_null.c index 9ff64725d..5f0dd0d51 100644 --- a/lib/bdev/null/bdev_null.c +++ b/lib/bdev/null/bdev_null.c @@ -38,6 +38,7 @@ #include "spdk/env.h" #include "spdk/thread.h" #include "spdk/json.h" +#include "spdk/string.h" #include "spdk/bdev_module.h" #include "spdk_internal/log.h" @@ -329,8 +330,8 @@ bdev_null_initialize(void) block_size = 512; } else { errno = 0; - block_size = (int)strtol(val, NULL, 10); - if (errno) { + block_size = (int)spdk_strtol(val, 10); + if (block_size <= 0) { SPDK_ERRLOG("Null entry %d: Invalid block size %s\n", i, val); continue; } diff --git a/lib/bdev/nvme/bdev_nvme.c b/lib/bdev/nvme/bdev_nvme.c index 63582c5d8..64b3f468c 100644 --- a/lib/bdev/nvme/bdev_nvme.c +++ b/lib/bdev/nvme/bdev_nvme.c @@ -1332,13 +1332,11 @@ bdev_nvme_library_init(void) val = spdk_conf_section_get_val(sp, "TimeoutUsec"); if (val != NULL) { - intval = strtoll(val, NULL, 10); - if (intval == LLONG_MIN || intval == LLONG_MAX) { + intval = spdk_strtoll(val, 10); + if (intval < 0) { SPDK_ERRLOG("Invalid TimeoutUsec value\n"); rc = -1; goto end; - } else if (intval < 0) { - intval = 0; } } diff --git a/lib/bdev/rbd/bdev_rbd.c b/lib/bdev/rbd/bdev_rbd.c index 593ecf2ea..d3fd02e54 100644 --- a/lib/bdev/rbd/bdev_rbd.c +++ b/lib/bdev/rbd/bdev_rbd.c @@ -786,6 +786,7 @@ bdev_rbd_library_init(void) const char *pool_name; const char *rbd_name; uint32_t block_size; + long int tmp; struct spdk_conf_section *sp = spdk_conf_find_section(NULL, "Ceph"); @@ -823,13 +824,18 @@ bdev_rbd_library_init(void) if (val == NULL) { block_size = 512; /* default value */ } else { - block_size = (int)strtol(val, NULL, 10); - if (block_size & 0x1ff) { - SPDK_ERRLOG("current block_size = %d, it should be multiple of 512\n", - block_size); + tmp = spdk_strtol(val, 10); + if (tmp <= 0) { + SPDK_ERRLOG("Invalid block size\n"); + rc = -1; + goto end; + } else if (tmp & 0x1ff) { + SPDK_ERRLOG("current block_size = %ld, it should be multiple of 512\n", + tmp); rc = -1; goto end; } + block_size = (uint32_t)tmp; } /* TODO(?): user_id and rbd config values */ diff --git a/lib/conf/conf.c b/lib/conf/conf.c index 384b088cf..c419b3a19 100644 --- a/lib/conf/conf.c +++ b/lib/conf/conf.c @@ -419,7 +419,7 @@ spdk_conf_section_get_intval(struct spdk_conf_section *sp, const char *key) return -1; } - value = (int)strtol(v, NULL, 10); + value = (int)spdk_strtol(v, 10); return value; } @@ -474,7 +474,7 @@ parse_line(struct spdk_conf *cp, char *lp) for (p = key; *p != '\0' && !isdigit((int) *p); p++) ; if (*p != '\0') { - num = (int)strtol(p, NULL, 10); + num = (int)spdk_strtol(p, 10); } else { num = 0; } diff --git a/lib/util/string.c b/lib/util/string.c index 4702cabeb..cc22a9638 100644 --- a/lib/util/string.c +++ b/lib/util/string.c @@ -410,3 +410,65 @@ spdk_mem_all_zero(const void *data, size_t size) return true; } + +long int +spdk_strtol(const char *nptr, int base) +{ + long val; + char *endptr; + + /* Since strtoll() can legitimately return 0, LONG_MAX, or LONG_MIN + * on both success and failure, the calling program should set errno + * to 0 before the call. + */ + errno = 0; + + val = strtol(nptr, &endptr, base); + + if (*endptr != '\0') { + /* Non integer character was found. */ + return -EINVAL; + } else if (errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) { + /* Overflow occurred. */ + return -ERANGE; + } else if (errno != 0 && val == 0) { + /* Other error occurred. */ + return -errno; + } else if (val < 0) { + /* Input string was negative number. */ + return -ERANGE; + } + + return val; +} + +long long int +spdk_strtoll(const char *nptr, int base) +{ + long long val; + char *endptr; + + /* Since strtoll() can legitimately return 0, LLONG_MAX, or LLONG_MIN + * on both success and failure, the calling program should set errno + * to 0 before the call. + */ + errno = 0; + + val = strtoll(nptr, &endptr, base); + + if (*endptr != '\0') { + /* Non integer character was found. */ + return -EINVAL; + } else if (errno == ERANGE && (val == LLONG_MAX || val == LLONG_MIN)) { + /* Overflow occurred. */ + return -ERANGE; + } else if (errno != 0 && val == 0) { + /* Other error occurred. */ + return -errno; + } else if (val < 0) { + /* Input string was negative number. */ + return -ERANGE; + } + + return val; +} diff --git a/test/bdev/bdevperf/bdevperf.c b/test/bdev/bdevperf/bdevperf.c index 927963053..30eb1fe27 100644 --- a/test/bdev/bdevperf/bdevperf.c +++ b/test/bdev/bdevperf/bdevperf.c @@ -827,14 +827,16 @@ static int bdevperf_parse_arg(int ch, char *arg) { long long tmp; - char *end; if (ch == 'w') { g_workload_type = optarg; } else { - tmp = strtoll(optarg, &end, 10); - if (tmp <= INT_MIN || tmp >= INT_MAX) { - fprintf(stderr, "-%c out of range. Parse failed\n", ch); + tmp = spdk_strtoll(optarg, 10); + if (tmp < 0) { + fprintf(stderr, "Parse failed for the option %c.\n", ch); + return tmp; + } else if (tmp >= INT_MAX) { + fprintf(stderr, "Parsed option was too large %c.\n", ch); return -ERANGE; } diff --git a/test/unit/lib/util/string.c/string_ut.c b/test/unit/lib/util/string.c/string_ut.c index 9aadd414b..4ee2c7943 100644 --- a/test/unit/lib/util/string.c/string_ut.c +++ b/test/unit/lib/util/string.c/string_ut.c @@ -248,6 +248,135 @@ test_sprintf_append_realloc(void) free(str3); free(str4); } +static void +test_strtol(void) +{ + long int val; + + const char *val1 = "no_digits"; + /* LLONG_MIN - 1 */ + const char *val2 = "-9223372036854775809"; + /* LONG_MIN */ + const char *val3 = "-9223372036854775808"; + /* LONG_MIN + 1 */ + const char *val4 = "-9223372036854775807"; + /* LONG_MAX - 1 */ + const char *val5 = "9223372036854775806"; + /* LONG_MAX */ + const char *val6 = "9223372036854775807"; + /* LONG_MAX + 1 */ + const char *val7 = "9223372036854775808"; + /* digits + chars */ + const char *val8 = "10_is_ten"; + /* chars + digits */ + const char *val9 = "ten_is_10"; + /* all zeroes */ + const char *val10 = "00000000"; + /* leading minus sign, but not negative */ + const char *val11 = "-0"; + + val = spdk_strtol(val1, 10); + CU_ASSERT(val == -EINVAL); + + val = spdk_strtol(val2, 10); + CU_ASSERT(val == -ERANGE); + + val = spdk_strtol(val3, 10); + CU_ASSERT(val == -ERANGE); + + val = spdk_strtol(val4, 10); + CU_ASSERT(val == -ERANGE); + + val = spdk_strtol(val5, 10); + CU_ASSERT(val == LONG_MAX - 1); + + val = spdk_strtol(val6, 10); + CU_ASSERT(val == LONG_MAX); + + val = spdk_strtol(val7, 10); + CU_ASSERT(val == -ERANGE); + + val = spdk_strtol(val8, 10); + CU_ASSERT(val == -EINVAL); + + val = spdk_strtol(val9, 10); + CU_ASSERT(val == -EINVAL); + + val = spdk_strtol(val10, 10); + CU_ASSERT(val == 0); + + /* Invalid base */ + val = spdk_strtol(val10, 1); + CU_ASSERT(val == -EINVAL); + + val = spdk_strtol(val11, 10); + CU_ASSERT(val == 0); +} + +static void +test_strtoll(void) +{ + long long int val; + + const char *val1 = "no_digits"; + /* LLONG_MIN - 1 */ + const char *val2 = "-9223372036854775809"; + /* LLONG_MIN */ + const char *val3 = "-9223372036854775808"; + /* LLONG_MIN + 1 */ + const char *val4 = "-9223372036854775807"; + /* LLONG_MAX - 1 */ + const char *val5 = "9223372036854775806"; + /* LLONG_MAX */ + const char *val6 = "9223372036854775807"; + /* LLONG_MAX + 1 */ + const char *val7 = "9223372036854775808"; + /* digits + chars */ + const char *val8 = "10_is_ten"; + /* chars + digits */ + const char *val9 = "ten_is_10"; + /* all zeroes */ + const char *val10 = "00000000"; + /* leading minus sign, but not negative */ + const char *val11 = "-0"; + + val = spdk_strtoll(val1, 10); + CU_ASSERT(val == -EINVAL); + + val = spdk_strtoll(val2, 10); + CU_ASSERT(val == -ERANGE); + + val = spdk_strtoll(val3, 10); + CU_ASSERT(val == -ERANGE); + + val = spdk_strtoll(val4, 10); + CU_ASSERT(val == -ERANGE); + + val = spdk_strtoll(val5, 10); + CU_ASSERT(val == LLONG_MAX - 1); + + val = spdk_strtoll(val6, 10); + CU_ASSERT(val == LLONG_MAX); + + val = spdk_strtoll(val7, 10); + CU_ASSERT(val == -ERANGE); + + val = spdk_strtoll(val8, 10); + CU_ASSERT(val == -EINVAL); + + val = spdk_strtoll(val9, 10); + CU_ASSERT(val == -EINVAL); + + val = spdk_strtoll(val10, 10); + CU_ASSERT(val == 0); + + /* Invalid base */ + val = spdk_strtoll(val10, 1); + CU_ASSERT(val == -EINVAL); + + val = spdk_strtoll(val11, 10); + CU_ASSERT(val == 0); +} int main(int argc, char **argv) @@ -269,7 +398,9 @@ main(int argc, char **argv) CU_add_test(suite, "test_parse_ip_addr", test_parse_ip_addr) == NULL || CU_add_test(suite, "test_str_chomp", test_str_chomp) == NULL || CU_add_test(suite, "test_parse_capacity", test_parse_capacity) == NULL || - CU_add_test(suite, "test_sprintf_append_realloc", test_sprintf_append_realloc) == NULL) { + CU_add_test(suite, "test_sprintf_append_realloc", test_sprintf_append_realloc) == NULL || + CU_add_test(suite, "test_strtol", test_strtol) == NULL || + CU_add_test(suite, "test_strtoll", test_strtoll) == NULL) { CU_cleanup_registry(); return CU_get_error(); }