[libcxx-commits] [PATCH] D130538: [libc++] Fix `_IterOps::__iter_move` returning a dangling reference for non-conforming iterators.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const created this revision.
Herald added a project: All.
var-const requested review of this revision.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

Without this patch, `_IterOps::__iter_move` for classic algorithms
relies on `iterator_traits` to distinguish between cases when
`operator*` returns a reference or a temporary. However, an invalid
proxy iterator might incorrectly specify its
`iterator_traits::reference` to be `value_type&` while its `operator*`
returns a prvalue proxy type. This has led in practice to corrupted data
in MySQL, noticed by Chromium tests (https://reviews.llvm.org/D129823#3677969).

While such an iterator is invalid, this is a regression introduced by
D129823 <https://reviews.llvm.org/D129823> which replaced calls of the form `value = *std::move(*iter);`
with `value = _IterOps<_AlgPolicy>::__iter_move(iter)`. The previous
form would work even with an invalid iterator as described above.

To work around this problem, use `decltype` instead of relying on
`iterator_traits` to deduce the return type of `operator*`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130538

Files:
  libcxx/include/__algorithm/iterator_operations.h


Index: libcxx/include/__algorithm/iterator_operations.h
===================================================================
--- libcxx/include/__algorithm/iterator_operations.h
+++ libcxx/include/__algorithm/iterator_operations.h
@@ -17,6 +17,7 @@
 #include <__iterator/iter_swap.h>
 #include <__iterator/iterator_traits.h>
 #include <__iterator/next.h>
+#include <__utility/declval.h>
 #include <__utility/forward.h>
 #include <__utility/move.h>
 #include <type_traits>
@@ -63,24 +64,37 @@
     return std::distance(__first, __last);
   }
 
+  template <class _Iter>
+  using __deref_t = decltype(*std::declval<_Iter>());
+
+  template <class _Iter>
+  using __move_t = decltype(std::move(std::declval<__deref_t<_Iter> >()));
+
   // iter_move
   template <class _Iter>
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
-      // Declaring the return type is necessary for C++03, so we basically mirror what `decltype(auto)` would deduce.
-      static __enable_if_t<
-          is_reference<typename iterator_traits<__uncvref_t<_Iter> >::reference>::value,
-          typename remove_reference< typename iterator_traits<__uncvref_t<_Iter> >::reference >::type&&>
-      __iter_move(_Iter&& __i) {
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11 static
+  // Declaring the return type is necessary for C++03.
+  // If the result of dereferencing `_Iter` is a reference type, deduce the result of calling `std::move` on it.
+  // Note that we can't rely on `iterator_traits` because those might be invalid. While that makes the user code
+  // non-conforming, it wouldn't be visible if the implementation used `foo = std::move(*iter);` rather than
+  // `foo = _IterOps<_Policy>::__iter_move(iter)`.
+  __enable_if_t<
+      is_reference<__deref_t<_Iter> >::value,
+      __move_t<_Iter> >
+  __iter_move(_Iter&& __i) {
     return std::move(*std::forward<_Iter>(__i));
   }
 
   template <class _Iter>
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
-      // Declaring the return type is necessary for C++03, so we basically mirror what `decltype(auto)` would deduce.
-      static __enable_if_t<
-          !is_reference<typename iterator_traits<__uncvref_t<_Iter> >::reference>::value,
-          typename iterator_traits<__uncvref_t<_Iter> >::reference>
-      __iter_move(_Iter&& __i) {
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11 static
+  // Declaring the return type is necessary for C++03.
+  // If the result of dereferencing `_Iter` is a value type, deduce the return value of this function to also be a
+  // value -- otherwise, after `operator*` returns a temporary, this function would return a dangling reference to that
+  // temporary.
+  __enable_if_t<
+      !is_reference<__deref_t<_Iter> >::value,
+      __deref_t<_Iter> >
+  __iter_move(_Iter&& __i) {
     return *std::forward<_Iter>(__i);
   }
 
@@ -100,7 +114,7 @@
 
   template <class _Iter>
   _LIBCPP_HIDE_FROM_ABI static _LIBCPP_CONSTEXPR_AFTER_CXX11
-  __uncvref_t<_Iter> next(_Iter&& __it, 
+  __uncvref_t<_Iter> next(_Iter&& __it,
                           typename iterator_traits<__uncvref_t<_Iter> >::difference_type __n = 1){
     return std::next(std::forward<_Iter>(__it), __n);
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D130538.447538.patch
Type: text/x-patch
Size: 3205 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20220726/987840ab/attachment-0001.bin>