Message ID | fa2e2be3-f8d6-0773-7d17-3fc374156363@sawicki.us |
---|---|
State | New |
Headers | show |
Series | Rope iterators: don't retain pointers when copied | expand |
On 20/05/18 21:28 -0700, Jeremy Sawicki wrote: >Rope iterators sometimes contain pointers to an internal buffer >inside the iterator itself. When such an iterator is copied, the >copy incorrectly retains pointers to the original. > >This patch takes the simple approach of not copying the cached >information when the internal buffer is being used, instead >requiring it to be recomputed when the copied iterator is >dereferenced. An alternative would be to adjust the pointers so >they refer to the buffer in the copy. > >I tested on Linux x64 with "<srcdir>/configure", "make bootstrap", >and "make -k check", three times: (1) with no changes (as a >baseline), (2) with only the new test (to make sure it fails), and >(3) with the new test and bug fix (to make sure the test passes and >nothing else changes). > > > * include/ext/rope (_Rope_iterator_base(const _Rope_iterator_base&)) > (_Rope_const_iterator::operator=(const _Rope_const_iterator&)) > (_Rope_iterator::operator=(const _Rope_iterator&)): > Copied/assigned rope iterators don't retain pointers to the > iterator they were copied/assigned from. > * testsuite/ext/rope/7.cc: New. Jeremy's copyright assignment is complete, so I'm committing this patch. Tested x86_64-linux. Thanks very much! >Index: libstdc++-v3/include/ext/rope >=================================================================== >--- libstdc++-v3/include/ext/rope (revision 260403) >+++ libstdc++-v3/include/ext/rope (working copy) >@@ -1119,7 +1119,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > _Rope_iterator_base(const _Rope_iterator_base& __x) > { >- if (0 != __x._M_buf_ptr) >+ if (0 != __x._M_buf_ptr && __x._M_buf_start != __x._M_tmp_buf) > *this = __x; > else > { >@@ -1166,7 +1166,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > _Rope_const_iterator& > operator=(const _Rope_const_iterator& __x) > { >- if (0 != __x._M_buf_ptr) >+ if (0 != __x._M_buf_ptr && __x._M_buf_start != __x._M_tmp_buf) > *(static_cast<_Rope_iterator_base<_CharT, _Alloc>*>(this)) = __x; > else > { >@@ -1345,7 +1345,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > _RopeRep* __old = this->_M_root; > > _RopeRep::_S_ref(__x._M_root); >- if (0 != __x._M_buf_ptr) >+ if (0 != __x._M_buf_ptr && __x._M_buf_start != __x._M_tmp_buf) > { > _M_root_rope = __x._M_root_rope; > *(static_cast<_Rope_iterator_base<_CharT, _Alloc>*>(this)) = __x; >Index: libstdc++-v3/testsuite/ext/rope/7.cc >=================================================================== >--- libstdc++-v3/testsuite/ext/rope/7.cc (nonexistent) >+++ libstdc++-v3/testsuite/ext/rope/7.cc (working copy) >@@ -0,0 +1,95 @@ >+// Copyright (C) 2018 Free Software Foundation, Inc. >+// >+// This file is part of the GNU ISO C++ Library. This library is free >+// software; you can redistribute it and/or modify it under the >+// terms of the GNU General Public License as published by the >+// Free Software Foundation; either version 3, or (at your option) >+// any later version. >+ >+// This library is distributed in the hope that it will be useful, >+// but WITHOUT ANY WARRANTY; without even the implied warranty of >+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >+// GNU General Public License for more details. >+ >+// You should have received a copy of the GNU General Public License along >+// with this library; see the file COPYING3. If not see >+// <http://www.gnu.org/licenses/>. >+ >+// rope (SGI extension) >+ >+// { dg-do run } >+ >+#include <ext/rope> >+#include <testsuite_hooks.h> >+ >+void >+test01() >+{ >+ using __gnu_cxx::crope; >+ >+ char str_a[] = >+ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" >+ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" >+ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" >+ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" >+ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; >+ char str_b[] = >+ "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" >+ "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" >+ "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" >+ "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" >+ "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; >+ >+ // Create ropes with leaf nodes longer than __lazy_threshold = 128 >+ // so substring nodes will be created by the next step >+ crope leaf_rope_a(str_a); >+ crope leaf_rope_b(str_b); >+ >+ // Create ropes with substring nodes referencing the leaf nodes >+ // of the prior ropes >+ crope substring_rope_a(leaf_rope_a.begin() + 1, >+ leaf_rope_a.begin() + 199); >+ crope substring_rope_b(leaf_rope_b.begin() + 1, >+ leaf_rope_b.begin() + 199); >+ >+ // Create iterators to substring_rope_a >+ crope::const_iterator cit_orig = substring_rope_a.begin(); >+ crope::iterator mit_orig = substring_rope_a.mutable_begin(); >+ >+ // Dereference the iterators so they cache a portion of the substring >+ // node in their internal buffers >+ *cit_orig; >+ *mit_orig; >+ >+ // Copy the original iterators, via both copy constructors and >+ // assignment operators. Prior to the bug fix, these iterators >+ // retained pointers to the internal buffers of the original >+ // iterators. >+ crope::const_iterator cit_copy(cit_orig); >+ crope::iterator mit_copy(mit_orig); >+ crope::const_iterator cit_assign; cit_assign = cit_orig; >+ crope::iterator mit_assign; mit_assign = mit_orig; >+ >+ // Modify the original iterators to refer to substring_rope_b >+ cit_orig = substring_rope_b.begin(); >+ mit_orig = substring_rope_b.mutable_begin(); >+ >+ // Dereference the original iterators so they fill their internal >+ // buffers with part of substring_rope_b >+ *cit_orig; >+ *mit_orig; >+ >+ // Verify that the copied iterators return data from >+ // substring_rope_a, not substring_rope_b >+ VERIFY(*cit_copy == 'a'); >+ VERIFY(*mit_copy == 'a'); >+ VERIFY(*cit_assign == 'a'); >+ VERIFY(*mit_assign == 'a'); >+} >+ >+int >+main() >+{ >+ test01(); >+ return 0; >+}
Index: libstdc++-v3/include/ext/rope =================================================================== --- libstdc++-v3/include/ext/rope (revision 260403) +++ libstdc++-v3/include/ext/rope (working copy) @@ -1119,7 +1119,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Rope_iterator_base(const _Rope_iterator_base& __x) { - if (0 != __x._M_buf_ptr) + if (0 != __x._M_buf_ptr && __x._M_buf_start != __x._M_tmp_buf) *this = __x; else { @@ -1166,7 +1166,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Rope_const_iterator& operator=(const _Rope_const_iterator& __x) { - if (0 != __x._M_buf_ptr) + if (0 != __x._M_buf_ptr && __x._M_buf_start != __x._M_tmp_buf) *(static_cast<_Rope_iterator_base<_CharT, _Alloc>*>(this)) = __x; else { @@ -1345,7 +1345,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _RopeRep* __old = this->_M_root; _RopeRep::_S_ref(__x._M_root); - if (0 != __x._M_buf_ptr) + if (0 != __x._M_buf_ptr && __x._M_buf_start != __x._M_tmp_buf) { _M_root_rope = __x._M_root_rope; *(static_cast<_Rope_iterator_base<_CharT, _Alloc>*>(this)) = __x; Index: libstdc++-v3/testsuite/ext/rope/7.cc =================================================================== --- libstdc++-v3/testsuite/ext/rope/7.cc (nonexistent) +++ libstdc++-v3/testsuite/ext/rope/7.cc (working copy) @@ -0,0 +1,95 @@ +// Copyright (C) 2018 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +// rope (SGI extension) + +// { dg-do run } + +#include <ext/rope> +#include <testsuite_hooks.h> + +void +test01() +{ + using __gnu_cxx::crope; + + char str_a[] = + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + char str_b[] = + "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; + + // Create ropes with leaf nodes longer than __lazy_threshold = 128 + // so substring nodes will be created by the next step + crope leaf_rope_a(str_a); + crope leaf_rope_b(str_b); + + // Create ropes with substring nodes referencing the leaf nodes + // of the prior ropes + crope substring_rope_a(leaf_rope_a.begin() + 1, + leaf_rope_a.begin() + 199); + crope substring_rope_b(leaf_rope_b.begin() + 1, + leaf_rope_b.begin() + 199); + + // Create iterators to substring_rope_a + crope::const_iterator cit_orig = substring_rope_a.begin(); + crope::iterator mit_orig = substring_rope_a.mutable_begin(); + + // Dereference the iterators so they cache a portion of the substring + // node in their internal buffers + *cit_orig; + *mit_orig; + + // Copy the original iterators, via both copy constructors and + // assignment operators. Prior to the bug fix, these iterators + // retained pointers to the internal buffers of the original + // iterators. + crope::const_iterator cit_copy(cit_orig); + crope::iterator mit_copy(mit_orig); + crope::const_iterator cit_assign; cit_assign = cit_orig; + crope::iterator mit_assign; mit_assign = mit_orig; + + // Modify the original iterators to refer to substring_rope_b + cit_orig = substring_rope_b.begin(); + mit_orig = substring_rope_b.mutable_begin(); + + // Dereference the original iterators so they fill their internal + // buffers with part of substring_rope_b + *cit_orig; + *mit_orig; + + // Verify that the copied iterators return data from + // substring_rope_a, not substring_rope_b + VERIFY(*cit_copy == 'a'); + VERIFY(*mit_copy == 'a'); + VERIFY(*cit_assign == 'a'); + VERIFY(*mit_assign == 'a'); +} + +int +main() +{ + test01(); + return 0; +}