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 rangesfor (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.
New patch attached. Thanks, -- Giuseppe D'Angelo
From 61b84e56c2034ae0f93f8d46ff0a1ca7bebf21e2 Mon Sep 17 00:00:00 2001
From: Giuseppe D'Angelo <giuseppe.dang...@kdab.com>
Date: Fri, 20 Dec 2024 12:55:01 +0100
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);
+
int
main()
{
test01();
test02();
test03();
+ VERIFY( test04() );
}
--
2.34.1
smime.p7s
Description: S/MIME Cryptographic Signature
