On Sat, 21 Dec 2024, Giuseppe D'Angelo wrote: > Hello, > > On 20/12/2024 22:20, Patrick Palka wrote: > > > > The attached patch fixes it. I've tested on Linux x86-64. Adding a > > > > negative test for this is somehow challenging (how do you test you're > > > > not using a dangling reference?), but running the modified test under > > > > ASAN shows the fix in place. > > > > I'd expect a constexpr version of the test to reliably fail as soon as > > it encounters the UB. > > Good idea! added. > > > > > > > > > > > > Do you need me to create a report on bugzilla and cross-reference it > > > > from the patch? > > > > That'd be good since we'll probably want to backport the fix to the > > release branches and the PR could reflect that. > > Done, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118160 > > > > Makes sense, but it might be preferable for sake of QoI to fix this > > without introducing extra dereferences or projection applications > > if possible. > > > > auto&& is supposed to perform lifetime extension, but that only happens > > for an outermost temporary and not any temporaries within a > > subexpression IIUC. So how about if we use a second auto&& for the > > *__scan subexpression so that lifetime extension occurs there? > > > > > > > > libstdc++-v3/ChangeLog: > > > > > > * include/bits/ranges_algo.h (__is_permutation_fn): Do not cache > > > the projection result in a local variable, as that may create > > > dangling references. > > > * testsuite/25_algorithms/is_permutation/constrained.cc: Add a > > > test with a range returning prvalues. > > > > > > Signed-off-by: Giuseppe D'Angelo <giuseppe.dang...@kdab.com> > > > --- > > > libstdc++-v3/include/bits/ranges_algo.h | 3 +-- > > > .../25_algorithms/is_permutation/constrained.cc | 11 +++++++++++ > > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > > > diff --git a/libstdc++-v3/include/bits/ranges_algo.h > > > b/libstdc++-v3/include/bits/ranges_algo.h > > > index 772bf4dd997..4d3c4325e2c 100644 > > > --- a/libstdc++-v3/include/bits/ranges_algo.h > > > +++ b/libstdc++-v3/include/bits/ranges_algo.h > > > @@ -567,9 +567,8 @@ namespace ranges > > > for (auto __scan = __first1; __scan != __last1; ++__scan) > > > { > > > - auto&& __proj_scan = std::__invoke(__proj1, *__scan); > > > auto __comp_scan = [&] <typename _Tp> (_Tp&& __arg) -> bool > > > { > > > - return std::__invoke(__pred, __proj_scan, > > > > If we go with the second auto&& approach then we should perfect forward > > __proj_scan here as you alluded to. That might seem unsafe at first > > glance (if say the project returns an rvalue) but since the predicate is > > regular_invocable it mustn't modify its arguments IIUC. > > Just to reiterate: if the projection returns an rvalue, it would be wrong for > the predicate to actually move from it, as it would violate the > equality-preserving semantic requirements of regular_invocable? I'll add the > missing forward then.
IIUC yes. I just opened https://gcc.gnu.org/PR118185 to track a similar missing forward in our ranges::clamp implementation. > > New patch attached. > > Thanks, > -- > Giuseppe D'Angelo > > > Subject: [PATCH] libstdc++: fix a dangling reference crash in > ranges::is_permutation [PR118160] > > The code was caching the result of `invoke(proj, *it)` in a local > `auto &&` variable. The problem is that this may create dangling > references, for instance in case `proj` is `std::identity` (the common > case) and `*it` produces a prvalue: lifetime extension does not > apply here due to the expressions involved. > > Instead, store (and lifetime-extend) the result of `*it` in a separate > variable, then project that variable. While at it, also forward the > result of the projection to the predicate, so that the predicate can > act on the proper value category. > > libstdc++-v3/ChangeLog: > > PR libstdc++/118160 > * include/bits/ranges_algo.h (__is_permutation_fn): Avoid a > dangling reference by storing the result of the iterator > dereference and the result of the projection in two distinct > variables, in order to lifetime-extend each one. > Forward the projected value to the predicate. > * testsuite/25_algorithms/is_permutation/constrained.cc: Add a > test with a range returning prvalues. Test it in a constexpr > context, in order to rely on the compiler to catch UB. > > Signed-off-by: Giuseppe D'Angelo <giuseppe.dang...@kdab.com> > --- > libstdc++-v3/include/bits/ranges_algo.h | 7 +++++-- > .../25_algorithms/is_permutation/constrained.cc | 13 +++++++++++++ > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/libstdc++-v3/include/bits/ranges_algo.h > b/libstdc++-v3/include/bits/ranges_algo.h > index 772bf4dd997..80d8123443f 100644 > --- a/libstdc++-v3/include/bits/ranges_algo.h > +++ b/libstdc++-v3/include/bits/ranges_algo.h > @@ -567,9 +567,12 @@ namespace ranges > > for (auto __scan = __first1; __scan != __last1; ++__scan) > { > - auto&& __proj_scan = std::__invoke(__proj1, *__scan); > + auto&& __scan_deref = *__scan; > + auto&& __proj_scan = > + std::__invoke(__proj1, > std::forward<decltype(__scan_deref)>(__scan_deref)); > auto __comp_scan = [&] <typename _Tp> (_Tp&& __arg) -> bool { > - return std::__invoke(__pred, __proj_scan, > + return std::__invoke(__pred, > + > std::forward<decltype(__proj_scan)>(__proj_scan), > std::forward<_Tp>(__arg)); > }; > if (__scan != ranges::find_if(__first1, __scan, > diff --git > a/libstdc++-v3/testsuite/25_algorithms/is_permutation/constrained.cc > b/libstdc++-v3/testsuite/25_algorithms/is_permutation/constrained.cc > index 2fbebe37609..d656d2761b9 100644 > --- a/libstdc++-v3/testsuite/25_algorithms/is_permutation/constrained.cc > +++ b/libstdc++-v3/testsuite/25_algorithms/is_permutation/constrained.cc > @@ -19,6 +19,7 @@ > > #include <algorithm> > #include <iterator> > +#include <ranges> > #include <testsuite_hooks.h> > #include <testsuite_iterators.h> > > @@ -76,10 +77,22 @@ test03() > while (std::next_permutation(std::begin(cx), std::end(cx))); > } > > +constexpr > +bool > +test04() // PR118160, do not create dangling references > +{ > + int x[] = { 4, 3, 2, 1 }; > + auto y = std::views::iota(1, 5); > + return ranges::is_permutation(x, y) && ranges::is_permutation(y, x); > +} > + > +static_assert(test04); Missing (), otherwise LGTM! > + > int > main() > { > test01(); > test02(); > test03(); > + VERIFY( test04() ); > } > -- > 2.34.1 >
