From 1ec949998ea6bc2c21e808346052b2e92d50479b Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Thu, 31 May 2018 13:23:10 -0400 Subject: [PATCH] Fix handling of errors with strtol(), factor out port parsing * Set errno to 0 before calling strtol() * Fix warnings in gssconn_{server,client}.c * Add tr_parse_port() to tr_inet_util.[ch] and use throughout the codebase for parsing port numbers --- common/tr_inet_util.c | 40 ++++++++++++++++++++++++++++++++++++---- gsscon/test/gsscon_client.c | 13 ++++++++++--- gsscon/test/gsscon_server.c | 11 +++++++++-- include/tr_inet_util.h | 1 + tid/example/tidc_main.c | 15 ++++++++++++++- tr/trmon_main.c | 40 +++++++++++++++++++++++++++------------- tr/trpc_main.c | 15 ++++++++++++++- 7 files changed, 111 insertions(+), 24 deletions(-) diff --git a/common/tr_inet_util.c b/common/tr_inet_util.c index 6d84918..28f52fe 100644 --- a/common/tr_inet_util.c +++ b/common/tr_inet_util.c @@ -39,6 +39,7 @@ #include #include +#include /** * Determine whether a string is a valid address of a given family @@ -158,7 +159,7 @@ char *tr_parse_host(TALLOC_CTX *mem_ctx, const char *s, int *port_out) { const char *colon; char *hostname; - long int port; + int port; if (s == NULL) return NULL; @@ -189,9 +190,9 @@ char *tr_parse_host(TALLOC_CTX *mem_ctx, const char *s, int *port_out) if (colon == NULL) { *port_out = 0; } else { - port = strtol(colon+1, NULL, 10); - if (tr_str_all_digits(colon+1) && (port > 0) && (port <= 65535)) - *port_out = (int) port; + port = tr_parse_port(colon+1); + if ((port > 0) && tr_str_all_digits(colon+1)) + *port_out = port; else *port_out = -1; } @@ -218,4 +219,35 @@ TR_NAME *tr_hostname_and_port_to_name(TR_NAME *hn, int port) } return retval; +} + +/** + * Parse a string containing a port + * + * Returns the port number, which is always in the range 1-65535. + * On error, returns < 0. The absolute value is an error code from errno.h + * + * @param s + * @return port number or < 0 on error + */ +int tr_parse_port(const char *s) +{ + long port; + char *end; + + errno = 0; /* strtol sets this, make sure it's zero to avoid false positives */ + port = strtol(s, &end, 10); + if (errno) { + return -errno; + } + + if (*end != '\0') { + return -EINVAL; + } + + if ((port <= 0) || (port > 65535)) { + return -ERANGE; + } + + return (int) port; } \ No newline at end of file diff --git a/gsscon/test/gsscon_client.c b/gsscon/test/gsscon_client.c index 37430ee..c2815d2 100644 --- a/gsscon/test/gsscon_client.c +++ b/gsscon/test/gsscon_client.c @@ -68,6 +68,7 @@ int main (int argc, const char *argv[]) int err = 0; int fd = -1; int port = kDefaultPort; + long tmp; const char *server = "127.0.0.1"; const char *clientName = NULL; const char *serviceName = "host"; @@ -77,8 +78,14 @@ int main (int argc, const char *argv[]) for (i = 1; (i < argc) && !err; i++) { if ((strcmp (argv[i], "--port") == 0) && (i < (argc - 1))) { - port = strtol (argv[++i], NULL, 0); - if (port == 0) { err = errno; } + errno = 0; /* ensure this starts off as 0 */ + tmp = strtol (argv[++i], NULL, 0); + if (errno) + err = errno; + else if ((tmp <= 0) || (tmp > 65535)) + err = ERANGE; + else + port = (int) tmp; } else if ((strcmp (argv[i], "--server") == 0) && (i < (argc - 1))) { server = argv[++i]; } else if ((strcmp(argv[i], "--cprinc") == 0) && (i < (argc - 1))) { @@ -93,7 +100,7 @@ int main (int argc, const char *argv[]) if (!err) { printf ("%s: Starting up...\n", argv[0]); - err = gsscon_connect (server, port, serviceName, &fd, &gssContext); + err = gsscon_connect (server, (unsigned int) port, serviceName, &fd, &gssContext); } if (!err) { diff --git a/gsscon/test/gsscon_server.c b/gsscon/test/gsscon_server.c index be56372..3ca69c4 100644 --- a/gsscon/test/gsscon_server.c +++ b/gsscon/test/gsscon_server.c @@ -129,6 +129,7 @@ int main (int argc, const char *argv[]) int err = 0; OM_uint32 minorStatus; int port = kDefaultPort; + long tmp; int listenFD = -1; gss_ctx_id_t gssContext = GSS_C_NO_CONTEXT; int i = 0; @@ -136,8 +137,14 @@ int main (int argc, const char *argv[]) for (i = 1; (i < argc) && !err; i++) { if ((strcmp (argv[i], "--port") == 0) && (i < (argc - 1))) { - port = strtol (argv[++i], NULL, 0); - if (port == 0) { err = errno; } + errno = 0; /* ensure this starts off at 0 */ + tmp = strtol (argv[++i], NULL, 0); + if (errno) + err = errno; + else if ((tmp <= 0) || (tmp > 65535)) + err = ERANGE; + else + port = (int) tmp; } else if ((strcmp(argv[i], "--service") == 0) && (i < (argc - 1))) { gServiceName = argv[++i]; } else { diff --git a/include/tr_inet_util.h b/include/tr_inet_util.h index 44d8ba5..f24bbf8 100644 --- a/include/tr_inet_util.h +++ b/include/tr_inet_util.h @@ -41,5 +41,6 @@ char *tr_parse_host(TALLOC_CTX *mem_ctx, const char *s, int *port_out); TR_NAME *tr_hostname_and_port_to_name(TR_NAME *hn, int port); +int tr_parse_port(const char *s); #endif //TRUST_ROUTER_TR_INET_UTIL_H diff --git a/tid/example/tidc_main.c b/tid/example/tidc_main.c index 02cd1db..f1dfce5 100644 --- a/tid/example/tidc_main.c +++ b/tid/example/tidc_main.c @@ -42,6 +42,7 @@ #include #include #include +#include static void tidc_resp_handler (TIDC_INSTANCE * tidc, TID_REQ *req, @@ -146,7 +147,19 @@ static error_t parse_option(int key, char *arg, struct argp_state *state) break; case 4: - arguments->port=strtol(arg, NULL, 10); /* optional */ + arguments->port=tr_parse_port(arg); /* optional */ + if (arguments->port < 0) { + switch(-(arguments->port)) { + case ERANGE: + printf("\nError parsing port (%s): port must be an integer in the range 1 - 65535\n\n", arg); + break; + + default: + printf("\nError parsing port (%s): %s\n\n", arg, strerror(-arguments->port)); + break; + } + argp_usage(state); + } break; default: diff --git a/tr/trmon_main.c b/tr/trmon_main.c index 7ec6a4f..b1a2a63 100644 --- a/tr/trmon_main.c +++ b/tr/trmon_main.c @@ -39,6 +39,7 @@ #include #include +#include /* command-line option setup */ @@ -75,7 +76,7 @@ struct cmdline_args { /* parser for individual options - fills in a struct cmdline_args */ static error_t parse_option(int key, char *arg, struct argp_state *state) { - long tmp_l = 0; + int err = 0; /* get a shorthand to the command line argument structure, part of state */ struct cmdline_args *arguments=state->input; @@ -84,7 +85,6 @@ static error_t parse_option(int key, char *arg, struct argp_state *state) case 'v': print_version_info(); exit(0); - break; case ARGP_KEY_ARG: /* handle argument (not option) */ switch (state->arg_num) { @@ -93,31 +93,40 @@ static error_t parse_option(int key, char *arg, struct argp_state *state) break; case 1: - tmp_l = strtol(arg, NULL, 10); - if (errno || (tmp_l < 0) || (tmp_l > 65535)) /* max valid port */ + arguments->port=tr_parse_port(arg); /* optional */ + if (arguments->port < 0) { + switch(-(arguments->port)) { + case ERANGE: + printf("\nError parsing port (%s): port must be an integer in the range 1 - 65535\n\n", arg); + break; + + default: + printf("\nError parsing port (%s): %s\n\n", arg, strerror(-arguments->port)); + break; + } argp_usage(state); - - arguments->port = (int) tmp_l; /* we already checked the range */ + } break; case 2: arguments->command=mon_cmd_from_string(arg); if (arguments->command == MON_CMD_UNKNOWN) { - printf("\nUnknown command '%s'\n", arg); - argp_usage(state); + printf("\nUnknown command '%s'\n\n", arg); + err = 1; } break; default: if (arguments->n_options >= MAX_OPTIONS) { - printf("\nToo many command options given, limit is %d\n", MAX_OPTIONS); - argp_usage(state); + printf("\nToo many command options given, limit is %d\n\n", MAX_OPTIONS); + err = 1; + break; } arguments->options[arguments->n_options] = mon_opt_type_from_string(arg); if (arguments->options[arguments->n_options] == OPT_TYPE_UNKNOWN) { - printf("\nUnknown command option '%s'\n", arg); - argp_usage(state); + printf("\nUnknown command option '%s'\n\n", arg); + err = 1; } arguments->n_options++; break; @@ -127,7 +136,7 @@ static error_t parse_option(int key, char *arg, struct argp_state *state) case ARGP_KEY_END: /* no more arguments */ if (state->arg_num < 3) { /* not enough arguments encountered */ - argp_usage(state); + err = 1; } break; @@ -135,6 +144,11 @@ static error_t parse_option(int key, char *arg, struct argp_state *state) return ARGP_ERR_UNKNOWN; } + if (err) { + argp_usage(state); + return EINVAL; /* argp_usage() usually does not return, but just in case */ + } + return 0; /* success */ } diff --git a/tr/trpc_main.c b/tr/trpc_main.c index 039bf51..db29417 100644 --- a/tr/trpc_main.c +++ b/tr/trpc_main.c @@ -41,6 +41,7 @@ #include #include #include +#include /* command-line option setup */ @@ -92,7 +93,19 @@ static error_t parse_option(int key, char *arg, struct argp_state *state) break; case 2: - arguments->port=strtol(arg, NULL, 10); /* optional */ + arguments->port=tr_parse_port(arg); /* optional */ + if (arguments->port < 0) { + switch(-(arguments->port)) { + case ERANGE: + printf("\nError parsing port (%s): port must be an integer in the range 1 - 65535\n\n", arg); + break; + + default: + printf("\nError parsing port (%s): %s\n\n", arg, strerror(-arguments->port)); + break; + } + argp_usage(state); + } break; default: -- 2.1.4