apache / apache/trafficserver

Cripts: reading a header returns only its first field line

Open
#13,647 0 comments 0 reactions 0 assignees View on GitHub
Cripts
Dominant language
C++
Stars
2k
Forks
874
Avg merge
6d 15h
Merged PRs (30d)
46

Description

## 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` 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.

Contributor guide

Open the contributing guide

Research direction

Start with src/cripts/Headers.cc and include/cripts/Headers.hpp, especially operator[], operator+=, iteration, and Header::String ownership; then read the referenced InkAPI paths and Context::reset(). Run the existing Cripts/AuTest coverage mentioned in the issue and verify joined reads, per-line access, duplicate-header branching, and write/read round trips. Done requires an agreed resolution to the iterator, AsDate, lifetime, and release-note questions.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend-api-design, networking
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.