From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 1ED7F3834F0F for ; Wed, 22 Jun 2022 09:45:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1ED7F3834F0F Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-564-xj8dZiT7PD24uSuWfAk3Sg-1; Wed, 22 Jun 2022 05:45:26 -0400 X-MC-Unique: xj8dZiT7PD24uSuWfAk3Sg-1 Received: by mail-ej1-f71.google.com with SMTP id qa41-20020a17090786a900b00722f313a60eso549610ejc.13 for ; Wed, 22 Jun 2022 02:45:25 -0700 (PDT) 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=UDNjcTwe2UOxhKOnBtkKquhlpKEB4jodwerUrEN1foY=; b=asm0JWJVHY8MMtRv4dwcAZo4NXGu5djutOIk+nkL7a8AUYsn0H+y9rWsyKFueKWmmX gy6aCbIMuM9BfKmAg/pDa+1TimoY8ijl4J7zD4LTwOc1sjn4+9o32mjCV3xtPW+w/3re bSxrRcQvuRHuqHTxISjBztolgPJZUI/s+4kTspeHoJ8Rna9MId3GGq3VF3DDHjxqvnN6 TnkF4zPjU0VtkZz5hGKWYpUtZ/qqtB9j27NnFLmLvkFpECt+n9RB1AnUjdzByFI9VwP9 covko6EpplvizFyBI96VGnq3fSa2oJu0JgNqA6k+sugRXK/lLQVxciHb5mu3h8qHggY2 dANQ== X-Gm-Message-State: AJIora/o2q1HlxMPIHiZF0/nmrpyccSF1QAMOy29K+q5VEMTADOUBX59 ZqU9VZKocGK9VrsBW/JVRAvZkHxp0lJH3LmJ+tYTI7FelJr2aM6EHWx/8YY8ZbDRfRuWNvTeJOF qAd6M60Jj3qEfyH5uQnzfOsoKSvQvpvo= X-Received: by 2002:a17:906:65c8:b0:713:ee3e:254d with SMTP id z8-20020a17090665c800b00713ee3e254dmr2382142ejn.0.1655891125032; Wed, 22 Jun 2022 02:45:25 -0700 (PDT) X-Google-Smtp-Source: AGRyM1tHopZNyfC31/4LFfrgCBePZASxIUlbzu0f0xUYkfMA89tevk7L84hK8cW9BUUOXvmDYJHKrDMn2egMVdRoeto= X-Received: by 2002:a17:906:65c8:b0:713:ee3e:254d with SMTP id z8-20020a17090665c800b00713ee3e254dmr2382122ejn.0.1655891124668; Wed, 22 Jun 2022 02:45:24 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Jonathan Wakely Date: Wed, 22 Jun 2022 10:45:13 +0100 Message-ID: Subject: Re: [PATCH] libstdc++: retry removal of dir entries if dir removal fails To: Alexandre Oliva Cc: gcc Patches , "libstdc++" X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-13.3 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=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jun 2022 09:45:29 -0000 On Wed, 22 Jun 2022 at 07:20, Alexandre Oliva via Libstdc++ wrote: > > > On some target systems (e.g. rtems6.0), removing directory components > while iterating over directory entries may cause some of the directory > entries to be skipped, which prevents the removal of the parent > directory from succeeding. This probably comes as no surprise to you, but for the record I don't think that's POSIX-confirming. The spec for readdir says: "If a file is removed from or added to the directory after the most recent call to opendir() or rewinddir(), whether a subsequent call to readdir() returns an entry for that file is unspecified." This implies to me that all existing directory entries should get visited, even if you remove some. > > Advancing the iterator before removing a member proved not to be > enough, so I've instead arranged for remove_all to retry the removal > of components if the removal of the parent dir fails after removing at > least one entry. The fail will be permanent only if no components got > removed in the current try. > > Regstrapped on x86_64-linux-gnu, also tested with a cross to > aarch64-rtems6. Ok to install? I haven't properly reviewed it yet, just commenting for now. It looks like it would be possible for this to livelock. If another process keeps writing to the directory tree you're trying to remove, it will keep removing some entries (increasing the count) but also keep failing to remove a non-empty directory. The current implementation will fail with an error in that case, rather than getting stuck forever in a loop. We could add a max-retries limit, although that would presumably mean the original problem remains for rtems if trying to remove a directory with a huge number of entries. > > PS: The implementation of remove_all has changed completely, compared That's PR104161, and in the r12-7062 commit I wrote: It would be possible to add a __rewind member to directory iterators too, to call rewinddir after each modification to the directory. That would make it more likely for filesystem::remove_all to successfully remove everything even if files are being written to the directory tree while removing it. It's unclear if that is actually prefereable, or if it's better to fail and report an error at the first opportunity. I was thinking of cases where there are concurrent modifications to the directory while we're trying to remove it, but the RTEMS case of skipping subsequent entries is more motivating. The rewind operation would reopen the current directory stream, but without creating a new directory iterator and starting again from the very top of the directory tree. For the non-recursive directory iterator there's no real difference, but for the recursive one it would only rewind at the current nesting depth, not restart from the top of the tree. I also need to consider the interaction with symlink races, and whether retrying can bring back the vulnerability that PR104161 addressed (I think the way you've done it doesn't have the same race condition, but would be susceptible to a different race condition where the top of the directory tree to be removed is different when you retry). > with the gcc-11 environment in which the need for this patch came up. I > have reimplemented it for mainline, and I have attempted to test it in > this environment, but new filesystem tests and subtests that fail on > rtems6.0 have impaired testing and prevented the full pass rate I got > for them with a similar patchset on gcc-11. > > > for libstdc++-v3/ChangeLog > > * src/c++17/fs_ops.cc (remove_all): Retry removal of > directory entries. > --- > libstdc++-v3/src/c++17/fs_ops.cc | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc > index 435368fa5c5ff..b3390310132b4 100644 > --- a/libstdc++-v3/src/c++17/fs_ops.cc > +++ b/libstdc++-v3/src/c++17/fs_ops.cc > @@ -1286,6 +1286,8 @@ fs::remove_all(const path& p) > { > error_code ec; > uintmax_t count = 0; > + retry: > + uintmax_t init_count = count; > recursive_directory_iterator dir(p, directory_options{64|128}, ec); > switch (ec.value()) // N.B. assumes ec.category() == std::generic_category() > { > @@ -1303,7 +1305,7 @@ fs::remove_all(const path& p) > break; > case ENOENT: > // Our work here is done. > - return 0; > + return count; > case ENOTDIR: > case ELOOP: > // Not a directory, will remove below. > @@ -1313,6 +1315,18 @@ fs::remove_all(const path& p) > _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove all", p, ec)); > } > > + if (count > init_count) > + { > + if (int last = fs::remove(p, ec); !ec) > + return count + last; > + else > + // Some systems seem to skip entries in the dir iteration if > + // you remove dir entries while iterating, so if we removed > + // anything in the dir in this round, and failed to remove > + // the dir (presumably because it wasn't empty), retry. > + goto retry; > + } > + > // Remove p itself, which is either a non-directory or is now empty. > return count + fs::remove(p); > } > @@ -1321,6 +1335,8 @@ std::uintmax_t > fs::remove_all(const path& p, error_code& ec) > { > uintmax_t count = 0; > + retry: > + uintmax_t init_count = count; > recursive_directory_iterator dir(p, directory_options{64|128}, ec); > switch (ec.value()) // N.B. assumes ec.category() == std::generic_category() > { > @@ -1341,7 +1357,7 @@ fs::remove_all(const path& p, error_code& ec) > case ENOENT: > // Our work here is done. > ec.clear(); > - return 0; > + return count; > case ENOTDIR: > case ELOOP: > // Not a directory, will remove below. > @@ -1354,6 +1370,8 @@ fs::remove_all(const path& p, error_code& ec) > // Remove p itself, which is either a non-directory or is now empty. > if (int last = fs::remove(p, ec); !ec) > return count + last; > + if (count > init_count) > + goto retry; > return -1; > } > > > -- > Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > Disinformation flourishes because many people care deeply about injustice > but very few check the facts. Ask me about >