diff mbox series

libstdc++: fix buffer overflow in path::operator+= (PR92853)

Message ID 20191209095530.GA1835487@redhat.com
State New
Headers show
Series libstdc++: fix buffer overflow in path::operator+= (PR92853) | expand

Commit Message

Jonathan Wakely Dec. 9, 2019, 9:55 a.m. UTC 
When concatenating a path ending in a root-directory onto another path,
we added an empty filename to the end of the path twice, but only
reserved space for one. That meant the second write went past the end of
the allocated buffer.

	PR libstdc++/92853
	* src/c++17/fs_path.cc (filesystem::path::operator+=(const path&)):
	Do not process a trailing directory separator twice.
	* testsuite/27_io/filesystem/path/concat/92853.cc: New test.
	* testsuite/27_io/filesystem/path/concat/path.cc: Test more cases.

Tested powerpc64le-linux, committed to trunk. I'll backport to
gcc-9-branch too.
commit ac0d55229433ddd9609684e56474ed2335dd98d8
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon Dec 9 09:12:26 2019 +0000

    libstdc++: fix buffer overflow in path::operator+= (PR92853)
    
    When concatenating a path ending in a root-directory onto another path,
    we added an empty filename to the end of the path twice, but only
    reserved space for one. That meant the second write went past the end of
    the allocated buffer.
    
            PR libstdc++/92853
            * src/c++17/fs_path.cc (filesystem::path::operator+=(const path&)):
            Do not process a trailing directory separator twice.
            * testsuite/27_io/filesystem/path/concat/92853.cc: New test.
            * testsuite/27_io/filesystem/path/concat/path.cc: Test more cases.

Comments

Jonathan Wakely Dec. 9, 2019, 10 a.m. UTC | #1 
On 09/12/19 09:55 +0000, Jonathan Wakely wrote:
>When concatenating a path ending in a root-directory onto another path,
>we added an empty filename to the end of the path twice, but only
>reserved space for one. That meant the second write went past the end of
>the allocated buffer.
>
>	PR libstdc++/92853
>	* src/c++17/fs_path.cc (filesystem::path::operator+=(const path&)):
>	Do not process a trailing directory separator twice.
>	* testsuite/27_io/filesystem/path/concat/92853.cc: New test.
>	* testsuite/27_io/filesystem/path/concat/path.cc: Test more cases.

This adds similar improvements to the test for operator+= for strings,
rather than operator+=(const path&).

Tested x86_64-linux, committed to trunk. I'll backport this to
gcc-9-branch too.
diff mbox series

Patch

diff --git a/libstdc++-v3/src/c++17/fs_path.cc b/libstdc++-v3/src/c++17/fs_path.cc
index 5fba971fef6..3aefef271fa 100644
--- a/libstdc++-v3/src/c++17/fs_path.cc
+++ b/libstdc++-v3/src/c++17/fs_path.cc
@@ -975,16 +975,7 @@  path::operator+=(const path& p)
 	}
 
       if (it != last && it->_M_type() == _Type::_Root_dir)
-	{
-	  ++it;
-	  if (it == last)
-	    {
-	      // This root-dir becomes a trailing slash
-	      auto pos = _M_pathname.length() + p._M_pathname.length();
-	      ::new(output++) _Cmpt({}, _Type::_Filename, pos);
-	      ++_M_cmpts._M_impl->_M_size;
-	    }
-	}
+	++it;
 
       while (it != last)
 	{
diff --git a/libstdc++-v3/testsuite/27_io/filesystem/path/concat/92853.cc b/libstdc++-v3/testsuite/27_io/filesystem/path/concat/92853.cc
new file mode 100644
index 00000000000..62bde05c3ad
--- /dev/null
+++ b/libstdc++-v3/testsuite/27_io/filesystem/path/concat/92853.cc
@@ -0,0 +1,61 @@ 
+// Copyright (C) 2019 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/>.
+
+// { dg-options "-std=gnu++17" }
+// { dg-do run { target c++17 } }
+
+#include <filesystem>
+#include <testsuite_fs.h>
+
+void
+test01()
+{
+  // PR libstdc++/92853
+  using std::filesystem::path;
+  path p1{ "." }, p2{ "/" };
+  p1 += p2;	// corrupts heap
+  path p3{ p1 };	// CRASH!
+  __gnu_test::compare_paths( p3, "./" );
+}
+
+void
+test02()
+{
+  using std::filesystem::path;
+  path p1{ "." }, p2{ "////" };
+  p1 += p2;
+  path p3{ p1 };
+  __gnu_test::compare_paths( p3, ".////" );
+}
+
+void
+test03()
+{
+  using std::filesystem::path;
+  path p1{ "./" }, p2{ "/" };
+  p1 += p2;
+  path p3{ p1 };
+  __gnu_test::compare_paths( p3, ".//" );
+}
+
+int
+main()
+{
+  test01();
+  test02();
+  test03();
+}
diff --git a/libstdc++-v3/testsuite/27_io/filesystem/path/concat/path.cc b/libstdc++-v3/testsuite/27_io/filesystem/path/concat/path.cc
index 9f534e64cb7..16e668c0163 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/path/concat/path.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/path/concat/path.cc
@@ -55,6 +55,8 @@  test02()
     path x("//blah/di/blah");
     p += x;
     VERIFY( p.native() == prior_native + x.native() );
+    path copy(p);
+    compare_paths( copy, p );
   }
 }
 
@@ -66,10 +68,28 @@  test03()
   compare_paths(p, "a//b");
 }
 
+void
+test04()
+{
+  // Concat every test path onto every test path.
+  for (path p : __gnu_test::test_paths)
+  {
+    for (path x : __gnu_test::test_paths)
+    {
+      auto prior_native = p.native();
+      p += x;
+      VERIFY( p.native() == prior_native + x.native() );
+      path copy(p); // PR libstdc++/98523
+      compare_paths( copy, p );
+    }
+  }
+}
+
 int
 main()
 {
   test01();
   test02();
   test03();
+  test04();
 }