From 0aaa65e768e6b12e2366543a167a002a40bbff63 Mon Sep 17 00:00:00 2001 From: Beman Dawes Date: Sun, 17 May 2009 15:55:46 +0000 Subject: [PATCH] Fix Filesystem #2925, copy_file atomiticity [SVN r53073] --- doc/reference.html | 17 +++++++++--- include/boost/filesystem/operations.hpp | 14 +++++++--- src/operations.cpp | 36 +++++++++++++++---------- test/operations_test.cpp | 16 +++++++++++ 4 files changed, 61 insertions(+), 22 deletions(-) diff --git a/doc/reference.html b/doc/reference.html index 05ce462..3916c08 100644 --- a/doc/reference.html +++ b/doc/reference.html @@ -410,8 +410,15 @@ error_code with the value of ec.

template <class Path> bool remove(const Path& p); template <class Path1, class Path2> void rename(const Path1& from_p, const Path2& to_p); + + BOOST_SCOPED_ENUM_START(copy_option) + { fail_if_exists, overwrite_if_exists }; + BOOST_SCOPED_ENUM_END + template <class Path1, class Path2> - void copy_file(const Path1& from_fp, const Path2& to_fp); + void copy_file(const Path1& from_fp, const Path2& to_fp, + BOOST_SCOPED_ENUM(copy_option) option=copy_option::fail_if_exists); + template <class Path> Path system_complete(const Path& p); template <class Path> Path complete(const Path& p, const Path& base=initial_path<Path>()); @@ -2249,7 +2256,9 @@ systems. --end note.]

existing file, it is removed. A symbolic link is itself renamed, rather than the file it resolves to being renamed. -- end note]

-
template <class Path1, class Path2> void copy_file(const Path1& from_fp, const Path2& to_fp);
+
template <class Path1, class Path2>
+  void copy_file(const Path1& from_fp, const Path2& to_fp,
+                 BOOST_SCOPED_ENUM(copy_option) option=copy_option::fail_if_exists);

Requires: Path1::external_string_type and Path2::external_string_type are the same type.

@@ -2257,7 +2266,7 @@ systems. --end note.]

resolves to are copied to the file to_fp resolves to.

Throws: basic_filesystem_error<Path> if from_fp.empty() || to_fp.empty() ||!exists(from_fp) || !is_regular_file(from_fp) - || exists(to_fp)

+ || (option==copy_option::fail_if_exists && exists(to_fp))

template <class Path> Path complete(const Path& p, const Path& base=initial_path<Path>());
@@ -3076,7 +3085,7 @@ final document.

Distributed under the Boost Software License, Version 1.0. See www.boost.org/LICENSE_1_0.txt

Revised -13 October 2008

+17 May 2009

diff --git a/include/boost/filesystem/operations.hpp b/include/boost/filesystem/operations.hpp index 2cac537..4d52ab7 100644 --- a/include/boost/filesystem/operations.hpp +++ b/include/boost/filesystem/operations.hpp @@ -15,6 +15,7 @@ #define BOOST_FILESYSTEM_OPERATIONS_HPP #include +#include #include #include @@ -182,7 +183,7 @@ namespace boost BOOST_FILESYSTEM_DECL system::error_code rename_api( const std::string & from, const std::string & to ); BOOST_FILESYSTEM_DECL system::error_code - copy_file_api( const std::string & from, const std::string & to ); + copy_file_api( const std::string & from, const std::string & to, bool fail_if_exists ); # if defined(BOOST_WINDOWS_API) @@ -226,7 +227,7 @@ namespace boost BOOST_FILESYSTEM_DECL system::error_code rename_api( const std::wstring & from, const std::wstring & to ); BOOST_FILESYSTEM_DECL system::error_code - copy_file_api( const std::wstring & from, const std::wstring & to ); + copy_file_api( const std::wstring & from, const std::wstring & to, bool fail_if_exists ); # endif # endif @@ -506,11 +507,16 @@ namespace boost from_path, to_path, ec ) ); } - BOOST_FS_FUNC(void) copy_file( const Path & from_path, const Path & to_path ) + BOOST_SCOPED_ENUM_START(copy_option) + { fail_if_exists, overwrite_if_exists }; + BOOST_SCOPED_ENUM_END + + BOOST_FS_FUNC(void) copy_file( const Path & from_path, const Path & to_path, + BOOST_SCOPED_ENUM(copy_option) option = copy_option::fail_if_exists ) { system::error_code ec( detail::copy_file_api( from_path.external_directory_string(), - to_path.external_directory_string() ) ); + to_path.external_directory_string(), option == copy_option::fail_if_exists ) ); if ( ec ) boost::throw_exception( basic_filesystem_error( "boost::filesystem::copy_file", diff --git a/src/operations.cpp b/src/operations.cpp index 0c74504..eefd8e1 100644 --- a/src/operations.cpp +++ b/src/operations.cpp @@ -706,9 +706,9 @@ namespace boost } BOOST_FILESYSTEM_DECL error_code - copy_file_api( const std::wstring & from, const std::wstring & to ) + copy_file_api( const std::wstring & from, const std::wstring & to, bool fail_if_exists ) { - return error_code( ::CopyFileW( from.c_str(), to.c_str(), /*fail_if_exists=*/true ) + return error_code( ::CopyFileW( from.c_str(), to.c_str(), fail_if_exists ) ? 0 : ::GetLastError(), system_category ); } @@ -886,9 +886,9 @@ namespace boost } BOOST_FILESYSTEM_DECL error_code - copy_file_api( const std::string & from, const std::string & to ) + copy_file_api( const std::string & from, const std::string & to, bool fail_if_exists ) { - return error_code( ::CopyFileA( from.c_str(), to.c_str(), /*fail_if_exists=*/true ) + return error_code( ::CopyFileA( from.c_str(), to.c_str(), fail_if_exists ) ? 0 : ::GetLastError(), system_category ); } @@ -1203,22 +1203,30 @@ namespace boost BOOST_FILESYSTEM_DECL error_code copy_file_api( const std::string & from_file_ph, - const std::string & to_file_ph ) + const std::string & to_file_ph, bool fail_if_exists ) { const std::size_t buf_sz = 32768; boost::scoped_array buf( new char [buf_sz] ); int infile=-1, outfile=-1; // -1 means not open - struct stat from_stat; - if ( ::stat( from_file_ph.c_str(), &from_stat ) != 0 - || (infile = ::open( from_file_ph.c_str(), - O_RDONLY )) < 0 - || (outfile = ::open( to_file_ph.c_str(), - O_WRONLY | O_CREAT | O_EXCL, - from_stat.st_mode )) < 0 ) + // bug fixed: code previously did a stat() on the from_file first, but that + // introduced a gratuitous race condition; the stat() is now done after the open() + + if ( (infile = ::open( from_file_ph.c_str(), O_RDONLY )) < 0 ) + { return error_code( errno, system_category ); } + + struct stat from_stat; + if ( ::stat( from_file_ph.c_str(), &from_stat ) != 0 ) + { return error_code( errno, system_category ); } + + int oflag = O_CREAT | O_WRONLY; + if ( fail_if_exists ) oflag |= O_EXCL; + if ( (outfile = ::open( to_file_ph.c_str(), oflag, from_stat.st_mode )) < 0 ) { - if ( infile >= 0 ) ::close( infile ); - return error_code( errno, system_category ); + int open_errno = errno; + BOOST_ASSERT( infile >= 0 ); + ::close( infile ); + return error_code( open_errno, system_category ); } ssize_t sz, sz_read=1, sz_write; diff --git a/test/operations_test.cpp b/test/operations_test.cpp index a00a147..36406d1 100644 --- a/test/operations_test.cpp +++ b/test/operations_test.cpp @@ -711,6 +711,22 @@ int main( int argc, char * argv[] ) BOOST_TEST( fs::exists( d1 / "f2" ) ); BOOST_TEST( !fs::is_directory( d1 / "f2" ) ); verify_file( d1 / "f2", "foobar1" ); + + bool copy_ex_ok = false; + try { fs::copy_file( file_ph, d1 / "f2" ); } + catch ( const fs::filesystem_error & ) { copy_ex_ok = true; } + BOOST_TEST( copy_ex_ok ); + + copy_ex_ok = false; + try { fs::copy_file( file_ph, d1 / "f2", fs::copy_option::fail_if_exists ); } + catch ( const fs::filesystem_error & ) { copy_ex_ok = true; } + BOOST_TEST( copy_ex_ok ); + + copy_ex_ok = true; + try { fs::copy_file( file_ph, d1 / "f2", fs::copy_option::overwrite_if_exists ); } + catch ( const fs::filesystem_error & ) { copy_ex_ok = false; } + BOOST_TEST( copy_ex_ok ); + std::cout << "copy_file test complete" << std::endl; // rename() test case numbers refer to operations.htm#rename table