From 9faebe34cdbf3fcc0e23936aefbbdfa493997b12 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Fri, 7 Jun 2019 19:00:37 +0200 Subject: [PATCH] MEDIUM: tools: improve time format error detection As reported in GH issue #109 and in discourse issue https://discourse.haproxy.org/t/haproxy-returns-408-or-504-error-when-timeout-client-value-is-every-25d the time parser doesn't error on overflows nor underflows. This is a recurring problem which additionally has the bad taste of taking a long time before hitting the user. This patch makes parse_time_err() return special error codes for overflows and underflows, and adds the control in the call places to report suitable errors depending on the requested unit. In practice, underflows are almost never returned as the parsing function takes care of rounding values up, so this might possibly happen on 64-bit overflows returning exactly zero after rounding though. It is not really possible to cut the patch into pieces as it changes the function's API, hence all callers. Tests were run on about every relevant part (cookie maxlife/maxidle, server inter, stats timeout, timeout*, cli's set timeout command, tcp-request/response inspect-delay). --- include/common/standard.h | 4 ++ src/arg.c | 5 ++- src/cfgparse-global.c | 32 ++++++++++---- src/cfgparse-listen.c | 59 +++++++++++++++++++++++--- src/cfgparse.c | 45 ++++++++++++++++---- src/cli.c | 12 +++++- src/flt_spoe.c | 14 ++++++- src/hlua.c | 12 +++++- src/proto_tcp.c | 24 ++++++++++- src/proxy.c | 24 ++++++++++- src/server.c | 87 ++++++++++++++++++++++++++++++++++++--- src/ssl_sock.c | 12 +++++- src/standard.c | 17 +++++--- src/stick_table.c | 18 +++++--- src/tcp_rules.c | 14 ++++++- 15 files changed, 332 insertions(+), 47 deletions(-) diff --git a/include/common/standard.h b/include/common/standard.h index be85bb5e7..0f4b1870e 100644 --- a/include/common/standard.h +++ b/include/common/standard.h @@ -737,6 +737,10 @@ extern time_t my_timegm(const struct tm *tm); extern const char *parse_time_err(const char *text, unsigned *ret, unsigned unit_flags); extern const char *parse_size_err(const char *text, unsigned *ret); +/* special return values for the time parser */ +#define PARSE_TIME_UNDER ((char *)1) +#define PARSE_TIME_OVER ((char *)2) + /* unit flags to pass to parse_time_err */ #define TIME_UNIT_US 0x0000 #define TIME_UNIT_MS 0x0001 diff --git a/src/arg.c b/src/arg.c index b0fe94544..2719b5395 100644 --- a/src/arg.c +++ b/src/arg.c @@ -222,8 +222,11 @@ int make_arg_list(const char *in, int len, uint64_t mask, struct arg **argp, goto empty_err; ptr_err = parse_time_err(word, &uint, TIME_UNIT_MS); - if (ptr_err) + if (ptr_err) { + if (ptr_err == PARSE_TIME_OVER || ptr_err == PARSE_TIME_UNDER) + ptr_err = word; goto parse_err; + } arg->data.sint = uint; arg->type = ARGT_SINT; break; diff --git a/src/cfgparse-global.c b/src/cfgparse-global.c index 8355a8f1b..4ecaff1ae 100644 --- a/src/cfgparse-global.c +++ b/src/cfgparse-global.c @@ -263,9 +263,21 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm) } res = parse_time_err(args[1], &idle, TIME_UNIT_MS); - if (res) { + if (res == PARSE_TIME_OVER) { + ha_alert("parsing [%s:%d]: timer overflow in argument <%s> to <%s>, maximum value is 65535 ms.\n", + file, linenum, args[1], args[0]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (res == PARSE_TIME_UNDER) { + ha_alert("parsing [%s:%d]: timer underflow in argument <%s> to <%s>, minimum non-null value is 1 ms.\n", + file, linenum, args[1], args[0]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (res) { ha_alert("parsing [%s:%d]: unexpected character '%c' in argument to <%s>.\n", - file, linenum, *res, args[0]); + file, linenum, *res, args[0]); err_code |= ERR_ALERT | ERR_FATAL; goto out; } @@ -942,15 +954,21 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm) } err = parse_time_err(args[1], &val, TIME_UNIT_MS); - if (err) { + if (err == PARSE_TIME_OVER) { + ha_alert("parsing [%s:%d]: timer overflow in argument <%s> to <%s>, maximum value is 2147483647 ms (~24.8 days).\n", + file, linenum, args[1], args[0]); + err_code |= ERR_ALERT | ERR_FATAL; + } + else if (err == PARSE_TIME_UNDER) { + ha_alert("parsing [%s:%d]: timer underflow in argument <%s> to <%s>, minimum non-null value is 1 ms.\n", + file, linenum, args[1], args[0]); + err_code |= ERR_ALERT | ERR_FATAL; + } + else if (err) { ha_alert("parsing [%s:%d]: unsupported character '%c' in '%s' (wants an integer delay).\n", file, linenum, *err, args[0]); err_code |= ERR_ALERT | ERR_FATAL; } global.max_spread_checks = val; - if (global.max_spread_checks < 0) { - ha_alert("parsing [%s:%d]: '%s' needs a positive delay in milliseconds.\n",file, linenum, args[0]); - err_code |= ERR_ALERT | ERR_FATAL; - } } else if (strcmp(args[0], "cpu-map") == 0) { /* map a process list to a CPU set */ diff --git a/src/cfgparse-listen.c b/src/cfgparse-listen.c index 7ffc0e094..d659779a8 100644 --- a/src/cfgparse-listen.c +++ b/src/cfgparse-listen.c @@ -1067,7 +1067,19 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) } res = parse_time_err(args[cur_arg + 1], &maxidle, TIME_UNIT_S); - if (res) { + if (res == PARSE_TIME_OVER) { + ha_alert("parsing [%s:%d]: timer overflow in argument <%s> to <%s>, maximum value is 2147483647 s (~68 years).\n", + file, linenum, args[cur_arg+1], args[cur_arg]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (res == PARSE_TIME_UNDER) { + ha_alert("parsing [%s:%d]: timer underflow in argument <%s> to <%s>, minimum non-null value is 1 s.\n", + file, linenum, args[cur_arg+1], args[cur_arg]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (res) { ha_alert("parsing [%s:%d]: unexpected character '%c' in argument to <%s>.\n", file, linenum, *res, args[cur_arg]); err_code |= ERR_ALERT | ERR_FATAL; @@ -1087,8 +1099,21 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) goto out; } + res = parse_time_err(args[cur_arg + 1], &maxlife, TIME_UNIT_S); - if (res) { + if (res == PARSE_TIME_OVER) { + ha_alert("parsing [%s:%d]: timer overflow in argument <%s> to <%s>, maximum value is 2147483647 s (~68 years).\n", + file, linenum, args[cur_arg+1], args[cur_arg]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (res == PARSE_TIME_UNDER) { + ha_alert("parsing [%s:%d]: timer underflow in argument <%s> to <%s>, minimum non-null value is 1 s.\n", + file, linenum, args[cur_arg+1], args[cur_arg]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (res) { ha_alert("parsing [%s:%d]: unexpected character '%c' in argument to <%s>.\n", file, linenum, *res, args[cur_arg]); err_code |= ERR_ALERT | ERR_FATAL; @@ -1932,8 +1957,20 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) unsigned interval; err = parse_time_err(args[2], &interval, TIME_UNIT_S); - if (err) { - ha_alert("parsing [%s:%d] : unexpected character '%c' in stats refresh interval.\n", + if (err == PARSE_TIME_OVER) { + ha_alert("parsing [%s:%d]: timer overflow in argument <%s> to stats refresh interval, maximum value is 2147483647 s (~68 years).\n", + file, linenum, args[2]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (err == PARSE_TIME_UNDER) { + ha_alert("parsing [%s:%d]: timer underflow in argument <%s> to stats refresh interval, minimum non-null value is 1 s.\n", + file, linenum, args[2]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (err) { + ha_alert("parsing [%s:%d]: unexpected character '%c' in argument to stats refresh interval.\n", file, linenum, *err); err_code |= ERR_ALERT | ERR_FATAL; goto out; @@ -3374,7 +3411,19 @@ stats_error_parsing: goto out; } err = parse_time_err(args[1], &val, TIME_UNIT_MS); - if (err) { + if (err == PARSE_TIME_OVER) { + ha_alert("parsing [%s:%d]: timer overflow in argument <%s> to grace time, maximum value is 2147483647 ms (~24.8 days).\n", + file, linenum, args[1]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (err == PARSE_TIME_UNDER) { + ha_alert("parsing [%s:%d]: timer underflow in argument <%s> to grace time, minimum non-null value is 1 ms.\n", + file, linenum, args[1]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (err) { ha_alert("parsing [%s:%d] : unexpected character '%c' in grace time.\n", file, linenum, *err); err_code |= ERR_ALERT | ERR_FATAL; diff --git a/src/cfgparse.c b/src/cfgparse.c index 98adadffa..1101470be 100644 --- a/src/cfgparse.c +++ b/src/cfgparse.c @@ -1203,7 +1203,19 @@ resolv_out: goto out; } res = parse_time_err(args[2], &time, TIME_UNIT_MS); - if (res) { + if (res == PARSE_TIME_OVER) { + ha_alert("parsing [%s:%d]: timer overflow in argument <%s> to <%s>, maximum value is 2147483647 ms (~24.8 days).\n", + file, linenum, args[1], args[0]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (res == PARSE_TIME_UNDER) { + ha_alert("parsing [%s:%d]: timer underflow in argument <%s> to <%s>, minimum non-null value is 1 ms.\n", + file, linenum, args[1], args[0]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (res) { ha_alert("parsing [%s:%d]: unexpected character '%c' in argument to <%s>.\n", file, linenum, *res, args[0]); err_code |= ERR_ALERT | ERR_FATAL; @@ -1283,7 +1295,19 @@ resolv_out: goto out; } res = parse_time_err(args[2], &tout, TIME_UNIT_MS); - if (res) { + if (res == PARSE_TIME_OVER) { + ha_alert("parsing [%s:%d]: timer overflow in argument <%s> to <%s %s>, maximum value is 2147483647 ms (~24.8 days).\n", + file, linenum, args[2], args[0], args[1]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (res == PARSE_TIME_UNDER) { + ha_alert("parsing [%s:%d]: timer underflow in argument <%s> to <%s %s>, minimum non-null value is 1 ms.\n", + file, linenum, args[2], args[0], args[1]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + else if (res) { ha_alert("parsing [%s:%d]: unexpected character '%c' in argument to <%s %s>.\n", file, linenum, *res, args[0], args[1]); err_code |= ERR_ALERT | ERR_FATAL; @@ -1459,14 +1483,21 @@ int cfg_parse_mailers(const char *file, int linenum, char **args, int kwm) goto out; } res = parse_time_err(args[2], &timeout_mail, TIME_UNIT_MS); - if (res) { - ha_alert("parsing [%s:%d]: unexpected character '%c' in argument to <%s>.\n", - file, linenum, *res, args[0]); + if (res == PARSE_TIME_OVER) { + ha_alert("parsing [%s:%d]: timer overflow in argument <%s> to <%s %s>, maximum value is 2147483647 ms (~24.8 days).\n", + file, linenum, args[2], args[0], args[1]); err_code |= ERR_ALERT | ERR_FATAL; goto out; } - if (timeout_mail <= 0) { - ha_alert("parsing [%s:%d] : '%s %s' expects a positive