Repository navigation
regex: Constrain the lifetime of Regex::AsStr(), Match::Inner() and RegexFlagWithOptions::pattern() - #257
regex: Constrain the lifetime of Regex::AsStr(), Match::Inner() and RegexFlagWithOptions::pattern()#257dhivadhiva wants to merge 3 commits into
Conversation
…egexFlagWithOptions::pattern()
These accessors return a view or reference into storage owned by the
object, but had no lifetime annotation, so calling them on a temporary
compiled silently and dangled:
absl::string_view p = Regex::Compile(pattern)->AsStr(); // dangled
Apply the same guards used for zip, deflate, exif and xml:
- Regex::AsStr() and Match::Inner(): const&-qualify, add
ABSL_ATTRIBUTE_LIFETIME_BOUND, and delete the const&& overload.
- RegexFlagWithOptions::pattern(): add ABSL_ATTRIBUTE_LIFETIME_BOUND, but
keep the rvalue overload, since absl::GetFlag() returns a copy and
consuming the view inside the full-expression is common and safe.
Document what the compiler does and does not catch at each declaration.
Fixes google#256
|
Testing update: I've now checked this against real Crubit, not stand-in headers. I used Crubit f144a4f and
|
vabr-g
left a comment
There was a problem hiding this comment.
Thanks a lot for the PR! Looks good in general, I have one comment about amending the build config + two optional nits that are fine to ignore or do in a follow-up.
| #include <utility> | ||
| #include <vector> | ||
|
|
||
| #include "absl/base/attributes.h" |
There was a problem hiding this comment.
(regex/CMakeLists.txt:74-82). regex.h and regex_flag.h now include absl/base/attributes.h, so absl::core_headers should be added to the regex and regex_flag deps.
There was a problem hiding this comment.
Done: added absl::core_headers to the regex target. There's no regex_flag target in regex/CMakeLists.txt (regex_flag.h is header-only and nothing here compiles it), so there's nothing to add it to on this side. If your internal BUILD has a separate regex_flag target, it needs the dep there too.
There was a problem hiding this comment.
Optional: RegexFlagWithOptions::operator*, operator-> and get(), plus RegexListFlagWithOptions::begin()/end(), don't have ABSL_ATTRIBUTE_LIFETIME_BOUND. So absl::string_view p = absl::GetFlag(FLAGS_re)->AsStr(); gets around AsStr() const&& = delete and dangles. The header's own "BAD!" example (line 34) still compiles without a -Wdangling warning. Annotating these accessors fixes it, and calls like absl::GetFlag(FLAGS_re)->IsMatch(s) keep working.
It's OK not to add this here, the current change is already helpful without it. Just a suggestions for a related hardening.
There was a problem hiding this comment.
Done. operator*, operator->, get(), begin() and end() are now ABSL_ATTRIBUTE_LIFETIME_BOUND, and the rvalue overloads stay. With clang 21, your ->AsStr() example and the header's "BAD!" example now get -Wdangling. absl::GetFlag(FLAGS_re)->IsMatch(s) and a range-for over a RegexListFlag still compile cleanly.
There was a problem hiding this comment.
Optional: CaptureNames(), FindAll(), FindAllCaptures(), Split() and Captures::GetAll() return ranges that borrow *this (and text) on the Rust side. Before C++23, for (auto n : Regex::Compile(pat)->CaptureNames()) destroys the temporary before the loop runs, which is a use-after-free in next(). Consider making them const& ABSL_ATTRIBUTE_LIFETIME_BOUND plus const&& = delete.
Also this is fully optional, OK not to include here, just a suggestion for similar hardening.
There was a problem hiding this comment.
Done for CaptureNames(), FindAll(), FindAllCaptures(), both Split() overloads and Captures::GetAll(). Your loop example, and the same loop over the other four, now get -Wdangling in C++20, and spellings that give an rvalue Regex hit the deleted overload. In C++23 mode clang doesn't warn on the loop, which is right, since P2718 keeps the temporary alive.
One gap is left, documented on GetAll(): re.FindCaptures(text)->GetAll() goes through std::optional::operator->, like the Inner() case. I left the text borrow unannotated. Find() and FindCaptures() have the same issue, so it seemed better as a separate change.
regex.h and regex_flag.h now include absl/base/attributes.h for ABSL_ATTRIBUTE_LIFETIME_BOUND, so the regex target depends on absl::core_headers. regex_flag.h is header-only and has no target in this CMakeLists.txt.
Apply the same guards to two more groups of accessors, and document at
each declaration what the compiler does and does not catch.
RegexFlagWithOptions::operator*, operator-> and get(), and
RegexListFlagWithOptions::begin() and end(), now carry
ABSL_ATTRIBUTE_LIFETIME_BOUND. Before this, operator-> got around
AsStr() const&& = delete, so this compiled and dangled:
absl::string_view p = absl::GetFlag(FLAGS_re)->AsStr();
and so did the header's own "BAD!" example. The rvalue overloads stay,
so absl::GetFlag(FLAGS_re)->IsMatch(s) keeps working.
Regex::CaptureNames(), FindAll(), FindAllCaptures(), both Split()
overloads and Captures::GetAll() return ranges that borrow *this on the
Rust side. Before C++23, a range-based for loop destroys the Regex
before the loop body runs:
for (auto n : Regex::Compile(pat)->CaptureNames()) { ... }
These are now const& ABSL_ATTRIBUTE_LIFETIME_BOUND, and the const&&
overloads are deleted.
Not covered, and documented: the ranges also borrow `text`, which is
not annotated (Find() and FindCaptures() have the same gap), and a
temporary Captures reached through std::optional::operator-> is not
diagnosed.
Fixes #256.
Regex::AsStr(),Match::Inner()andRegexFlagWithOptions::pattern()return a view or reference into storage owned by the object, but had no lifetime annotation. So calling them on a temporary compiled silently and dangled:This applies the same guards used in zip (feed713), deflate (4eb75b8), exif (b2d80ab) and xml (7f76098):
Regex::AsStr()andMatch::Inner():const&-qualified,ABSL_ATTRIBUTE_LIFETIME_BOUNDadded, and theconst&&overload deleted.RegexFlagWithOptions::pattern():ABSL_ATTRIBUTE_LIFETIME_BOUNDonly. The rvalue overload is kept on purpose:absl::GetFlag()returns a copy, and consuming the view inside the full-expression (LOG(INFO) << absl::GetFlag(FLAGS_re).pattern();) is common and safe. Binding the result to a local is now diagnosed, because a copy of the flag can end up as the last owner of theRegexonce the flag is set again.Each declaration documents what the compiler does and doesn't catch, following the wording used in
zip/converters.h.Testing
Built with the full CMake setup (Crubit f144a4f, Rust nightly-2026-11-07, clang 21.1.8):
regex,regex_exampleandregex_benchmarkbuild, andregex_exampleruns.main.cc,regex.ccandregex_benchmark.cccompile with-Werror=dangling.clang -verifytest (below), run against the generated bindings, passes on this branch. Onmain, every one of its 23 expected diagnostics is missing.main:AsStr(),Inner()andpattern()reached from a temporary, as listed in the description above.*absl::GetFlag(...),->AsStr(),.get(),.begin()and.end()on a flag copy, kept in a local.FindAll()kept from a temporaryRegex, and a range-for overRegex::Compile(p)->CaptureNames(),FindAll(),FindAllCaptures()or eitherSplit()overload (C++20).CaptureNames(),FindAll(),FindAllCaptures(),Split()andGetAll()on an rvalueRegexorCaptures(deleted overloads).absl::GetFlag(FLAGS_re)->IsMatch(s), range-for over aRegexListFlagcopy,ParseNumericthroughInner(), andAbslUnparseFlagfor both flag types.lifetime_verify.cc
As in 7f76098,
re.Find(...)->Inner()still gets past the compiler, becausestd::optional::operator->isn't annotated. The header can't close that one.I know the README says the project isn't set up for contributions yet. If it's easier to land this internally, feel free to close this PR and take the change.