RE: [PATCH v2] libstdc++: Make forward_list::sort exception-safe [PR124051]
Odysseas Georgoudis <[email protected]>
| Newsgroups | gmane.comp.gcc.patches,gmane.comp.gcc.libstdc++.devel |
|---|---|
| Message-ID | <FRWP195MB28645250A5BB204F43EB5213CCA52@FRWP195MB2864.EURP195.PROD.OUTLOOK.COM> |
Hi Jonathan, Thanks for the review. I used an LLM as an additional review tool, but I wrote both the change and the test myself. I thought the added copyright block was needed. I have removed it in v2 and added the explicit return from main. I was not expecting a measurable regression on the normal path, because the added recovery code only runs when the comparator throws, but I benchmarked it to confirm. With the default comparator, the generated code was identical and timings were within noise. With an external potentially-throwing comparator, the worst result was about 1.9% slower. I kept the benchmark local, but I can send it if useful. Outlook sent the v1 patch as application/octet-stream, so I have attached v2 as PR124051-v2.patch.txt instead. Thanks, Odysseas ________________________________ From: Jonathan Wakely <[email protected]> Sent: 19 August 2026 12:19 To: Odysseas Georgoudis <[email protected]> Cc: [email protected] <[email protected]>; [email protected] <[email protected]> Subject: Re: [PATCH] libstdc++: Make forward_list::sort exception-safe [PR124051] On Sun, 16 Aug 2026 at 20:43 +0000, Odysseas Georgoudis wrote: >If a comparison called by forward_list::sort throws, the current >implementation can leave the list corrupted, which may cause a later >crash. > >This patch restores valid links before allowing the same exception to >continue. The element order remains unspecified, as permitted by the >standard, while iterators and references remain valid. > >Tested on x86_64-pc-linux-gnu. > >Thanks, >Odysseas Thanks for the patch to fix this. Have you tested whether this affects performance of the sort function? I hope it doesn't make much difference. Please don't attach patches as application/octet-stream - patches are plain text, not binary data. It makes it hard to review the patch by replying inline. Please read https://gcc.gnu.org/onlinedocs/libstdc++/manual/test.html#test.new_tests and adjust the new test accordingly. Based on the copyright notice in the test, it seems likely this was produced with the assistance of an LLM, is that correct?
PR124051-v2.patch.txt
(text/plain, 5.5 KB)
From c5b3713abef5c4c3831467a433523e282eb4010a Mon Sep 17 00:00:00 2001 From: Odysseas Georgoudis <[email protected]> Date: Sun, 16 Aug 2026 21:14:59 +0100 Subject: [PATCH v2] libstdc++: Make forward_list::sort exception-safe [PR124051] The bottom-up merge sort keeps its partially merged list in local pointers while the container head still points into the original chain. If the comparison throws after links have been changed, the head can refer to a cycle and some nodes are no longer reachable. Reconnect the merged prefix and both unconsumed runs, update the container head, and then rethrow. This leaves every node reachable exactly once and preserves all iterators and references. The success path still performs the same comparisons and link changes. libstdc++-v3/ChangeLog: PR libstdc++/124051 * include/bits/forward_list.tcc (forward_list::sort): Restore the list before propagating an exception from the comparison. * testsuite/23_containers/forward_list/operations/ sort_exception_124051.cc: New test. Signed-off-by: Odysseas Georgoudis <[email protected]> --- libstdc++-v3/include/bits/forward_list.tcc | 50 +++++++--- .../operations/sort_exception_124051.cc | 95 +++++++++++++++++++ 2 files changed, 133 insertions(+), 12 deletions(-) create mode 100644 libstdc++-v3/testsuite/23_containers/forward_list/operations/sort_exception_124051.cc diff --git a/libstdc++-v3/include/bits/forward_list.tcc b/libstdc++-v3/include/bits/forward_list.tcc index ffe2a2de84b..c5b417d8b87 100644 --- a/libstdc++-v3/include/bits/forward_list.tcc +++ b/libstdc++-v3/include/bits/forward_list.tcc @@ -468,20 +468,46 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER __p = __p->_M_next; --__psize; } - else if (!__comp(*static_cast<_Node&>(*__q)._M_valptr(), - *static_cast<_Node&>(*__p)._M_valptr())) - { - // First node of q is not lower; e must come from p. - __e = __p; - __p = __p->_M_next; - --__psize; - } else { - // First node of q is lower; e must come from q. - __e = __q; - __q = __q->_M_next; - --__qsize; + bool __take_q; + __try + { + __take_q + = __comp(*static_cast<_Node&>(*__q)._M_valptr(), + *static_cast<_Node&>(*__p)._M_valptr()); + } + __catch(...) + { + // Reconnect the merged prefix and unmerged nodes + // before propagating. + _Base_ptr __last_p = __p; + for (unsigned long __i = 1; __i < __psize; ++__i) + __last_p = __last_p->_M_next; + + if (__tail) + __tail->_M_next = __p; + else + __list = __p; + __last_p->_M_next = __q; + this->_M_impl._M_head._M_next = __list; + __throw_exception_again; + } + + if (!__take_q) + { + // First node of q is not lower; e must come from p. + __e = __p; + __p = __p->_M_next; + --__psize; + } + else + { + // First node of q is lower; e must come from q. + __e = __q; + __q = __q->_M_next; + --__qsize; + } } // Add the next node to the merged list. diff --git a/libstdc++-v3/testsuite/23_containers/forward_list/operations/sort_exception_124051.cc b/libstdc++-v3/testsuite/23_containers/forward_list/operations/sort_exception_124051.cc new file mode 100644 index 00000000000..d3a7d763e09 --- /dev/null +++ b/libstdc++-v3/testsuite/23_containers/forward_list/operations/sort_exception_124051.cc @@ -0,0 +1,95 @@ +// { dg-do run { target c++11 } } +// { dg-require-effective-target exceptions_enabled } + +#include <forward_list> +#include <testsuite_hooks.h> + +// PR libstdc++/124051 - forward_list::sort is not exception-safe + +struct exception { }; + +struct throwing_less +{ + unsigned* countdown; + + bool operator()(int lhs, int rhs) const + { + if (--*countdown == 0) + throw exception(); + return lhs < rhs; + } +}; + +typedef std::forward_list<int> list_type; + +void verify_list(const list_type& list, const int* const* addresses, + const list_type::iterator* iterators, unsigned size) +{ + for (unsigned i = 0; i < size; ++i) + { + VERIFY( *iterators[i] == static_cast<int>(i) ); + VERIFY( &*iterators[i] == addresses[i] ); + } + + unsigned seen = 0; + unsigned count = 0; + for (const int& value : list) + { + VERIFY( count < size ); + VERIFY( value >= 0 && value < static_cast<int>(size) ); + const unsigned bit = 1u << value; + VERIFY( (seen & bit) == 0 ); + VERIFY( &value == addresses[value] ); + seen |= bit; + ++count; + } + VERIFY( count == size ); + VERIFY( seen == (1u << size) - 1 ); +} + +void test01() +{ + const int values[] = { 6, 2, 8, 4, 11, 1, 12, 7, 3, 9, 5, 0, 10 }; + const unsigned size = sizeof(values) / sizeof(values[0]); + + for (unsigned throw_after = 1; ; ++throw_after) + { + list_type list(values, values + size); + const int* addresses[size]; + list_type::iterator iterators[size]; + for (list_type::iterator i = list.begin(); i != list.end(); ++i) + { + addresses[*i] = &*i; + iterators[*i] = i; + } + + unsigned countdown = throw_after; + bool caught = false; + try + { + list.sort(throwing_less{&countdown}); + } + catch (const exception&) + { + caught = true; + } + + verify_list(list, addresses, iterators, size); + if (caught) + list.sort(); + + int expected = 0; + for (int value : list) + VERIFY( value == expected++ ); + + if (!caught) + break; + VERIFY( throw_after < 100 ); + } +} + +int main() +{ + test01(); + return 0; +} -- 2.43.5