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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
