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

[Giuseppe D'Angelo]

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.dangelo@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.

New patch attached.

Thanks,
-- 
Giuseppe D'Angelo

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libstdc-fix-a-dangling-reference-crash-in-ranges-is_.patch
Type: text/x-patch
Size: 3342 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20241221/69754404/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4244 bytes
Desc: S/MIME Cryptographic Signature
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20241221/69754404/attachment.p7s>