You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by "bryancall (via GitHub)" <gi...@apache.org> on 2026/09/09 22:58:47 UTC
[I] Regex: replace RegexMatchContext with a value type; the current one has a latent double free [trafficserver]
bryancall opened a new issue, #13663:
URL: https://github.com/apache/trafficserver/issues/13663
## 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.
--
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: issues-unsubscribe@trafficserver.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Regex: replace RegexMatchContext with a value type; the current one has a latent double free [trafficserver]
Posted by "bneradt (via GitHub) via issues" <is...@trafficserver.apache.org>.
bneradt closed issue #13663: Regex: replace RegexMatchContext with a value type; the current one has a latent double free
URL: https://github.com/apache/trafficserver/issues/13663
--
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: issues-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Regex: replace RegexMatchContext with a value type; the current one has a latent double free [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on issue #13663:
URL: https://github.com/apache/trafficserver/issues/13663#issuecomment-5624460308
Findings 1 and 2 are fixed in #13661 rather than here, because the fix turned out to
be deletion rather than repair.
Nothing in the tree copies or moves a `RegexMatchContext`. Every use is a plain member
or local: `plugins/regex_remap/regex_remap.cc:810`,
`plugins/esi/lib/IncludeUrlValidator.h:83`, and two unit tests. So all four special
members were dead code, and both defects lived only in code nobody called. They are now
`= delete`, which makes the double free and the null-copy unrepresentable instead of
patched. Verified: the tree still builds, and the full unit suite passes.
That leaves this issue with the three design points, which stand on their own:
- two nulls with two meanings and one spelling
- `my_malloc` and `my_free` as ceremony around a general context that carries no
information
- the optional-pointer-with-null-sentinel parameter, on a type whose entire public
surface is one setter
One correction to the framing above, which I had wrong. I described the JIT stack
callback in #13661 as necessary because a copied context can cross threads. That is not
the reason. `regex_remap` holds one context that every ET_NET thread matches through, so
the sharing is there whether or not the type is copyable, and deleting the copy members
does not remove the need for the callback.
That actually strengthens the case for the `Options` proposal. The problem is not that
the context can be copied. It is that a caller owns a long lived context at all.
--
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: issues-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org