From a4d12f2a71d80a21aa2d04baa2e4cd525aea517a Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 9 Aug 2022 19:27:22 +0900 Subject: [PATCH] Replace the use of strtoul and strtol with parse_uint Replace the use of strtoul and strtol with parse_uint to fix the handling of negative integer. --- src/CMakeLists.txt | 2 ++ src/Makefile.am | 5 +++- src/deflatehd.cc | 21 +++++++------- src/h2load.cc | 69 +++++++++++++++++++++++++++++++-------------- src/nghttp.cc | 66 ++++++++++++++++++++++++++++--------------- src/nghttpd.cc | 33 ++++++++++++++-------- src/shrpx_config.cc | 20 ------------- 7 files changed, 130 insertions(+), 86 deletions(-) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 8057de1b..1aa1f764 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -235,6 +235,8 @@ if(ENABLE_HPACK_TOOLS) set(deflatehd_SOURCES deflatehd.cc comp_helper.c + util.cc + timegm.c ) add_executable(inflatehd ${inflatehd_SOURCES}) add_executable(deflatehd ${deflatehd_SOURCES}) diff --git a/src/Makefile.am b/src/Makefile.am index 2fd6eadd..694025e2 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -246,7 +246,10 @@ if ENABLE_HPACK_TOOLS bin_PROGRAMS += inflatehd deflatehd -HPACK_TOOLS_COMMON_SRCS = comp_helper.c comp_helper.h +HPACK_TOOLS_COMMON_SRCS = \ + comp_helper.c comp_helper.h \ + util.cc util.h \ + timegm.c timegm.h inflatehd_SOURCES = inflatehd.cc $(HPACK_TOOLS_COMMON_SRCS) diff --git a/src/deflatehd.cc b/src/deflatehd.cc index 5d28e241..7dcfccff 100644 --- a/src/deflatehd.cc +++ b/src/deflatehd.cc @@ -45,6 +45,7 @@ #include "template.h" #include "comp_helper.h" +#include "util.h" namespace nghttp2 { @@ -381,8 +382,6 @@ constexpr static struct option long_options[] = { {nullptr, 0, nullptr, 0}}; int main(int argc, char **argv) { - char *end; - config.table_size = 4_k; config.deflate_table_size = 4_k; config.http1text = 0; @@ -401,24 +400,26 @@ int main(int argc, char **argv) { // --http1text config.http1text = 1; break; - case 's': + case 's': { // --table-size - errno = 0; - config.table_size = strtoul(optarg, &end, 10); - if (errno == ERANGE || *end != '\0') { + auto n = util::parse_uint(optarg); + if (n == -1) { fprintf(stderr, "-s: Bad option value\n"); exit(EXIT_FAILURE); } + config.table_size = n; break; - case 'S': + } + case 'S': { // --deflate-table-size - errno = 0; - config.deflate_table_size = strtoul(optarg, &end, 10); - if (errno == ERANGE || *end != '\0') { + auto n = util::parse_uint(optarg); + if (n == -1) { fprintf(stderr, "-S: Bad option value\n"); exit(EXIT_FAILURE); } + config.deflate_table_size = n; break; + } case 'd': // --dump-header-table config.dump_header_table = 1; diff --git a/src/h2load.cc b/src/h2load.cc index e950bd28..6e5ec340 100644 --- a/src/h2load.cc +++ b/src/h2load.cc @@ -2370,44 +2370,65 @@ int main(int argc, char **argv) { break; } switch (c) { - case 'n': - config.nreqs = strtoul(optarg, nullptr, 10); + case 'n': { + auto n = util::parse_uint(optarg); + if (n == -1) { + std::cerr << "-n: bad option value: " << optarg << std::endl; + exit(EXIT_FAILURE); + } + config.nreqs = n; nreqs_set_manually = true; break; - case 'c': - config.nclients = strtoul(optarg, nullptr, 10); + } + case 'c': { + auto n = util::parse_uint(optarg); + if (n == -1) { + std::cerr << "-c: bad option value: " << optarg << std::endl; + exit(EXIT_FAILURE); + } + config.nclients = n; break; + } case 'd': datafile = optarg; break; - case 't': + case 't': { #ifdef NOTHREADS std::cerr << "-t: WARNING: Threading disabled at build time, " << "no threads created." << std::endl; #else - config.nthreads = strtoul(optarg, nullptr, 10); + auto n = util::parse_uint(optarg); + if (n == -1) { + std::cerr << "-t: bad option value: " << optarg << std::endl; + exit(EXIT_FAILURE); + } + config.nthreads = n; #endif // NOTHREADS break; - case 'm': - config.max_concurrent_streams = strtoul(optarg, nullptr, 10); + } + case 'm': { + auto n = util::parse_uint(optarg); + if (n == -1) { + std::cerr << "-m: bad option value: " << optarg << std::endl; + exit(EXIT_FAILURE); + } + config.max_concurrent_streams = n; break; + } case 'w': case 'W': { - errno = 0; - char *endptr = nullptr; - auto n = strtoul(optarg, &endptr, 10); - if (errno == 0 && *endptr == '\0' && n < 31) { - if (c == 'w') { - config.window_bits = n; - } else { - config.connection_window_bits = n; - } - } else { + auto n = util::parse_uint(optarg); + if (n == -1 || n > 30) { std::cerr << "-" << static_cast(c) << ": specify the integer in the range [0, 30], inclusive" << std::endl; exit(EXIT_FAILURE); } + if (c == 'w') { + config.window_bits = n; + } else { + config.connection_window_bits = n; + } break; } case 'f': { @@ -2470,14 +2491,20 @@ int main(int argc, char **argv) { } break; } - case 'r': - config.rate = strtoul(optarg, nullptr, 10); - if (config.rate == 0) { + case 'r': { + auto n = util::parse_uint(optarg); + if (n == -1) { + std::cerr << "-r: bad option value: " << optarg << std::endl; + exit(EXIT_FAILURE); + } + if (n == 0) { std::cerr << "-r: the rate at which connections are made " << "must be positive." << std::endl; exit(EXIT_FAILURE); } + config.rate = n; break; + } case 'T': config.conn_active_timeout = util::parse_duration_with_unit(optarg); if (!std::isfinite(config.conn_active_timeout)) { diff --git a/src/nghttp.cc b/src/nghttp.cc index 06ca7888..e6e0f885 100644 --- a/src/nghttp.cc +++ b/src/nghttp.cc @@ -2832,33 +2832,43 @@ int main(int argc, char **argv) { break; } switch (c) { - case 'M': + case 'M': { // peer-max-concurrent-streams option - config.peer_max_concurrent_streams = strtoul(optarg, nullptr, 10); + auto n = util::parse_uint(optarg); + if (n == -1) { + std::cerr << "-M: Bad option value: " << optarg << std::endl; + exit(EXIT_FAILURE); + } + config.peer_max_concurrent_streams = n; break; + } case 'O': config.remote_name = true; break; case 'h': print_help(std::cout); exit(EXIT_SUCCESS); - case 'b': - config.padding = strtol(optarg, nullptr, 10); + case 'b': { + auto n = util::parse_uint(optarg); + if (n == -1) { + std::cerr << "-b: Bad option value: " << optarg << std::endl; + exit(EXIT_FAILURE); + } + config.padding = n; break; + } case 'n': config.null_out = true; break; case 'p': { - errno = 0; - auto n = strtoul(optarg, nullptr, 10); - if (errno == 0 && NGHTTP2_MIN_WEIGHT <= n && n <= NGHTTP2_MAX_WEIGHT) { - config.weight.push_back(n); - } else { + auto n = util::parse_uint(optarg); + if (n == -1 || NGHTTP2_MIN_WEIGHT > n || n > NGHTTP2_MAX_WEIGHT) { std::cerr << "-p: specify the integer in the range [" << NGHTTP2_MIN_WEIGHT << ", " << NGHTTP2_MAX_WEIGHT << "], inclusive" << std::endl; exit(EXIT_FAILURE); } + config.weight.push_back(n); break; } case 'r': @@ -2884,21 +2894,18 @@ int main(int argc, char **argv) { break; case 'w': case 'W': { - errno = 0; - char *endptr = nullptr; - unsigned long int n = strtoul(optarg, &endptr, 10); - if (errno == 0 && *endptr == '\0' && n < 31) { - if (c == 'w') { - config.window_bits = n; - } else { - config.connection_window_bits = n; - } - } else { + auto n = util::parse_uint(optarg); + if (n == -1 || n > 30) { std::cerr << "-" << static_cast(c) << ": specify the integer in the range [0, 30], inclusive" << std::endl; exit(EXIT_FAILURE); } + if (c == 'w') { + config.window_bits = n; + } else { + config.connection_window_bits = n; + } break; } case 'H': { @@ -2939,9 +2946,15 @@ int main(int argc, char **argv) { case 'd': config.datafile = optarg; break; - case 'm': - config.multiply = strtoul(optarg, nullptr, 10); + case 'm': { + auto n = util::parse_uint(optarg); + if (n == -1) { + std::cerr << "-m: Bad option value: " << optarg << std::endl; + exit(EXIT_FAILURE); + } + config.multiply = n; break; + } case 'c': { auto n = util::parse_uint_with_unit(optarg); if (n == -1) { @@ -3025,10 +3038,17 @@ int main(int argc, char **argv) { // no-push option config.no_push = true; break; - case 12: + case 12: { // max-concurrent-streams option - config.max_concurrent_streams = strtoul(optarg, nullptr, 10); + auto n = util::parse_uint(optarg); + if (n == -1) { + std::cerr << "--max-concurrent-streams: Bad option value: " << optarg + << std::endl; + exit(EXIT_FAILURE); + } + config.max_concurrent_streams = n; break; + } case 13: // expect-continue option config.expect_continue = true; diff --git a/src/nghttpd.cc b/src/nghttpd.cc index 8fd66993..3c5f5b4b 100644 --- a/src/nghttpd.cc +++ b/src/nghttpd.cc @@ -250,9 +250,15 @@ int main(int argc, char **argv) { case 'V': config.verify_client = true; break; - case 'b': - config.padding = strtol(optarg, nullptr, 10); + case 'b': { + auto n = util::parse_uint(optarg); + if (n == -1) { + std::cerr << "-b: Bad option value: " << optarg << std::endl; + exit(EXIT_FAILURE); + } + config.padding = n; break; + } case 'd': config.htdocs = optarg; break; @@ -274,13 +280,12 @@ int main(int argc, char **argv) { std::cerr << "-n: WARNING: Threading disabled at build time, " << "no threads created." << std::endl; #else - char *end; - errno = 0; - config.num_worker = strtoul(optarg, &end, 10); - if (errno == ERANGE || *end != '\0' || config.num_worker == 0) { + auto n = util::parse_uint(optarg); + if (n == -1) { std::cerr << "-n: Bad option value: " << optarg << std::endl; exit(EXIT_FAILURE); } + config.num_worker = n; #endif // NOTHREADS break; } @@ -311,10 +316,8 @@ int main(int argc, char **argv) { break; case 'w': case 'W': { - char *endptr; - errno = 0; - auto n = strtoul(optarg, &endptr, 10); - if (errno != 0 || *endptr != '\0' || n >= 31) { + auto n = util::parse_uint(optarg); + if (n == -1 || n > 30) { std::cerr << "-" << static_cast(c) << ": specify the integer in the range [0, 30], inclusive" << std::endl; @@ -432,7 +435,15 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } - config.port = strtol(argv[optind++], nullptr, 10); + { + auto portStr = argv[optind++]; + auto n = util::parse_uint(portStr); + if (n == -1 || n > std::numeric_limits::max()) { + std::cerr << ": Bad value: " << portStr << std::endl; + exit(EXIT_FAILURE); + } + config.port = n; + } if (!config.no_tls) { config.private_key_file = argv[optind++]; diff --git a/src/shrpx_config.cc b/src/shrpx_config.cc index 25bf4e48..1a609052 100644 --- a/src/shrpx_config.cc +++ b/src/shrpx_config.cc @@ -423,26 +423,6 @@ int parse_uint_with_unit(T *dest, const StringRef &opt, } } // namespace -// Parses |optarg| as signed integer. This requires |optarg| to be -// NULL-terminated string. -template -int parse_int(T *dest, const StringRef &opt, const char *optarg) { - char *end = nullptr; - - errno = 0; - - auto val = strtol(optarg, &end, 10); - - if (!optarg[0] || errno != 0 || *end) { - LOG(ERROR) << opt << ": bad value. Specify an integer."; - return -1; - } - - *dest = val; - - return 0; -} - namespace { int parse_altsvc(AltSvc &altsvc, const StringRef &opt, const StringRef &optarg) {