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

[bryancall]

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:

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.

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

Loading

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

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

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

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

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

Loading

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.

[bryancall]

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.