Re: [Boost-bugs] [Boost C++ Libraries] #12311: boost::filesystem::directory_iterator(const path& p) iteration to cause a std::terminate()

Subject: Re: [Boost-bugs] [Boost C++ Libraries] #12311: boost::filesystem::directory_iterator(const path& p) iteration to cause a std::terminate()
From: Boost C++ Libraries (noreply_at_[hidden])
Date: 2017-12-06 22:45:17

#12311: boost::filesystem::directory_iterator(const path& p) iteration to cause a
std::terminate()
---------------------------------+-------------------------
  Reporter:  florent.castelli@…  |      Owner:  Beman Dawes
      Type:  Bugs                |     Status:  new
 Milestone:  To Be Determined    |  Component:  filesystem
   Version:  Boost 1.61.0        |   Severity:  Problem
Resolution:                      |   Keywords:
---------------------------------+-------------------------

Comment (by Matthew DeLoera <mmdeloera@…>):

 I'm dealing with this issue in 1.64. The description isn't accurate, and
 there are actually multiple related issues.

 '''Issue 1'''
 * I'm working with the Win32 build BTW, but I expect all platforms are
 broken.
 * Referring to include/boost/filesystem/operations.hpp and
 libs/filesystem/src/operations.cpp.
 * Both the except/noexcept constructors of directory_iterator
 (operations.hpp lines 901-905) call the same
 detail::directory_iterator_construct, either passing 0 or &ec as normal
 convention. This is fine.
 * Win32's FindFileFirst typically first returns "." for me, so
 directory_iterator_construct internally calls {{{it.increment(*ec)}}}.
 '''This is NOT fine - you have just obfuscated the pointer with a
 reference.'''
 * So directory_iterator::increment '''always''' gets a system::error_code
 &ec, but it's a bad reference if I had used the except constructor.
 '''This is the piece that needs changed. Pass error_code*, not
 error_code&.'''
 * So even though obviously bad reference, non-NULL pointer is passed to
 detail::directory_iterator_increment. If dir_itr_increment/FindNextFile
 fails (and sets temp_ec), ec will always be non-NULL, so it will never
 throw, but the {{{ec->assign}}} call will immediately burst into flames
 (i.e. access violation). '''This is the actual crash, not an unexpected
 exception as suggested above.'''
 * FYI, I encountered this issue using URIs over SMB, rather than local
 file system. I occasionally get successful FindFirstFile but failed
 FindNextFile, but that's a tangent here.

 '''Issue 2'''
 * As the original description does mention, please remove BOOST_NOEXCEPT
 from directory_iterator::increment, since it's obviously not true. Er that
 is, it's only true right now because your throw is always skipped. But
 once you fix that, you'll want to do this too.

 '''Issue 3'''
 * When you branch on ec==0 to either throw or not throw within
 directory_iterator_increment (operations.cpp line 2401), you're always
 obliterating the error code. Hence, when using non-exception
 directory_iterator, it's impossible to distinguish between a valid empty
 directory or internal iteration errors.
 * Fix Issue 1.
 * '''Now, instead throw or return temp_ec.''' Do NOT call BOOST_ERRNO! If
 dir_itr_increment/FindNextFile failed, it grabs Win32 GetLastError into
 temp_ec as it should, but then cleans up with Win32's FindClose, which
 obviously clears the static errno. This 2nd call to BOOST_ERRNO is pretty
 much guaranteed to return 0.

 '''Issue 4'''
 * In general operations.cpp calls GetLastError way more than it
 needs/should. I suspect there are other places where a subsequent
 GetLastError gets obliterated because of prior resource cleanup. I'd
 suggest grep and confirm. This isn't quite so urgent, but please do
 yourself a favor.

 BTW, I'm willing to pitch in with these fixes if that might be helpful.

-- 
Ticket URL: <https://svn.boost.org/trac10/boost/ticket/12311#comment:3>
Boost C++ Libraries <http://www.boost.org/>
Boost provides free peer-reviewed portable C++ source libraries.