brbzull0 opened a new pull request, #13634:
URL: https://github.com/apache/trafficserver/pull/13634
`RecLookupMatchingRecords()` compiles a caller-supplied pattern and runs it
against every record and metric name with no complexity bound. It backs the
`admin_lookup_records` JSONRPC method, which is served inline on the RPC
server's single thread, so a pattern that backtracks heavily stalls that
thread
for the duration.
Measured on a stock server carrying 1328 names: roughly 19s of CPU per
request
unbounded, against about 123ms with the limit in this change.
### Change
- `include/tsutil/Regex.h`: add `RE_ERROR_MATCHLIMIT` (-47) to `REErrors`,
alongside the existing `RE_ERROR_NOMATCH` and `RE_ERROR_NULL`.
- `src/tsutil/Regex.cc`: add the matching `static_assert` against
`PCRE2_ERROR_MATCHLIMIT`, following the pattern already used for the other
two.
- `src/records/RecCore.cc`: give all four `regex.exec()` call sites in
`RecLookupMatchingRecords()` a `RegexMatchContext` with a 50,000 step match
limit.
Exhausting the limit is not a match verdict, so those names are counted and
reported once per lookup via `Warning()` rather than silently dropped from
the
caller's results. Any other negative return keeps the existing
treat-as-no-match behaviour. The pattern itself is deliberately not echoed
into
the log.
On the choice of 50,000: raising the limit costs run time linearly, while the
longest name a pattern can still resolve grows only as its square root, since
proving no match costs about L^2/2 steps. 50,000 resolves names up to ~310
characters, 4.7x the 66 character longest name a stock build registers. The
1750
used elsewhere in the tree reaches only ~59 characters and cannot resolve
what
ships.
### Tests
**Unit** -- `src/records/unit_tests/test_RecLookupMatchingRecords.cc`, three
cases covering config records, metrics (counter and static string) and hidden
metrics. `test_records` goes from 317 assertions in 44 cases on master to
636 in
49.
**Autest** --
`tests/gold_tests/traffic_ctl/traffic_ctl_config_match_limit.test.py`
drives `traffic_ctl config match` with `^([a-z.]+)+[~=]` and asserts three
things: the command returns promptly with no matches, an ordinary lookup
afterwards still answers (so the RPC server was not left wedged), and the
diags
log carries the incomplete-results warning rather than silently returning a
short answer.
Two details in that pattern are load bearing, and are commented in the test.
The
tail must be a character class rather than a literal, because for an absent
literal PCRE2's required-code-unit check rejects each name up front and no
backtracking happens -- the test would then pass with or without the change.
And
it must be unsatisfiable rather than merely unlikely: an earlier `[0-9]\z`
tail
matched `proxy.config.ssl.TLSv1`, since lookups are case insensitive. The
commonly cited `(a+)+$` is useless here, as no real record name holds two
consecutive `a` characters to backtrack over.
Asserting on the warning rather than on elapsed time keeps the test
independent
of machine speed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
