bryancall opened a new pull request, #13704:
URL: https://github.com/apache/trafficserver/pull/13704
### The bug
`BgFetchConfig::bgFetchAllowed()` decides whether a transaction is a range
request by looking for a `Range` or conditional header. The lookup had a
misplaced closing paren:
```cpp
if (TSMimeHdrFieldFind(bufp, hdr_loc, header.data(), header.size() ==
TS_SUCCESS)) {
```
`TSMimeHdrFieldFind()` returns a `TSMLoc`, not a `TSReturnCode`, so the
`== TS_SUCCESS` belonged on the call rather than on the length argument.
`TS_SUCCESS` is `0`, so `header.size() == TS_SUCCESS` evaluated to `false`,
which converted to a length of `0`. The plugin was searching every request
for
a zero-length field name, which never matches.
A zero-length name is not merely a lookup that misses.
`mime_hdr_field_find()`
asserts on it:
```
Fatal: src/proxy/hdrs/MIME.cc:1224: failed assertion `!field_name.empty()`
traffic_server: received signal 6 (Aborted)
```
That is an `ink_assert`, which is compiled out in release builds, so the
behaviour split by build type:
- **debug builds abort** on the first request through a remap rule that sets
either option
- **release builds** carry on with `hasRangeHdrs` always `false`, leaving
both
options broken in opposite directions:
| Option | Intended | Actual (release) |
|---|---|---|
| `--range-req-only=true` | background fill only for range requests | never
filled, including for range requests |
| `--cache-range-req=false` | suppress the fill for range requests | never
suppressed anything |
### The fix
Pass the name length and test the returned handle. Since the broken call
could
only ever return `TS_NULL_MLOC`, it never had a handle to release; now that
the
lookup can succeed, the field handle is released, as is the header handle
from
`TSHttpTxnClientReqGet()`. `TSMimeHdrFieldFind()`'s contract in `ts.h` asks
for
the former; the latter was already leaking on every call into this block.
Also reject a `Content-Length` condition that carries no size value, the way
`plugins/background_fetch` already does. Without it, `++cfg_value` leaves the
view empty, `swoc::svtou()` sets `parsed` empty too, so the
`parsed.size() != cfg_value.size()` guard compares `0 != 0`, passes, and a
`<= 0` rule is installed from a config line that specified no size at all.
### Testing
New autest `cache_fill_range_req_only`, driving both directions of the
`--range-req-only=true` decision so that a fix which simply always filled
would
not pass:
- a range request must fill the cache (second request is `hit-fresh` / `206`)
- a plain request must not (stays a miss)
Verified it discriminates, by rebuilding the plugin with the paren restored
and
re-running: the test fails, and the autest build aborts on the assertion
above.
With the fix, `cache_fill` and `cache_fill_range_req_only` both pass and no
crash log is produced.
The existing `cache_fill` gold test is unaffected, and that is itself the
reason this survived: `_range_req_only` defaults to `false` and
`_cache_range_req` to `true`, so `if (_range_req_only || !_cache_range_req)`
is false under the default config and the whole block was never entered by
any
existing test.
Neither of these was flagged by Coverity. They turned up while reading
`plugins/background_fetch` and `plugins/experimental/cache_fill` side by
side,
which is worth doing on its own -- `cache_fill` was forked from
`background_fetch` and the two have drifted.
--
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]
