From: Alan T. DeKok Date: Sun, 6 Apr 2025 01:40:21 +0000 (-0400) Subject: simplify parsing of time_delta X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0e89cb7ee06907b1a512f60526bbfa893546efb4;p=thirdparty%2Ffreeradius-server.git simplify parsing of time_delta the old code parsed fractions incorrectly. Rather than trying to debug and fix it, we just change the code to parse floating point numbers. --- diff --git a/src/lib/util/time.c b/src/lib/util/time.c index 6c8e72e674a..db400c2fbfb 100644 --- a/src/lib/util/time.c +++ b/src/lib/util/time.c @@ -216,15 +216,22 @@ fr_slen_t fr_time_delta_from_substr(fr_time_delta_t *out, fr_sbuff_t *in, fr_tim { fr_sbuff_t our_in = FR_SBUFF(in); int64_t integer; /* Whole units */ + double f; fr_time_res_t res; + bool do_float; bool negative; fr_sbuff_parse_error_t sberr; bool overflow; - fr_time_delta_t delta; /* The delta we're building */ size_t match_len; negative = fr_sbuff_is_char(&our_in, '-'); + do_float = false; + /* + * Look for: + * + * [scale] + */ if (fr_sbuff_out(&sberr, &integer, &our_in) < 0) { char const *err; @@ -232,147 +239,97 @@ fr_slen_t fr_time_delta_from_substr(fr_time_delta_t *out, fr_sbuff_t *in, fr_tim if (sberr != FR_SBUFF_PARSE_ERROR_NOT_FOUND) { err = fr_table_str_by_value(sbuff_parse_error_table, sberr, ""); } else { - err = "Invalid text, input should be an integer"; + err = "Invalid text, input should be a number"; } fr_strerror_printf("Failed parsing time_delta: %s", err); FR_SBUFF_ERROR_RETURN(&our_in); } - fr_sbuff_out_by_longest_prefix(&match_len, &res, fr_time_precision_table, &our_in, FR_TIME_RES_INVALID); /* - * We now determine which one of the three formats - * we accept the string is in. - * - * Either: - * - [] - * - .[] - * - [hours:]minutes:seconds + * hh:mm:ss */ + if (fr_sbuff_next_if_char(&our_in, ':')) goto do_timestamp; /* - * We have a fractional component + * If it's a fractional thing, then just parse it as a double. * - * .[] + * [scale] */ - if (fr_sbuff_next_if_char(&our_in, '.')) { - fr_sbuff_marker_t m_f; - size_t f_len; - uint64_t f = 0; /* Fractional units */ + if (fr_sbuff_is_char(&our_in, '.')) { + our_in = FR_SBUFF(in); - /* - * Normalise as a positive integer - */ - if (negative) integer = -(integer); + if (fr_sbuff_out(&sberr, &f, &our_in) < 0) goto num_error; - /* - * Mark the start of the fractional component - */ - fr_sbuff_marker(&m_f, &our_in); + do_float = true; + } - /* - * Leading zeros appear to mess up integer parsing - */ - fr_sbuff_adv_past_zeros(&our_in, SIZE_MAX, tt); + /* + * Now look for the time resolution. + */ + fr_sbuff_out_by_longest_prefix(&match_len, &res, fr_time_precision_table, &our_in, FR_TIME_RES_INVALID); - if (fr_sbuff_out(&sberr, &f, &our_in) < 0) { - /* - * Crappy workaround for .0 - * - * Advancing past the leading zeros screws - * up the fractional parsing when the - * fraction is all zeros... - */ - if ((sberr != FR_SBUFF_PARSE_ERROR_NOT_FOUND) || !fr_sbuff_is_char(&m_f, '0')) goto num_error; - } + if (fr_sbuff_is_terminal(&our_in, tt)) { + if (match_len == 0) res = hint; - f_len = fr_sbuff_behind(&m_f); - if (f_len > 9) { - fr_strerror_const("Too much precision for time_delta"); - fr_sbuff_set(&our_in, fr_sbuff_current(&m_f) + 10); + } else if (no_trailing) { + fail_trailing_data: + /* Got a qualifier but there is more text after it. */ + if (res != FR_TIME_RES_INVALID) { + fr_strerror_const("Trailing data after time_delta"); FR_SBUFF_ERROR_RETURN(&our_in); } - /* - * Convert to nanoseconds - * - * This can't overflow. - */ - while (f_len < 9) { -#ifdef STATIC_ANALYZER - // Coverity doesn't understand that the previous conditions prevent overflow. - if (f > UINT64_MAX / 10) break; -#endif - f *= 10; - f_len++; - } + fr_strerror_const("Invalid precision qualifier for time_delta"); + FR_SBUFF_ERROR_RETURN(&our_in); + } else if (match_len == 0) { /* - * Look for a scale suffix + * There is trailing data, but we don't care about it. Ensure that we have a time resolution. */ - fr_sbuff_out_by_longest_prefix(&match_len, &res, fr_time_precision_table, &our_in, FR_TIME_RES_INVALID); - - if (no_trailing && !fr_sbuff_is_terminal(&our_in, tt)) { - trailing_data: - /* Got a qualifier but there's stuff after */ - if (res != FR_TIME_RES_INVALID) { - fr_strerror_const("Trailing data after time_delta"); - FR_SBUFF_ERROR_RETURN(&our_in); - } - - fr_strerror_const("Invalid precision qualifier for time_delta"); - FR_SBUFF_ERROR_RETURN(&our_in); - } + res = hint; + } - /* Scale defaults to hint */ - if (res == FR_TIME_RES_INVALID) res = hint; + fr_assert(res != FR_TIME_RES_INVALID); - /* - * Subseconds was parsed as if it was nanoseconds. - * But instead it may be something else, so it should - * be truncated. - * - * Note that this operation can't overflow. - */ + /* + * For floating point numbers, we pre-multiply by the time resolution, and then override the time + * resolution to indicate that no further scaling is necessary. + */ + if (do_float) { f *= fr_time_multiplier_by_res[res]; - f /= NSEC; + if (f < (double) INT64_MIN) goto fail_overflow; + if (f > (double) INT64_MAX) goto fail_overflow; - delta = fr_time_delta_from_integer(&overflow, integer, res); - if (overflow) { - overflow: - fr_strerror_printf("time_delta would %s", negative ? "underflow" : "overflow"); - fr_sbuff_set_to_start(&our_in); - FR_SBUFF_ERROR_RETURN(&our_in); - } - - { - int64_t tmp; + res = FR_TIME_RES_NSEC; + integer = f; + } - /* - * Add fractional and integral parts checking for overflow - */ - if (!fr_add(&tmp, fr_time_delta_unwrap(delta), f)) goto overflow; + /* + * We have a valid time scale. Let's use that. + */ + *out = fr_time_delta_from_integer(&overflow, integer, res); + if (overflow) { + fail_overflow: + fr_strerror_printf("time_delta would %s", negative ? "underflow" : "overflow"); + fr_sbuff_set_to_start(&our_in); + FR_SBUFF_ERROR_RETURN(&our_in); - /* - * Flip the sign back to negative - */ - if (negative) tmp = -(tmp); + } + FR_SBUFF_SET_RETURN(in, &our_in); - *out = fr_time_delta_wrap(tmp); - } +do_timestamp: + res = hint; - FR_SBUFF_SET_RETURN(in, &our_in); /* - * It's timestamp format + * It's a timestamp format * * [hours:]minutes:seconds */ - } else if (fr_sbuff_next_if_char(&our_in, ':')) { + { uint64_t hours, minutes, seconds; fr_sbuff_marker_t m1; - res = FR_TIME_RES_SEC; - fr_sbuff_marker(&m1, &our_in); if (fr_sbuff_out(&sberr, &seconds, &our_in) < 0) goto num_error; @@ -384,63 +341,56 @@ fr_slen_t fr_time_delta_from_substr(fr_time_delta_t *out, fr_sbuff_t *in, fr_tim hours = 0; minutes = negative ? -(integer) : integer; - if (minutes > UINT16_MAX) { + if (minutes > 60) { fr_strerror_printf("minutes component of time_delta is too large"); fr_sbuff_set_to_start(&our_in); FR_SBUFF_ERROR_RETURN(&our_in); } - /* - * hours:minutes:seconds - */ + } else { + /* + * hours:minutes:seconds + */ hours = negative ? -(integer) : integer; minutes = seconds; if (fr_sbuff_out(&sberr, &seconds, &our_in) < 0) goto num_error; + /* + * We allow >24 hours. What the heck. + */ if (hours > UINT16_MAX) { fr_strerror_printf("hours component of time_delta is too large"); fr_sbuff_set_to_start(&our_in); FR_SBUFF_ERROR_RETURN(&our_in); } - if (minutes > UINT16_MAX) { - fr_strerror_printf("minutes component of time_delta is too large"); + if (minutes > 60) { + fr_strerror_printf("minuts component of time_delta is too large"); + FR_SBUFF_ERROR_RETURN(&m1); + } + + if (seconds > 60) { + fr_strerror_printf("seconds component of time_delta is too large"); FR_SBUFF_ERROR_RETURN(&m1); } } - if (no_trailing && !fr_sbuff_is_terminal(&our_in, tt)) goto trailing_data; + if (no_trailing && !fr_sbuff_is_terminal(&our_in, tt)) goto fail_trailing_data; /* * Add all the components together... */ - if (!fr_add(&integer, ((hours * 60) * 60) + (minutes * 60), seconds)) goto overflow; + if (!fr_add(&integer, ((hours * 60) * 60) + (minutes * 60), seconds)) goto fail_overflow; /* * Flip the sign back to negative */ if (negative) integer = -(integer); - - *out = fr_time_delta_from_sec(integer); - FR_SBUFF_SET_RETURN(in, &our_in); - /* - * Nothing fancy here it's just a time delta as an integer - * - * [] - */ - } else { - if (no_trailing && !fr_sbuff_is_terminal(&our_in, tt)) goto trailing_data; - - /* Scale defaults to hint */ - if (res == FR_TIME_RES_INVALID) res = hint; - - /* Do the scale conversion */ - *out = fr_time_delta_from_integer(&overflow, integer, res); - if (overflow) goto overflow; - - FR_SBUFF_SET_RETURN(in, &our_in); } + + *out = fr_time_delta_from_sec(integer); + FR_SBUFF_SET_RETURN(in, &our_in); } /** Create fr_time_delta_t from a string diff --git a/src/tests/unit/data_types.txt b/src/tests/unit/data_types.txt index b956b244999..a46152cd166 100644 --- a/src/tests/unit/data_types.txt +++ b/src/tests/unit/data_types.txt @@ -32,7 +32,7 @@ value time_delta 2.4 match 2.4 value time_delta yes -match Failed parsing time_delta: Invalid text, input should be an integer +match Failed parsing time_delta: Invalid text, input should be a number value time_delta 1ms match 0.001 @@ -61,6 +61,9 @@ match 3600 value time_delta 1d match 86400 +value time_delta 1.14285714w +match 691199.998272 + # # And negative numbers # @@ -247,4 +250,4 @@ encode-dns-label www_foo.com match Invalid character 0x5f in label count -match 122 +match 124