masaori335 opened a new issue, #13647:
URL: https://github.com/apache/trafficserver/issues/13647
## 1. The issue
`cripts::Header::operator[]` (`src/cripts/Headers.cc:227-246`) returns the
value of the **first** field line only. RFC 9110 §5.3 makes repeated field
lines semantically one comma-joined value, and `header_rewrite` implements
exactly that (`plugins/header_rewrite/conditions.cc:286-298`, walks
`TSMimeHdrFieldNextDup`, joins on a bare `,`). So the two disagree on any
duplicated header — `Cache-Control`, `Via`, `Accept-Encoding`, `Warning`, and
anything a client chose to split.
The mechanism: `TSMimeHdrFieldValueStringGet(..., idx = -1)` reaches
`TSMimeFieldValueGet`, whose `idx < 0` branch is
`handle->field_ptr->value_get()` (`src/api/InkAPI.cc:1655-1666`) — that one
field line, not the dup chain.
**This is not a subtly wrong value, it's an inverted branch.** Client sends
the field twice:
```
X-Check: yes
X-Check: no
```
against `if (req["X-Check"] == "yes") { req.Erase("X-Check"); }`:
| Read behaviour | Sees | Branch taken | Result |
| --- | --- | --- | --- |
| joined (`header_rewrite`) | `yes,no` | no | both lines reach the origin |
| first-field-only (Cripts) | `yes` | **yes** | every line destroyed, header
**absent** |
### Why this looks like an oversight rather than a decision
The same file walks duplicates correctly in all three write paths and in
neither read path:
| `src/cripts/Headers.cc` | Walks `NextDup`? | Behaviour |
| --- | --- | --- |
| `:113-164` `operator=(string_view)` | yes | replaces the first line,
destroys the rest |
| `:166-199` `operator=(integer)` | yes | same |
| `:118-128` empty-assign / `Erase` | yes | destroys every line |
| `:227-246` `operator[]` | **no** | reads line one, ignores the rest |
| `:248-264` `AsDate` | **no** | reads line one (defensible — comma-joining
dates is nonsense) |
Stronger still: `operator+=` (`:202-225`) **creates** a second field line
rather than extending the value. Cripts can therefore write a header it cannot
read back — `h["X"] = "a"; h["X"] += "b";` then reads as `a`. An implementation
that deliberately treated headers as single-valued would not do that.
### Adjacent defects in the same code, found while investigating
Listing these because any fix touches the same class, and two are
memory-safety issues:
| Where | Problem |
| --- | --- |
| `include/cripts/Headers.hpp:151-157` | `Header::String` has a destructor
that releases `_field_loc`, but no rule-of-three. The implicit copy
ctor/copy-assign duplicate the `TSMLoc`, so `h["A"] = h["B"]` (which binds the
implicit copy-assign, not the `string_view` overload) double-releases →
`sdk_free_field_handle` → `THREAD_FREE` twice on a proxy-allocator handle
(`src/api/InkAPI.cc:863-870`, `:197-202`). `operator[]` itself is safe only
because NRVO fires. |
| `src/cripts/Headers.cc:209-211` | `operator+=` releases `_field_loc`
without nulling it; `TSMimeHdrFieldCreateNamed` leaves `*locp` untouched on its
`!isWriteable` early return (`src/api/InkAPI.cc:1904-1906`), so on a
non-writeable heap `~String` releases the stale handle again. |
| `src/cripts/Headers.cc:285-328` | `begin()`/`iterate()` use
`TSMimeHdrFieldNext`, which steps the flat field list including duplicates
(`src/api/InkAPI.cc:2038-2055`). The documented `for (auto h : req) CDebug("{}:
{}", h, req[h])` idiom therefore prints a duplicated header's *first* value
once per line. |
| `src/cripts/Headers.cc:113-199` | After any assignment the proxy's
`_field_loc` ends up `nullptr` and its cached value is stale, so
read-after-write on the same proxy is wrong. |
## 2. Proposed fix
**Make `operator[]` return the joined value, and expose the individual field
lines.** Joining alone is not enough: `Set-Cookie` is explicitly exempted from
list semantics by RFC 9110 §5.3 (its values contain commas and cannot be split
back), so scripts need per-line access as a first-class API, not a workaround.
```cpp
req["Accept-Encoding"]; // "gzip,br" (was "gzip")
req["Accept-Encoding"].Count(); // 2 (0 when absent)
req["Accept-Encoding"].Values(); // {"gzip", "br"}
```
### Where the joined buffer lives — the one real design question
`Header::String` is a `StringViewMixin` holding a `string_view` and no owned
buffer, and `operator[]` returns it by value. A joined value needs backing
store somewhere:
| Option | Cost |
| --- | --- |
| **A. Own a `std::string` in the proxy** | `StringViewMixin::operator
string_view()` is implicit (`include/cripts/Lulu.hpp:162`), so
`cripts::string_view v = req["Cache-Control"];` would dangle at the end of the
full expression — **but only when the header is duplicated**. Silently fine in
test, wrong in production. Trades a wrong-value bug for a use-after-free on the
same trigger. |
| **B. Arena on the owning `Header`** ✅ | Cleared in `Header::Reset()`,
which `Context::reset()` (`src/cripts/Context.cc:33`) already calls between
hooks — i.e. exactly the lifetime a header view has today. No new dangling
class. Must be node-stable (`std::list`), since `vector<std::string>`
reallocation moves SSO strings and recreates A's bug. Allocates only when
`TSMimeHdrFieldNextDup != nullptr`, so the single-line path is unchanged. |
| **C. Leave `operator[]`, add `Joined()`** | No behaviour change, but the
default stays wrong and every caller must know to avoid `[]`. |
Proposing **B**.
### Prerequisite
Delete `Header::String`'s copy/move and build it as a prvalue from a private
ctor (guaranteed elision), plus a real `operator=(const String&)` that assigns
the *value*. Without this, adding a buffer turns the latent double-free above
into an easy one. This also fixes `h["A"] = h["B"]`, which is currently a
memory bug.
### Scope questions for discussion
1. **The iterator.** Once reads are joined, should iteration deduplicate
names, or keep yielding one entry per field line? Keeping it is defensible
("duplicates stay visible"), but it makes the documented print-all-headers
idiom emit the joined value N times.
2. **`AsDate`** — leave on the first field line?
3. **Blast radius.** Any Cript comparing a header that can legitimately
repeat starts seeing `a,b` where it saw `a`. Correct, but not what those
scripts were written against, so this wants a release note in
`doc/release-notes/upgrading.en.rst` rather than a quiet patch. Is v11 the
right place, or does this need a deprecation path?
I have a working implementation of the above (Option B +
`Count()`/`Values()` + the rule-of-three fix), with an AuTest covering the
joined read, per-line access, the inverted-branch case, and the write-path
round trip. Happy to open a PR once there's agreement on 1–3.
--
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]
