Re: [PATCH v2] libstdc++: fix a dangling reference crash in ranges::is_permutation

[Giuseppe D'Angelo]

Hello,

On 20/12/2024 13:23, Giuseppe D'Angelo wrote:

Hi,

The implementation of ranges::is_permutation may create a dangling
reference, which then results (sometimes) in a crash. A minimal example
that shows the problem under ASAN is https://gcc.godbolt.org/z/7bP9nE8fK

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.

Do you need me to create a report on bugzilla and cross-reference it
from the patch?

Better patch attached.

Thanks,
--
Giuseppe D'Angelo

From 2fe40c1dd49662c12d46dcb08b14c2ee68f9ea82 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

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.

Rather than finding a "creative" solution (e.g. use the value category
of `*it`, and the return type of calling the projection on that, to
determine whether we can keep a reference or we need a value), get rid
of the caching and call `invoke` as needed. In the common case the
projection is cheap, and we are allowed to dereference the same iterator
more than once (they're forward). This also sounds more correct because
we pass the correct value category (obtained from applying the
projection to the iterator) to the comparator.

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,
+	      return std::__invoke(__pred, std::__invoke(__proj1, *__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..4981d2c07ce 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,20 @@ test03()
   while (std::next_permutation(std::begin(cx), std::end(cx)));
 }
 
+void
+test04()
+{
+  int x[] = { 4, 3, 2, 1 };
+  auto y = std::views::iota(1, 5);
+  VERIFY( ranges::is_permutation(x, y) );
+  VERIFY( ranges::is_permutation(y, x) );
+}
+
 int
 main()
 {
   test01();
   test02();
   test03();
+  test04();
 }
-- 
2.34.1

smime.p7s
Description: S/MIME Cryptographic Signature