Conversation
The range header lookup in bgFetchAllowed() had a misplaced closing paren: TSMimeHdrFieldFind(bufp, hdr_loc, header.data(), header.size() == TS_SUCCESS) TSMimeHdrFieldFind() returns a TSMLoc rather than a TSReturnCode, so the comparison belonged on the call, not on the length argument. Because TS_SUCCESS is 0, "header.size() == TS_SUCCESS" evaluated to false and the plugin searched for a zero length field name, which never matches. That left hasRangeHdrs false for every request and broke both options that depend on it: --range-req-only=true suppressed the background fetch even for real range requests, and --cache-range-req=false never suppressed anything. Pass the name length and test the returned handle, and release the handles the lookup hands back, which the broken call never produced. Also reject a Content-Length condition with no size value, the way background_fetch already does. Without the check an empty value leaves both the parsed and remaining views empty, the length comparison succeeds, and a "<= 0" rule is installed from a line that specified no size at all. Adds an autest covering both directions of the --range-req-only decision: a range request fills the cache, a plain request does not.
…ache state The fixed Date in the past against max-age=300 made every stored object immediately stale, so the cache-hit assertions could not pass; the origin supplies a current Date on its own. The non-range case also cannot be observed through cache state, because a plain cacheable response is stored by ordinary proxy caching whether or not the plugin background fills. Assert on the plugin's own logged decision instead, which is what distinguishes the two.
There was a problem hiding this comment.
🟡 Changes recommended
Additional regression coverage is needed for incomplete Content-Length rules and the --cache-range-req=false path.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes cache_fill range-request detection and incomplete Content-Length configuration handling.
Changes:
- Corrects MIME header lookup and releases ATS handles.
- Rejects missing
Content-Lengthvalues. - Adds range-only request coverage.
Review follow-ups: Add coverage for incomplete Content-Length rules and --cache-range-req=false.
File summaries
| File | Summary |
|---|---|
tests/gold_tests/pluginTest/cache_fill/cache_fill_range_req_only.test.py |
Tests range-only cache filling for range and plain requests. |
plugins/experimental/cache_fill/configs.cc |
Fixes header detection, handle cleanup, and configuration parsing. |
Review details
Suppressed comments (1)
tests/gold_tests/pluginTest/cache_fill/cache_fill_range_req_only.test.py:77
- This regression test exercises only
--range-req-only=true. The same header lookup also controls--cache-range-req=false, whose range-request path is part of the bug described by this change; without a case for that option, a regression in that branch can pass. Add a range-request case that verifies the plugin declines the fill when--cache-range-req=falseis set.
self.ts.Disk.remap_config.AddLines(
[
'map http://www.example.com/fill_on_range http://127.0.0.1:{}/fill_on_range'.format(self.server.Variables.Port) +
' @plugin=cache_fill.so @pparam=--range-req-only=true',
'map http://www.example.com/skip_when_plain http://127.0.0.1:{}/skip_when_plain'.format(
self.server.Variables.Port) + ' @plugin=cache_fill.so @pparam=--range-req-only=true',
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (cfg_value.empty()) { | ||
| TSError("[%s] missing Content-Length size value, skipping config value", PLUGIN_NAME); | ||
| continue; | ||
| } |
Group the range-option coverage with the existing cache_fill test rather than keeping a separate file, in its own ATS process: the existing one loads cache_fill globally with default options, and a global instance hooks every transaction, which would background fill these paths regardless of the per-remap options and make the negative cases vacuous. Add the --cache-range-req=false case. No test in the tree set either option before this, and the same range header lookup drives both, so that branch had no coverage. Under the broken lookup hasRangeHdrs was always false and the branch never ran, which makes it an independent check on the same fix.
|
Thanks — I added the While adding it I folded the range-option coverage into the existing On coverage for the incomplete |
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate issues remain in test isolation and malformed Content-Length coverage.
Review details
Suppressed comments (2)
plugins/experimental/cache_fill/configs.cc:174
- This new malformed
Content-Lengthrejection is not covered by the added cache_fill test: the new cases exercise only range-request options, while the existingContent-Lengthcoverage is for the separate background_fetch plugin. Please add a cache_fill config case such asContent-Length <and assert that it is skipped (and does not install a<= 0rule), so a regression in this guard or the subsequent parse cannot go unnoticed.
if (cfg_value.empty()) {
TSError("[%s] missing Content-Length size value, skipping config value", PLUGIN_NAME);
continue;
}
tests/gold_tests/pluginTest/cache_fill/cache_fill.test.py:284
- These are three remap instances in one traffic_server, but
BgFetchConfig::parseOptions()callsgetopt_long()without resetting the process-globaloptind. After the first--range-req-onlyinstance, the second and third instances will retain their defaults, so the negative decision assertions cannot be satisfied. Resetoptindbefore each parse or isolate each option in a separate ATS process.
'map http://www.example.com/skip_when_plain http://127.0.0.1:{}/skip_when_plain'.format(port) +
' @plugin=cache_fill.so @pparam=--range-req-only=true',
'map http://www.example.com/decline_on_range http://127.0.0.1:{}/decline_on_range'.format(port) +
' @plugin=cache_fill.so @pparam=--cache-range-req=false',
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Correcting my own description: I overstated the severity, and the original text was up for a few hours, so the correction is worth stating rather than quietly editing. I wrote that release builds had both options broken and that the header handle was leaking. Both are wrong.
What is actually wrong, and what this change fixes:
I have also been precise in the description about what the new test proves: it discriminates on an assertion-enabled build, which is what AuTest and CI use, but it would pass against the unfixed plugin in a release build, since there the defect is the leak rather than a behaviour change. The runs still earn their place because they pin two options that had no coverage at all. The diff itself is unchanged and still correct. |
bryancall
added
Backport
The bug
BgFetchConfig::bgFetchAllowed()decides whether a transaction is a rangerequest by looking for a
Rangeor conditional header. The lookup had amisplaced closing paren:
TSMimeHdrFieldFind()returns aTSMLoc, not aTSReturnCode, so the== TS_SUCCESSbelonged on the call rather than on the length argument.TS_SUCCESSis0, soheader.size() == TS_SUCCESSevaluated tofalse,which converted to a length of
0. Every lookup asked for a zero-length fieldname.
Detection still worked in a release build.
mime_hdr_field_find()beginswith
hdrtoken_is_wks(field_name.data()), which is a pointer-range test againstthe well-known-string heap and never consults the length.
FILTER_HEADERSholdsgenuine WKS pointers (
TS_MIME_FIELD_RANGEand friends), so the lookup took theWKS-index path and found the field despite the zero length. So the two options
behaved correctly in release; what they did not do is clean up.
The two real defects:
MIMEFieldSDKHandleper matching request.TSMimeHdrFieldFind()allocates a handle via
sdk_alloc_field_handle()for every hit. The call wasused in boolean context and the handle was dropped on the floor, so every
request carrying one of the six headers leaked one, on any build.
mime_hdr_field_find()also doesink_assert(!field_name.empty()), which is compiled out in release but fireson the first request through a remap rule that sets either option:
The fix
Pass the name length and test the returned handle, then release it. The
TSHandleMLocRelease()on the header mloc fromTSHttpTxnClientReqGet()iscontract hygiene rather than a leak fix:
TSHandleMLocRelease()returnsTS_SUCCESSimmediately for anHTTP_HEADERobject and frees nothing. It isthere because the API asks for it and because the equivalent code in
plugins/experimental/maxmind_acldoes the same.Also reject a
Content-Lengthcondition that carries no size value, the wayplugins/background_fetchalready does. Without it,++cfg_valueleaves theview empty,
swoc::svtou()setsparsedempty too, so theparsed.size() != cfg_value.size()guard compares0 != 0, passes, and a<= 0rule is installed from a config line that specified no size at all.Testing
Folded into the existing
tests/gold_tests/pluginTest/cache_fill/cache_fill.test.py,in its own ATS process. It needs a separate process rather than sharing the
existing one, because that one loads
cache_fillglobally with default options,and a global instance hooks every transaction -- it would background fill these
paths regardless of the per-remap options and make the negative cases vacuous.
Four runs, covering both options and both directions of the decision, so a fix
that simply always filled would not pass:
--range-req-only=true, range request: must fill (second request ishit-fresh/206/Content-Range)--range-req-only=true, plain request: must not fill--cache-range-req=false, range request: must decline the fillNo test in the tree set either option before this, so both branches of the
lookup were uncovered. The negative cases assert on the plugin's logged
decision rather than on cache state, because a plain cacheable response is
stored by ordinary proxy caching whether or not the plugin fills, so both
outcomes look identical from the client.
Verified it discriminates, by rebuilding the plugin with the paren restored and
re-running: the test fails on every one of those assertions, with the crash log
under
ts-rangeonly, leaving the default-options process unaffected. With thefix, 13 runs pass and no crash log is produced.
To be precise about what that proves: the test discriminates on a build with
assertions enabled, which is what the AuTest build and CI use. On a release
build it would pass against the unfixed plugin, because there the defect is the
leaked handle rather than a behaviour change. The runs are still worth having --
they pin the intended behaviour of two options that had no coverage at all.
The existing default-options coverage is untouched, and that is itself the
reason this survived:
_range_req_onlydefaults tofalseand_cache_range_reqtotrue, soif (_range_req_only || !_cache_range_req)is false under the default config and the whole block was never entered by any
existing test.
Affected branches
10.2.xand10.1.xboth carry the paren bug and lack the empty-value guard.9.2.xdoes not ship thecache_fillplugin.Neither defect was flagged by Coverity. They turned up while reading
plugins/background_fetchandplugins/experimental/cache_fillside by side,which is worth doing on its own --
cache_fillwas forked frombackground_fetchand the two have drifted.