This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] libstdc++/67747 Allocate space for dirent::d_name


On 02/10/15 14:41 +0200, Florian Weimer wrote:
On 10/02/2015 02:34 PM, Jonathan Wakely wrote:
On 02/10/15 14:16 +0200, Florian Weimer wrote:
On 09/29/2015 01:37 PM, Jonathan Wakely wrote:
POSIX says that dirent::d_name has an unspecified length, so calls to
readdir_r must pass a buffer with enough trailing space for
{NAME_MAX}+1 characters. I wasn't doing that, which works OK on
GNU/Linux and BSD where d_name is a large array, but fails on Solaris
32-bit.

This uses pathconf to get NAME_MAX and allocates a buffer.

This still has a buffer overflow on certain file systems.

You must not use readdir_r, it is deprecated and always insecure.  We
should probably mark it as such in the glibc headers.

OK, I'll just use readdir() then. The directory stream is private to
the library type, so the only way to call readdir() concurrently on a
single directory stream is to increment iterators concurrently, which
is undefined anyway.

Right, that's the only case where readdir_r could be theoretically
useful.  But it's not a global structure, the callers have to coordinate
anyway, and so you could well use an external lock.

Here's a much simpler patch that just uses readdir, so not need for
pathconf, NAME_MAX and all that jazz.

Tested on GNU/Linux, DragonFly, AIX, committed to trunk.

So that will work as long as readdir() doesn't use a global static
buffer shared between streams, i.e. it meets the POSIX requirement
that "They shall not be affected by a call to readdir() on a different
directory stream." I don't know if mingw meets that, but there is lots
of work needed to make this stuff work in mingw.

If mingw has this flaw, it is worth fixing on its own, and mingw is
sufficiently alive that sticking workarounds into libstdc++ for its bugs
doesn't make sense (IMHO).

FWIW I checked and the readdir in mingw-w64 is OK.


commit 9cef7454bf1a18f21a7a561eef9165149a3330c9
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Oct 2 15:53:09 2015 +0100

    PR libstdc++/67747 use readdir instead of readdir_r
    
    	PR libstdc++/67747
    	* src/filesystem/dir.cc (native_readdir): Remove.
    	(_Dir::advance): Use readdir instead of native_readdir.
    	(recursive_directory_iterator(const path&, directory_options,
    	error_code*)): Use swap instead of reset.

diff --git a/libstdc++-v3/src/filesystem/dir.cc b/libstdc++-v3/src/filesystem/dir.cc
index bce751c..33280ec 100644
--- a/libstdc++-v3/src/filesystem/dir.cc
+++ b/libstdc++-v3/src/filesystem/dir.cc
@@ -69,15 +69,17 @@ struct fs::_Dir
 namespace
 {
   template<typename Bitmask>
-    inline bool is_set(Bitmask obj, Bitmask bits)
+    inline bool
+    is_set(Bitmask obj, Bitmask bits)
     {
       return (obj & bits) != Bitmask::none;
     }
 
   // Returns {dirp, p} on success, {nullptr, p} on error.
   // If an ignored EACCES error occurs returns {}.
-  fs::_Dir
-  open_dir(const fs::path& p, fs::directory_options options, std::error_code* ec)
+  inline fs::_Dir
+  open_dir(const fs::path& p, fs::directory_options options,
+	   std::error_code* ec)
   {
     if (ec)
       ec->clear();
@@ -100,7 +102,7 @@ namespace
   }
 
   inline fs::file_type
-  get_file_type(const dirent& d __attribute__((__unused__)))
+  get_file_type(const ::dirent& d __attribute__((__unused__)))
   {
 #ifdef _GLIBCXX_HAVE_STRUCT_DIRENT_D_TYPE
     switch (d.d_type)
@@ -128,20 +130,9 @@ namespace
     return fs::file_type::none;
 #endif
   }
-
-  int
-  native_readdir(DIR* dirp, ::dirent*& entryp)
-  {
-#ifdef _GLIBCXX_FILESYSTEM_IS_WINDOWS
-    if ((entryp = ::readdir(dirp)))
-      return 0;
-    return errno;
-#else
-    return ::readdir_r(dirp, entryp, &entryp);
-#endif
-  }
 }
 
+
 // Returns false when the end of the directory entries is reached.
 // Reports errors by setting ec or throwing.
 bool
@@ -150,9 +141,20 @@ fs::_Dir::advance(error_code* ec, directory_options options)
   if (ec)
     ec->clear();
 
-  ::dirent ent;
-  ::dirent* result = &ent;
-  if (int err = native_readdir(dirp, result))
+  int err = std::exchange(errno, 0);
+  const auto entp = readdir(dirp);
+  std::swap(errno, err);
+
+  if (entp)
+    {
+      // skip past dot and dot-dot
+      if (!strcmp(entp->d_name, ".") || !strcmp(entp->d_name, ".."))
+	return advance(ec, options);
+      entry = fs::directory_entry{path / entp->d_name};
+      type = get_file_type(*entp);
+      return true;
+    }
+  else if (err)
     {
       if (err == EACCES
         && is_set(options, directory_options::skip_permission_denied))
@@ -165,15 +167,6 @@ fs::_Dir::advance(error_code* ec, directory_options options)
       ec->assign(err, std::generic_category());
       return true;
     }
-  else if (result != nullptr)
-    {
-      // skip past dot and dot-dot
-      if (!strcmp(ent.d_name, ".") || !strcmp(ent.d_name, ".."))
-	return advance(ec, options);
-      entry = fs::directory_entry{path / ent.d_name};
-      type = get_file_type(ent);
-      return true;
-    }
   else
     {
       // reached the end
@@ -251,10 +244,10 @@ recursive_directory_iterator(const path& p, directory_options options,
 {
   if (DIR* dirp = ::opendir(p.c_str()))
     {
-      _M_dirs = std::make_shared<_Dir_stack>();
-      _M_dirs->push(_Dir{ dirp, p });
-      if (!_M_dirs->top().advance(ec))
-	_M_dirs.reset();
+      auto sp = std::make_shared<_Dir_stack>();
+      sp->push(_Dir{ dirp, p });
+      if (sp->top().advance(ec))
+	_M_dirs.swap(sp);
     }
   else
     {

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]