From patchwork Tue Feb 8 14:01:12 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 50916 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 4983D3858424 for ; Tue, 8 Feb 2022 14:03:12 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 4983D3858424 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1644328992; bh=sQuDv+Z0Et90QjR9GQS1pEHs7BZVO0UAGWW9AQcTF5s=; h=References:In-Reply-To:Date:Subject:To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=wmGrumgibf40/LxDNmgJWTenXH10bCgoQGC3AMGQP1QtIaNIlUm0JK2tK7WwG6fw8 5upEssfMk1vkYeXxCKjksA7OfRqJP6q3jX8wpx4+8jO85Nl9En/cWTq5Lg9fxGNRMZ mBT9fmebrPgUeWb5k1B0Abo3wq6gmQDb3BXkU2Hk= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 8942A3858404 for ; Tue, 8 Feb 2022 14:01:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8942A3858404 Received: from mail-yb1-f198.google.com (mail-yb1-f198.google.com [209.85.219.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-173-Bs8Ye92wOI-ARgklAZsgGQ-1; Tue, 08 Feb 2022 09:01:25 -0500 X-MC-Unique: Bs8Ye92wOI-ARgklAZsgGQ-1 Received: by mail-yb1-f198.google.com with SMTP id h6-20020a253a06000000b0061de83305f2so7262742yba.19 for ; Tue, 08 Feb 2022 06:01:25 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=sQuDv+Z0Et90QjR9GQS1pEHs7BZVO0UAGWW9AQcTF5s=; b=gpDxCG5OPU9DxLuYX9/fNJDt6q6QqPVULjg7SDcS9HmmxRG/Q5lOvNjqhZadgShNKK BsOwjNf3bBGxtxT2exRhsESXKXyM7hVryjb+PkF9Zj9J4vaOfBWRjzxSgVLBchr3Aq40 e9mzk1Or4SVuSUmhVjxfK7V1AUdXelf5KrTB+xNPAllQag7/Rx3rpj3e5bnWZv4cPM/F EFsEPa0AjbG6tegDUDA4qoUUT6ZJFtSMOPg++nD1+s8LWvqJDqO8/PIPq1le/FIzQgz/ MZpMFYpwzNBvLpRqUNgJj69W47I3W/V/0tBxYOQyZMOEN2JJWMEgraK5ZbsyxbM8r/Nu LqAQ== X-Gm-Message-State: AOAM533l5N5UY/V0HkBbalFhwHYyNMgxy2FVYnaKAWc9iWZ9fuf9UxKS b8FRdzhbYTWOHGXDXBrlgdkZR9zqgfUWf9q3Ggc2zKOhvM0OqqiwUJv2xmr3YZftyISUnbJiqLC xNasOcaaIFQ3JtRMv4uu+a9IKnx2OheulEw== X-Received: by 2002:a81:b61b:: with SMTP id u27mr4958830ywh.450.1644328884496; Tue, 08 Feb 2022 06:01:24 -0800 (PST) X-Google-Smtp-Source: ABdhPJxKV4uMPaQKcydDQzsQbMptuuXfc2eDaiCrMQnbmo72+lKlbqDCPsYi2bVotQFtsl3n/VkP5I2urhveCwO3Gjk= X-Received: by 2002:a81:b61b:: with SMTP id u27mr4958728ywh.450.1644328883356; Tue, 08 Feb 2022 06:01:23 -0800 (PST) MIME-Version: 1.0 References: <20220204235422.2710793-1-jwakely@redhat.com> In-Reply-To: Date: Tue, 8 Feb 2022 14:01:12 +0000 Message-ID: Subject: [committed] libstdc++: Fix filesystem::remove_all for Windows [PR104161] To: Jonathan Wakely X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-12.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Jonathan Wakely via Gcc-patches From: Jonathan Wakely Reply-To: Jonathan Wakely Cc: libstdc++ , gcc Patches Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" On Sat, 5 Feb 2022 at 01:08, Jonathan Wakely wrote: > > On Fri, 4 Feb 2022 at 23:55, Jonathan Wakely wrote: > > +// Used to implement filesystem::remove_all. > > +fs::recursive_directory_iterator& > > +fs::recursive_directory_iterator::__erase(error_code* ecptr) > > +{ > > + error_code ec; > > + if (!_M_dirs) > > + { > > + ec = std::make_error_code(errc::invalid_argument); > > + return *this; > > + } > > + > > + // We never want to skip permission denied when removing files. > > + const bool skip_permission_denied = false; > > + // We never want to follow directory symlinks when removing files. > > + const bool nofollow = true; > > + > > + // Loop until we find something we can remove. > > + while (!ec) > > + { > > + auto& top = _M_dirs->top(); > > + > > + if (top.entry._M_type == file_type::directory) > > + { > > + _Dir dir = top.open_subdir(skip_permission_denied, nofollow, ec); > > + if (!ec) > > + { > > + __glibcxx_assert(dir.dirp != nullptr); > > + if (dir.advance(skip_permission_denied, ec)) > > + { > > + // Non-empty directory, recurse into it. > > + _M_dirs->push(std::move(dir)); > > + continue; > > + } > > + if (!ec) > > + { > > + // Directory is empty so we can remove it. > > + if (top.rmdir(ec)) > > + break; // Success > > + } > > + } > > + } > > + else if (top.unlink(ec)) > > + break; // Success > > + else if (top.entry._M_type == file_type::none) > > + { > > + // We did not have a cached type, so it's possible that top.entry > > + // is actually a directory, and that's why the unlink above failed. > > +#ifdef EPERM > > + // POSIX.1-2017 says unlinking a directory returns EPERM, > > + // but LSB allows EISDIR too. Some targets don't even define EPERM. > > + if (ec.value() == EPERM || ec.value() == EISDIR) > > +#else > > + if (ec.value() == EISDIR) > > +#endif > > This doesn't work on Windows because the top.unlink(ec) sets a Windows > error using the system category, so doesn't match the errno values > here. > > I have a fix. > > > std::uintmax_t > > fs::remove_all(const path& p) > > { > > - return fs::do_remove_all(p, ErrorReporter{"cannot remove all", p}); > > + uintmax_t count = 0; > > + auto st = filesystem::status(p); > > + if (!exists(st)) > > + return 0; > > + if (is_directory(st)) > > Gah, this remove_all(const path&) overload was supposed to be using > the same logic as the one below with an error_code parameter. > > I'll fix it on Monday. Here's that fix. Tested x86_64-linux, powerpc-aix, x86_64-w64-mingw. Pushed to trunk. commit 5750952bec1e632d1f804f4a1bed2f74c0f3b189 Author: Jonathan Wakely Date: Mon Feb 7 23:36:47 2022 libstdc++: Fix filesystem::remove_all for Windows [PR104161] The recursive_directory_iterator::__erase member was failing for Windows, because the entry._M_type value is always file_type::none (because _Dir_base::advance doesn't populate it for Windows) and top.unlink uses fs::remove which sets an error using the system_category. That meant that ec.value() was a Windows error code and not an errno value, so the comparisons to EPERM and EISDIR failed. Instead of depending on a specific Windows error code for attempting to remove a directory, just use directory_entry::refresh() to query the type first. This doesn't avoid the TOCTTOU races with directory symlinks, but we can't avoid them on Windows without openat and unlinkat, and creating symlinks requires admin privs on Windows anyway. This also fixes the fs::remove_all(const path&) overload, which was supposed to use the same logic as the other overload, but I forgot to change it before my previous commit. libstdc++-v3/ChangeLog: PR libstdc++/104161 * src/c++17/fs_dir.cc (fs::recursive_directory_iterator::__erase): [i_GLIBCXX_FILESYSTEM_IS_WINDOWS]: Refresh entry._M_type member, instead of checking for errno values indicating a directory. * src/c++17/fs_ops.cc (fs::remove_all(const path&)): Use similar logic to non-throwing overload. (fs::remove_all(const path&, error_code&)): Add comments. * src/filesystem/ops-common.h: Likewise. diff --git a/libstdc++-v3/src/c++17/fs_dir.cc b/libstdc++-v3/src/c++17/fs_dir.cc index 01b8c0d5693..54f135d2baf 100644 --- a/libstdc++-v3/src/c++17/fs_dir.cc +++ b/libstdc++-v3/src/c++17/fs_dir.cc @@ -476,6 +476,16 @@ fs::recursive_directory_iterator::__erase(error_code* ecptr) { auto& top = _M_dirs->top(); +#if _GLIBCXX_FILESYSTEM_IS_WINDOWS + // _Dir::unlink uses fs::remove which uses std::system_category() for + // Windows errror codes, so we can't just check for EPERM and EISDIR. + // Use directory_entry::refresh() here to check if we have a directory. + // This can be a TOCTTOU race, but we don't have openat or unlinkat to + // solve that on Windows, and generally don't support symlinks anyway. + if (top.entry._M_type == file_type::none) + top.entry.refresh(); +#endif + if (top.entry._M_type == file_type::directory) { _Dir dir = top.open_subdir(skip_permission_denied, nofollow, ec); @@ -498,12 +508,13 @@ fs::recursive_directory_iterator::__erase(error_code* ecptr) } else if (top.unlink(ec)) break; // Success +#if ! _GLIBCXX_FILESYSTEM_IS_WINDOWS else if (top.entry._M_type == file_type::none) { // We did not have a cached type, so it's possible that top.entry // is actually a directory, and that's why the unlink above failed. #ifdef EPERM - // POSIX.1-2017 says unlinking a directory returns EPERM, + // POSIX.1-2017 says unlink on a directory returns EPERM, // but LSB allows EISDIR too. Some targets don't even define EPERM. if (ec.value() == EPERM || ec.value() == EISDIR) #else @@ -516,6 +527,7 @@ fs::recursive_directory_iterator::__erase(error_code* ecptr) continue; } } +#endif } if (!ec) diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc index ae35b0535b3..4552a730bf2 100644 --- a/libstdc++-v3/src/c++17/fs_ops.cc +++ b/libstdc++-v3/src/c++17/fs_ops.cc @@ -1280,21 +1280,36 @@ fs::remove(const path& p, error_code& ec) noexcept std::uintmax_t fs::remove_all(const path& p) { + error_code ec; uintmax_t count = 0; - auto st = filesystem::status(p); - if (!exists(st)) - return 0; - if (is_directory(st)) + recursive_directory_iterator dir(p, directory_options{64|128}, ec); + switch (ec.value()) // N.B. assumes ec.category() == std::generic_category() + { + case 0: + // Iterate over the directory removing everything. { - recursive_directory_iterator dir(p, directory_options{64|128}), end; - path failed; + const recursive_directory_iterator end; while (dir != end) { - failed = dir->path(); - dir.__erase(); + dir.__erase(); // throws on error ++count; } } + // Directory is empty now, will remove it below. + break; + case ENOENT: + // Our work here is done. + return 0; + case ENOTDIR: + case ELOOP: + // Not a directory, will remove below. + break; + default: + // An error occurred. + _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove all", p, ec)); + } + + // Remove p itself, which is either a non-directory or is now empty. return count + fs::remove(p); } @@ -1303,11 +1318,12 @@ fs::remove_all(const path& p, error_code& ec) { uintmax_t count = 0; recursive_directory_iterator dir(p, directory_options{64|128}, ec); - switch (ec.value()) + switch (ec.value()) // N.B. assumes ec.category() == std::generic_category() { case 0: + // Iterate over the directory removing everything. { - recursive_directory_iterator end; + const recursive_directory_iterator end; while (dir != end) { dir.__erase(&ec); @@ -1316,6 +1332,7 @@ fs::remove_all(const path& p, error_code& ec) ++count; } } + // Directory is empty now, will remove it below. break; case ENOENT: // Our work here is done. @@ -1329,6 +1346,7 @@ fs::remove_all(const path& p, error_code& ec) // An error occurred. return -1; } + // Remove p itself, which is either a non-directory or is now empty. if (int last = fs::remove(p, ec); !ec) return count + last; diff --git a/libstdc++-v3/src/filesystem/ops-common.h b/libstdc++-v3/src/filesystem/ops-common.h index 2aa9b571230..978e8724154 100644 --- a/libstdc++-v3/src/filesystem/ops-common.h +++ b/libstdc++-v3/src/filesystem/ops-common.h @@ -63,6 +63,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __last_system_error() noexcept { #ifdef _GLIBCXX_FILESYSTEM_IS_WINDOWS + // N.B. use error_code::default_error_condition() to convert to generic. return {(int)::GetLastError(), std::system_category()}; #else return {errno, std::generic_category()};