JosiahWI commented on code in PR #13683:
URL: https://github.com/apache/trafficserver/pull/13683#discussion_r4083500390
##########
src/tsutil/Regex.cc:
##########
@@ -278,22 +351,61 @@ RegexMatchContext::operator=(RegexMatchContext const
&other)
{
if (&other != this) {
auto ptr = _MatchContext::get(other._match_context);
- if (nullptr != ptr) {
- pcre2_match_context *const ctx = pcre2_match_context_copy(ptr);
- _MatchContext::set(_match_context, ctx);
- } else {
- _MatchContext::set(_match_context, nullptr);
+
+ // Take the copy before releasing what this object already holds, so a
failing
+ // copy leaves it holding its old context rather than a freed one.
Releasing it
+ // is what this operator used to omit, and every assignment leaked one
context.
+ pcre2_match_context *const ctx = nullptr != ptr ?
pcre2_match_context_copy(ptr) : nullptr;
+
+ // pcre2_match_context_copy() returns nullptr when it cannot allocate.
Assigning it
+ // anyway would free the old context and leave this object empty, which is
the very
+ // thing the ordering above exists to prevent, so leave the object
untouched instead.
+ if (nullptr != ptr && nullptr == ctx) {
+ return *this;
}
+
+ pcre2_match_context *const old = _MatchContext::get(_match_context);
+
+ _MatchContext::set(_match_context, ctx);
+ if (old != nullptr) {
+ pcre2_match_context_free(old);
+ }
+ }
+ return *this;
+}
+
+//----------------------------------------------------------------------------
+RegexMatchContext::RegexMatchContext(RegexMatchContext &&that) noexcept
+{
+ // Through the typed accessors rather than std::exchange on the raw member:
_ptr is
+ // void *, and void * does not implicitly convert to pcre2_match_context *,
which is
+ // what set() takes.
+ _MatchContext::set(_match_context, _MatchContext::get(that._match_context));
+ _MatchContext::set(that._match_context, nullptr);
+}
+
+//----------------------------------------------------------------------------
+RegexMatchContext &
+RegexMatchContext::operator=(RegexMatchContext &&that) noexcept
+{
+ if (this != &that) {
+ if (auto *const old = _MatchContext::get(_match_context); old != nullptr) {
+ pcre2_match_context_free(old);
+ }
+ _MatchContext::set(_match_context,
_MatchContext::get(that._match_context));
+ _MatchContext::set(that._match_context, nullptr);
}
return *this;
}
//----------------------------------------------------------------------------
RegexMatchContext::~RegexMatchContext()
{
- auto ptr = _MatchContext::get(_match_context);
- debug_assert_message(ptr, "Failed to get the match context");
- if (ptr != nullptr) {
+ // No assert that the pointer is set. Null is now a legitimate state: a
moved-from
+ // object holds nothing, and asserting here would abort a debug build on the
first
+ // destruction of one. Before the move operations existed the only way to
reach this
+ // with null was a failed construction, which is why the assert was
reasonable then.
Review Comment:
This comment is talking about old code which is not relevant to this
revision.
--
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]
