From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 130613 invoked by alias); 8 Jan 2020 16:44:43 -0000 Mailing-List: contact libstdc++-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libstdc++-owner@gcc.gnu.org Received: (qmail 130602 invoked by uid 89); 8 Jan 2020 16:44:42 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=UD:ops.cc, ops.cc, opscc X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (205.139.110.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 08 Jan 2020 16:44:40 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578501878; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type; bh=AmCDFR7ElOEIqt7s5D5E1QpKfdwBM/qiAAhtSZ46o0M=; b=ehF2RUOScc6Ta6ufjlz9Eqonr6m1Wt9xKoiAnHj/7KPf6brS5ig2+p87WD4TtRdA1weSmJ iRtgz1ZSvwUlq83rO7eDicT94ELVo1m/8418fZnG133HfG62RT9/dvwoQv0ZDKj2VWll7+ 7Ct92s75J4nGEmQmfenUVMKny0v9V9U= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-350-N3ze9Y9CNOSDoa-GILsiHw-1; Wed, 08 Jan 2020 11:44:37 -0500 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1C5F518B9FC1; Wed, 8 Jan 2020 16:44:36 +0000 (UTC) Received: from localhost (unknown [10.33.36.12]) by smtp.corp.redhat.com (Postfix) with ESMTP id 847105D9E1; Wed, 8 Jan 2020 16:44:35 +0000 (UTC) Date: Wed, 08 Jan 2020 16:44:00 -0000 From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [PATCH] libstdc++: Fix error handling in filesystem::remove_all (PR93201) Message-ID: <20200108164434.GA852711@redhat.com> MIME-Version: 1.0 X-Clacks-Overhead: GNU Terry Pratchett X-Mimecast-Spam-Score: 0 Content-Type: multipart/mixed; boundary="9jxsPFA5p3P2qPhR" Content-Disposition: inline X-IsSubscribed: yes X-SW-Source: 2020-01/txt/msg00038.txt.bz2 --9jxsPFA5p3P2qPhR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-length: 1816 When recursing into a directory, any errors that occur while removing a directory entry are ignored, because the subsequent increment of the directory iterator clears the error_code object. This fixes that bug by checking the result of each recursive operation before incrementing. This is a change in observable behaviour, because previously other directory entries would still be removed even if one (or more) couldn't be removed due to errors. Now the operation stops on the first error, which is what the code intended to do all along. The standard doesn't specify what happens in this case (because the order that the entries are processed is unspecified anyway). It also improves the error reporting so that the name of the file that could not be removed is included in the filesystem_error exception. This is done by introducing a new helper type for reporting errors with additional context and a new function that uses that type. Then the overload of std::filesystem::remove_all that throws an exception can use the new function to ensure any exception contains the additional information. For std::experimental::filesystem::remove_all just fix the bug where errors are ignored. PR libstdc++/93201 * src/c++17/fs_ops.cc (do_remove_all): New function implementing more detailed error reporting for remove_all. Check result of recursive call before incrementing iterator. (remove_all(const path&), remove_all(const path&, error_code&)): Use do_remove_all. * src/filesystem/ops.cc (remove_all(const path&, error_code&)): Check result of recursive call before incrementing iterator. * testsuite/27_io/filesystem/operations/remove_all.cc: Check errors are reported correctly. * testsuite/experimental/filesystem/operations/remove_all.cc: Likewise. Tested powerpc64le-linux, committed to trunk. --9jxsPFA5p3P2qPhR Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="patch.txt" Content-Transfer-Encoding: quoted-printable Content-length: 8895 commit b89525a0036c9b9f6d3d6952b54624fca27d9774 Author: Jonathan Wakely Date: Wed Jan 8 16:23:07 2020 +0000 libstdc++: Fix error handling in filesystem::remove_all (PR93201) =20=20=20=20 When recursing into a directory, any errors that occur while removing a directory entry are ignored, because the subsequent increment of the directory iterator clears the error_code object. =20=20=20=20 This fixes that bug by checking the result of each recursive operation before incrementing. This is a change in observable behaviour, because previously other directory entries would still be removed even if one (or more) couldn't be removed due to errors. Now the operation stops on the first error, which is what the code intended to do all along. The standard doesn't specify what happens in this case (because the order that the entries are processed is unspecified anyway). =20=20=20=20 It also improves the error reporting so that the name of the file that could not be removed is included in the filesystem_error exception. This is done by introducing a new helper type for reporting errors with additional context and a new function that uses that type. Then the overload of std::filesystem::remove_all that throws an exception can use the new function to ensure any exception contains the additional information. =20=20=20=20 For std::experimental::filesystem::remove_all just fix the bug where errors are ignored. =20=20=20=20 PR libstdc++/93201 * src/c++17/fs_ops.cc (do_remove_all): New function implementin= g more detailed error reporting for remove_all. Check result of recurs= ive call before incrementing iterator. (remove_all(const path&), remove_all(const path&, error_code&))= : Use do_remove_all. * src/filesystem/ops.cc (remove_all(const path&, error_code&)):= Check result of recursive call before incrementing iterator. * testsuite/27_io/filesystem/operations/remove_all.cc: Check er= rors are reported correctly. * testsuite/experimental/filesystem/operations/remove_all.cc: L= ikewise. diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_o= ps.cc index 8ad2e7fce1f..873f93aacfc 100644 --- a/libstdc++-v3/src/c++17/fs_ops.cc +++ b/libstdc++-v3/src/c++17/fs_ops.cc @@ -1275,42 +1275,105 @@ fs::remove(const path& p, error_code& ec) noexcept return false; } =20 +namespace std::filesystem +{ +namespace +{ + struct ErrorReporter + { + explicit + ErrorReporter(error_code& ec) : code(&ec) + { } + + explicit + ErrorReporter(const char* s, const path& p) + : code(nullptr), msg(s), path1(&p) + { } + + error_code* code; + const char* msg; + const path* path1; + + void + report(const error_code& ec) const + { + if (code) + *code =3D ec; + else + _GLIBCXX_THROW_OR_ABORT(filesystem_error(msg, *path1, ec)); + } + + void + report(const error_code& ec, const path& path2) const + { + if (code) + *code =3D ec; + else if (path2 !=3D *path1) + _GLIBCXX_THROW_OR_ABORT(filesystem_error(msg, *path1, path2, ec)); + else + _GLIBCXX_THROW_OR_ABORT(filesystem_error(msg, *path1, ec)); + } + }; + + uintmax_t + do_remove_all(const path& p, const ErrorReporter& err) + { + error_code ec; + const auto s =3D symlink_status(p, ec); + if (!status_known(s)) + { + if (ec) + err.report(ec, p); + return -1; + } + + ec.clear(); + if (s.type() =3D=3D file_type::not_found) + return 0; + + uintmax_t count =3D 0; + if (s.type() =3D=3D file_type::directory) + { + directory_iterator d(p, ec), end; + while (d !=3D end) + { + const auto removed =3D fs::do_remove_all(d->path(), err); + if (removed =3D=3D numeric_limits::max()) + return -1; + count +=3D removed; + + d.increment(ec); + if (ec) + { + err.report(ec, p); + return -1; + } + } + } + + if (fs::remove(p, ec)) + ++count; + if (ec) + { + err.report(ec, p); + return -1; + } + return count; + } +} +} =20 std::uintmax_t fs::remove_all(const path& p) { - error_code ec; - const auto result =3D remove_all(p, ec); - if (ec) - _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove all", p, ec)); - return result; + return fs::do_remove_all(p, ErrorReporter{"cannot remove all", p}); } =20 std::uintmax_t fs::remove_all(const path& p, error_code& ec) { - const auto s =3D symlink_status(p, ec); - if (!status_known(s)) - return -1; - ec.clear(); - if (s.type() =3D=3D file_type::not_found) - return 0; - - uintmax_t count =3D 0; - if (s.type() =3D=3D file_type::directory) - { - for (directory_iterator d(p, ec), end; !ec && d !=3D end; d.incremen= t(ec)) - count +=3D fs::remove_all(d->path(), ec); - if (ec.value() =3D=3D ENOENT) - ec.clear(); - else if (ec) - return -1; - } - - if (fs::remove(p, ec)) - ++count; - return ec ? -1 : count; + return fs::do_remove_all(p, ErrorReporter{ec}); } =20 void diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesyst= em/ops.cc index 79dc0fc6511..29ea9c0ce87 100644 --- a/libstdc++-v3/src/filesystem/ops.cc +++ b/libstdc++-v3/src/filesystem/ops.cc @@ -1099,12 +1099,17 @@ fs::remove_all(const path& p, error_code& ec) noexc= ept uintmax_t count =3D 0; if (s.type() =3D=3D file_type::directory) { - for (directory_iterator d(p, ec), end; !ec && d !=3D end; d.incremen= t(ec)) - count +=3D fs::remove_all(d->path(), ec); - if (ec.value() =3D=3D ENOENT) - ec.clear(); - else if (ec) - return -1; + directory_iterator d(p, ec), end; + while (!ec && d !=3D end) + { + const auto removed =3D fs::remove_all(d->path(), ec); + if (removed =3D=3D numeric_limits::max()) + return -1; + count +=3D removed; + d.increment(ec); + if (ec) + return -1; + } } =20 if (fs::remove(p, ec)) diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.= cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc index 5b920e490cb..7e018b51af2 100644 --- a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc +++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc @@ -140,10 +140,45 @@ test03() VERIFY( !exists(p) ); } =20 +void +test04() +{ +#if defined(__MINGW32__) || defined(__MINGW64__) + // no permissions +#else + // PR libstdc++/93201 + std::error_code ec; + std::uintmax_t n; + + auto dir =3D __gnu_test::nonexistent_path(); + fs::create_directory(dir); + __gnu_test::scoped_file f(dir/"file"); + // remove write permission on the directory: + fs::permissions(dir, fs::perms::owner_read|fs::perms::owner_exec); + n =3D fs::remove_all(dir, ec); + VERIFY( n =3D=3D -1 ); + VERIFY( ec =3D=3D std::errc::permission_denied ); // not ENOTEMPTY + + try { + fs::remove_all(dir); + VERIFY( false ); + } catch (const fs::filesystem_error& e) { + VERIFY( e.code() =3D=3D std::errc::permission_denied ); + // First path is the argument to remove_all + VERIFY( e.path1() =3D=3D dir ); + // Second path is the first file that couldn't be removed + VERIFY( e.path2() =3D=3D dir/"file" ); + } + + fs::permissions(dir, fs::perms::owner_write, fs::perm_options::add); +#endif +} + int main() { test01(); test02(); test03(); + test04(); } diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/remo= ve_all.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/remov= e_all.cc index 9e46c762f88..0e2aedae96d 100644 --- a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.= cc +++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.= cc @@ -108,9 +108,42 @@ test02() VERIFY( !exists(dir) ); } =20 +void +test04() +{ +#if defined(__MINGW32__) || defined(__MINGW64__) + // no permissions +#else + // PR libstdc++/93201 + std::error_code ec; + std::uintmax_t n; + + auto dir =3D __gnu_test::nonexistent_path(); + fs::create_directory(dir); + __gnu_test::scoped_file f(dir/"file"); + // remove write permission on the directory: + fs::permissions(dir, fs::perms::owner_read|fs::perms::owner_exec); + n =3D fs::remove_all(dir, ec); + VERIFY( n =3D=3D -1 ); + VERIFY( ec =3D=3D std::errc::permission_denied ); // not ENOTEMPTY + + try { + fs::remove_all(dir); + VERIFY( false ); + } catch (const fs::filesystem_error& e) { + VERIFY( e.code() =3D=3D std::errc::permission_denied ); + // First path is the argument to remove_all + VERIFY( e.path1() =3D=3D dir ); + } + + fs::permissions(dir, fs::perms::owner_write|fs::perms::add_perms); +#endif +} + int main() { test01(); test02(); + test04(); } --9jxsPFA5p3P2qPhR--