Skip to content

Fix wrong checks for some integer config records #12643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix wrong checks for some integer config records #12643

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e6e1f06
Fix wrong checks for some integer config records
moonchen Nov 5, 2025
681ddb3
Add support for negative ranges
moonchen Nov 5, 2025
220a2fe
Dodge printf portability issue
moonchen Nov 6, 2025
3fc1bb6
Apply suggestions from code review
moonchen Nov 6, 2025
1fcab70
Merge branch 'master' into fix-rec-check-types
moonchen Nov 9, 2025
6a2c978
Refine parameter documentation in parse_int function
moonchen Nov 9, 2025
a7b084e
Formatting
moonchen Nov 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/records/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ target_link_libraries(
)

if(BUILD_TESTING)
add_executable(test_records unit_tests/unit_test_main.cc unit_tests/test_RecHttp.cc)
add_executable(test_records unit_tests/unit_test_main.cc unit_tests/test_RecHttp.cc unit_tests/test_RecUtils.cc)
target_link_libraries(test_records PRIVATE records Catch2::Catch2 ts::tscore libswoc::libswoc)
add_catch2_test(NAME test_records COMMAND test_records)
endif()
Expand Down
58 changes: 42 additions & 16 deletions src/records/RecUtils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@
#include "records/RecordsConfig.h"
#include "P_RecUtils.h"
#include "P_RecCore.h"

#include <charconv>
#include <string_view>
//-------------------------------------------------------------------------
// RecRecord initializer / Free
//-------------------------------------------------------------------------
Expand Down Expand Up @@ -387,26 +390,49 @@ recordRegexCheck(const char *pattern, const char *value)
return regex.compile(pattern) && regex.exec(value);
}

// Helper to parse integer from string_view using std::from_chars
// Requires the ENTIRE string to be a valid integer (strict parsing)
bool
parse_int(std::string_view sv, int &out)
{
auto [ptr, ec] = std::from_chars(sv.data(), sv.data() + sv.size(), out);
// Success only if no error AND entire string was consumed
return ec == std::errc{} && ptr == sv.data() + sv.size();
}

bool
recordRangeCheck(const char *pattern, const char *value)
{
char *p = const_cast<char *>(pattern);
Tokenizer dashTok("-");

if (recordRegexCheck("^[0-9]+$", value)) {
while (*p != '[') {
p++;
} // skip to '['
if (dashTok.Initialize(++p, COPY_TOKS) == 2) {
int l_limit = atoi(dashTok[0]);
int u_limit = atoi(dashTok[1]);
int val = atoi(value);
if (val >= l_limit && val <= u_limit) {
return true;
}
}
std::string_view sv_pattern(pattern);

// Find '[' and ']'
auto start = sv_pattern.find('[');
if (start == std::string_view::npos) {
return false; // No '[' found
}
return false;

auto end = sv_pattern.find(']', start);
if (end == std::string_view::npos) {
return false; // No ']' found
}

// Extract range portion between brackets: "[0-10]" -> "0-10"
std::string_view range = sv_pattern.substr(start + 1, end - start - 1);

// Find the dash separator
auto dash_pos = range.find('-');
if (dash_pos == std::string_view::npos) {
return false; // No dash found
}
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation will incorrectly find the first dash in patterns with negative numbers like [-10-0], resulting in parsing an empty string before the dash as the lower bound. While the test correctly validates that negative ranges should be rejected, this bug could cause silent failures or unexpected behavior. Consider adding a comment explaining that negative ranges are not supported, or add an explicit check to reject patterns starting with - after the opening bracket.

Suggested change
// Find the dash separator
auto dash_pos = range.find('-');
if (dash_pos == std::string_view::npos) {
return false; // No dash found
}
// Negative ranges are not supported. Explicitly reject if lower or upper bound starts with '-'.
auto dash_pos = range.find('-');
if (dash_pos == std::string_view::npos) {
return false; // No dash found
}
if (range.size() > 0 && range[0] == '-') {
return false; // Lower bound is negative, not supported
}
if (dash_pos + 1 < range.size() && range[dash_pos + 1] == '-') {
return false; // Upper bound is negative, not supported
}

Copilot uses AI. Check for mistakes.

// Parse the three integers: lower bound, upper bound, and value
int l_limit, u_limit, val;
if (!parse_int(range.substr(0, dash_pos), l_limit) || !parse_int(range.substr(dash_pos + 1), u_limit) ||
!parse_int(std::string_view(value), val)) {
return false; // Parse error
}

return (val >= l_limit && val <= u_limit);
}

bool
Expand Down
108 changes: 94 additions & 14 deletions src/records/RecordsConfig.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
#include "tscore/Filenames.h"
#include "records/RecordsConfig.h"

#include <string_view>
#include <cstddef>

#if TS_USE_REMOTE_UNWINDING
#define MGMT_CRASHLOG_HELPER "traffic_crashlog"
#else
Expand Down Expand Up @@ -72,11 +75,11 @@ static constexpr RecordElement RecordsConfig[] =
{RECT_CONFIG, "proxy.config.dump_mem_info_frequency", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
,
//# 0 == CLOCK_REALTIME, change carefully
{RECT_CONFIG, "proxy.config.system_clock", RECD_INT, "0", RECU_RESTART_TS, RR_NULL, RECC_INT, "[0-9]+", RECA_NULL}
{RECT_CONFIG, "proxy.config.system_clock", RECD_INT, "0", RECU_RESTART_TS, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
{RECT_CONFIG, "proxy.config.cache.max_disk_errors", RECD_INT, "5", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
,
{RECT_CONFIG, "proxy.config.cache.persist_bad_disks", RECD_INT, "0", RECU_RESTART_TS, RR_NULL, RECC_INT, "[01]", RECA_NULL}
{RECT_CONFIG, "proxy.config.cache.persist_bad_disks", RECD_INT, "0", RECU_RESTART_TS, RR_NULL, RECC_INT, "[0-1]", RECA_NULL}
,
{RECT_CONFIG, "proxy.config.output.logfile.name", RECD_STRING, "traffic.out", RECU_RESTART_TS, RR_REQUIRED, RECC_NULL, nullptr,
RECA_NULL}
Expand Down Expand Up @@ -204,7 +207,7 @@ static constexpr RecordElement RecordsConfig[] =
,
{RECT_CONFIG, "proxy.config.diags.debug.tags", RECD_STRING, "http|dns", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
,
{RECT_CONFIG, "proxy.config.diags.debug.throttling_interval_msec", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_INT, "^[0-9]+$", RECA_NULL}
{RECT_CONFIG, "proxy.config.diags.debug.throttling_interval_msec", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
{RECT_CONFIG, "proxy.config.diags.debug.client_ip", RECD_STRING, nullptr, RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
,
Expand Down Expand Up @@ -409,7 +412,7 @@ static constexpr RecordElement RecordsConfig[] =
,
{RECT_CONFIG, "proxy.config.http.auth_server_session_private", RECD_INT, "1", RECU_DYNAMIC, RR_NULL, RECC_INT, "[0-2]", RECA_NULL}
,
{RECT_CONFIG, "proxy.config.http.max_post_size", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_INT, "^[0-9]+$", RECA_NULL}
{RECT_CONFIG, "proxy.config.http.max_post_size", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
// ##############################
// # parent proxy configuration #
Expand Down Expand Up @@ -1075,9 +1078,9 @@ static constexpr RecordElement RecordsConfig[] =
{RECT_CONFIG, "proxy.config.log.max_line_size", RECD_INT, "9216", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
,
// How often periodic tasks get executed in the Log.cc infrastructure
{RECT_CONFIG, "proxy.config.log.periodic_tasks_interval", RECD_INT, "5", RECU_DYNAMIC, RR_NULL, RECC_INT, "^[0-9]+$", RECA_NULL}
{RECT_CONFIG, "proxy.config.log.periodic_tasks_interval", RECD_INT, "5", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
{RECT_CONFIG, "proxy.config.log.throttling_interval_msec", RECD_INT, "60000", RECU_DYNAMIC, RR_NULL, RECC_INT, "^[0-9]+$", RECA_NULL}
{RECT_CONFIG, "proxy.config.log.throttling_interval_msec", RECD_INT, "60000", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,

//##############################################################################
Expand All @@ -1097,7 +1100,7 @@ static constexpr RecordElement RecordsConfig[] =
,
{RECT_CONFIG, "proxy.config.plugin.dynamic_reload_mode", RECD_INT, "1", RECU_RESTART_TS, RR_NULL, RECC_INT, "[0-1]", RECA_NULL}
,
{RECT_CONFIG, "proxy.config.url_remap.min_rules_required", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_INT, "[0-9]+", RECA_NULL}
{RECT_CONFIG, "proxy.config.url_remap.min_rules_required", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
{RECT_CONFIG, "proxy.config.url_remap.acl_behavior_policy", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_INT, "[0-1]", RECA_NULL}
,
Expand Down Expand Up @@ -1244,16 +1247,16 @@ static constexpr RecordElement RecordsConfig[] =
{RECT_CONFIG, "proxy.config.ssl.ocsp.enabled", RECD_INT, "0", RECU_RESTART_TS, RR_NULL, RECC_INT, "[0-1]", RECA_NULL}
,
// # Number of seconds before an OCSP response expires in the stapling cache. 3600s (1 hour) by default.
{RECT_CONFIG, "proxy.config.ssl.ocsp.cache_timeout", RECD_INT, "3600", RECU_DYNAMIC, RR_NULL, RECC_INT, "^[0-9]+$", RECA_NULL}
{RECT_CONFIG, "proxy.config.ssl.ocsp.cache_timeout", RECD_INT, "3600", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
// # Request method "mode" for queries to OCSP responders; 0 is POST, 1 is "prefer GET."
{RECT_CONFIG, "proxy.config.ssl.ocsp.request_mode", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_INT, "[0-1]", RECA_NULL}
,
// # Timeout for queries to OCSP responders. 10s by default.
{RECT_CONFIG, "proxy.config.ssl.ocsp.request_timeout", RECD_INT, "10", RECU_DYNAMIC, RR_NULL, RECC_INT, "^[0-9]+$", RECA_NULL}
{RECT_CONFIG, "proxy.config.ssl.ocsp.request_timeout", RECD_INT, "10", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
// # Update period for stapling caches. 60s (1 min) by default.
{RECT_CONFIG, "proxy.config.ssl.ocsp.update_period", RECD_INT, "60", RECU_DYNAMIC, RR_NULL, RECC_INT, "^[0-9]+$", RECA_NULL}
{RECT_CONFIG, "proxy.config.ssl.ocsp.update_period", RECD_INT, "60", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
// # Base path for OCSP prefetched responses
{RECT_CONFIG, "proxy.config.ssl.ocsp.response.path", RECD_STRING, TS_BUILD_SYSCONFDIR, RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
Expand Down Expand Up @@ -1465,13 +1468,13 @@ static constexpr RecordElement RecordsConfig[] =
,
{RECT_CONFIG, "proxy.config.quic.disable_active_migration", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "[0-1]", RECA_NULL}
,
{RECT_CONFIG, "proxy.config.quic.max_recv_udp_payload_size_in", RECD_INT, "65527", RECU_DYNAMIC, RR_NULL, RECC_INT, "^[0-9]+$", RECA_NULL}
{RECT_CONFIG, "proxy.config.quic.max_recv_udp_payload_size_in", RECD_INT, "65527", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
{RECT_CONFIG, "proxy.config.quic.max_recv_udp_payload_size_out", RECD_INT, "65527", RECU_DYNAMIC, RR_NULL, RECC_INT, "^[0-9]+$", RECA_NULL}
{RECT_CONFIG, "proxy.config.quic.max_recv_udp_payload_size_out", RECD_INT, "65527", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
{RECT_CONFIG, "proxy.config.quic.max_send_udp_payload_size_in", RECD_INT, "65527", RECU_DYNAMIC, RR_NULL, RECC_INT, "^[0-9]+$", RECA_NULL}
{RECT_CONFIG, "proxy.config.quic.max_send_udp_payload_size_in", RECD_INT, "65527", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
{RECT_CONFIG, "proxy.config.quic.max_send_udp_payload_size_out", RECD_INT, "65527", RECU_DYNAMIC, RR_NULL, RECC_INT, "^[0-9]+$", RECA_NULL}
{RECT_CONFIG, "proxy.config.quic.max_send_udp_payload_size_out", RECD_INT, "65527", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
{RECT_CONFIG, "proxy.config.quic.disable_http_0_9", RECD_INT, "1", RECU_DYNAMIC, RR_NULL, RECC_STR, "[0-1]", RECA_NULL}
,
Expand Down Expand Up @@ -1565,6 +1568,79 @@ validate_check_type_has_regex()
return true;
}

//-------------------------------------------------------------------------
// Compile-time validation helpers for RECC_INT patterns
//-------------------------------------------------------------------------

namespace
{
constexpr bool
is_digit(char c)
{
return c >= '0' && c <= '9';
}

// TODO: C++23 has std::from_chars for constexpr integer parsing
constexpr bool
parse_uint(std::string_view s, std::size_t &i)
{
std::size_t start = i;
while (i < s.size() && is_digit(s[i])) {
++i;
}
return i > start; // at least one digit
}

constexpr bool
matches_bracketed_int_range(std::string_view s)
{
std::size_t i = 0;
if (i >= s.size() || s[i++] != '[') {
return false;
}
if (!parse_uint(s, i)) {
return false;
}
if (i >= s.size() || s[i++] != '-') {
return false;
}
if (!parse_uint(s, i)) {
return false;
}
if (i >= s.size() || s[i++] != ']') {
return false;
}
return i == s.size(); // must end exactly here
}

// For string literals: deduces N and strips the trailing '\0'
template <std::size_t N>
consteval bool
matches_bracketed_int_range(const char (&lit)[N])
{
// N includes the null terminator
return matches_bracketed_int_range(std::string_view{lit, N - 1});
}

// Validate all RECC_INT entries in the RecordsConfig array at compile time
template <std::size_t N>
consteval bool
validate_recc_int_patterns(const RecordElement (&config)[N])
{
for (std::size_t i = 0; i < N; ++i) {
if (config[i].check == RECC_INT) {
if (config[i].regex == nullptr) {
return false; // RECC_INT must have a pattern
}
if (!matches_bracketed_int_range(config[i].regex)) {
return false; // Pattern must match [lower-upper] format
}
}
}
return true;
}
} // namespace

static_assert(validate_regex_has_check_type(),
"RecordsConfig validation failed: found RecordElement with regex pattern but no check type (RECC_NULL). "
"Specify appropriate check type like RECC_INT, RECC_STRING, etc.");
Expand All @@ -1573,6 +1649,10 @@ static_assert(validate_check_type_has_regex(),
"RecordsConfig validation failed: found RecordElement with check type but no regex pattern. "
"Provide a regex pattern to validate the input or use RECC_NULL if no validation is needed.");

static_assert(validate_recc_int_patterns(RecordsConfig),
"RecordsConfig validation failed: found RECC_INT with invalid pattern. "
"RECC_INT patterns must be in format [lower-upper] with numeric bounds, e.g., \"[0-2]\" or \"[1-256]\".");

void
RecordsConfigIterate(RecordElementCallback callback, void *data)
{
Expand Down
Loading