apache / apache/trafficserver

Regex: replace RegexMatchContext with a value type; the current one has a latent double free

Open
#13,663 1 comment 0 reactions 1 assignee Claimed by @bryancall View on GitHub
Bug Core
Dominant language
C++
Stars
2k
Forks
874
Avg merge
6d 15h
Merged PRs (30d)
46

Description

## Summary

#13661 stops a caller-supplied `RegexMatchContext` from silently diverging from the
shared context. It does not address why the type made that divergence easy in the
first place. Reviewing the interface for that question turned up four more defects,
one of which is a latent double free.

This issue proposes replacing the type rather than patching it.

## The defects

### 1. The defaulted move constructor is a double free

`include/tsutil/Regex.h`:

```cpp
RegexMatchContext(RegexMatchContext &&) = default;
RegexMatchContext &operator=(RegexMatchContext &&) = default;

private:
struct _MatchContextPtr {
void *_ptr = nullptr;
};
_MatchContextPtr _match_context;
```

The only member is a struct holding a raw `void *`. A defaulted move copies that
pointer and leaves the source holding it, so both destructors call
`pcre2_match_context_free` on the same object.

```mermaid
flowchart LR
A["source ctx"] -->|"defaulted move"| B["moved-to ctx"]
A --- P(("pcre2_match_context"))
B --- P
A -->|"~RegexMatchContext"| F1["pcre2_match_context_free"]
B -->|"~RegexMatchContext"| F2["pcre2_match_context_free"]
F1 --> D["same pointer freed twice"]
F2 --> D
```

This is latent only because nothing in the tree moves one today. `regex_remap`
heap-allocates the struct that owns it and `esi` holds one by value. A `std::vector`
of either, or a reseat during config reload, reaches it.

### 2. The copy constructor can build an object that trips its own destructor

```cpp
RegexMatchContext::RegexMatchContext(RegexMatchContext const &other)
{
auto ptr = _MatchContext::get(other._match_context);
if (nullptr != ptr) {
...
}
}
```

If the source pointer is null, `_ptr` keeps its default null. The destructor then
fires `debug_assert_message` in a debug build and silently does nothing in a release
build. The copy assignment operator has an explicit `else` branch for the same case;
the copy constructor does not.

### 3. Two nulls, two meanings, one spelling

```cpp
pcre2_match_context_create(nullptr) // null = the general context: use standard malloc/free
Regex::exec(subject, matches, 0, nullptr) // null = the match context: use the shared one
```

Unrelated semantics, identical at the call site.

### 4. `my_malloc` and `my_free` are ceremony

```cpp
void *my_malloc(size_t size, void * /*caller*/) { return malloc(size); }
void my_free(void *ptr, void * /*caller*/) { free(ptr); }
```

So `_general_context` carries no information, and passing `nullptr` instead is
behaviourally identical today. That makes finding 3 harmless right now and a real
divergence the moment anyone makes that allocator do something, which is the same
shape as the bug in #13660 one layer down.

### 5. The root cause is an optional pointer with a null sentinel

Making "use the shared default" and "use mine" the same argument, distinguished by a
null, is what let a blank context hide. The type also exposes exactly one setter,
`set_match_limit`, so it is a one-field options object wearing a heap-allocated PCRE2
context as a costume, with hand-rolled copy semantics every caller has to get right.

## Proposal

```mermaid
flowchart TD
subgraph now["today"]
N1["caller builds a RegexMatchContext"] --> N2["owns a heap PCRE2 object"]
N2 --> N3["hand-rolled copy, move, destructor"]
N3 --> N4["passes a pointer, or null"]
end
subgraph after["proposed"]
A1["caller fills an Options value"] --> A2["Regex owns the only context"]
A2 --> A3["nothing to copy or free"]
end
now --> after
```

```cpp
struct Regex::Options {
uint32_t match_limit = 0; // 0 = the shared default
};

int32_t exec(std::string_view subject, RegexMatches &matches, uint32_t flags = 0,
Options const &opts = {}) const;
```

- One configured match context per thread, owned by `Regex`, applied per call.
- Callers state intent as a value rather than owning a resource.
- A blank or partially configured context becomes unrepresentable, which is the fix
for the bug class rather than for one instance of it.
- Findings 1 and 2 disappear with the type instead of being patched.
- Findings 3 and 4 can be settled at the same time by deleting the no-op allocator
indirection or making it real.

## Scope

Two callers today: `plugins/regex_remap/regex_remap.cc` and
`plugins/esi/lib/IncludeUrlValidator.h`. Both set only a match limit, so both convert
directly. 31 files include `tsutil/Regex.h`, but the rest reach the shared context
through the default argument and are unaffected at the source level.

Related: #13660, #13661, #13654.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.