Re: [PATCH] libstdc++: testsuite: fs rename to self may fail

[Alexandre Oliva <oliva@adacore.com>]

On Jun 22, 2022, Jonathan Wakely <jwakely@redhat.com> wrote:

> "If the old argument and the new argument resolve to either the same
> existing directory entry or different directory entries for the same
> existing file, rename() shall return successfully and perform no other
> action." and "If the link named by the new argument exists, it shall
> be removed and old renamed to new."

> Instead, the implementation of std::filesystem::rename should have a
> special-case for rtems (and maybe other targets) that implements the
> POSIX rename semantics if calling ::rename isn't good enough.

Other POSIX requirements that make implementing them "interesting" when
::rename() doesn't are:

- leaving "new" alone when the operation fails: implies permissions,
  same filesystem, etc must all be checked first, atomically, before
  removing the file, and if renaming somehow fails after that, "new"
  must be somehow brought back to existence

- if "new" exists, it must keep on existing throughout the renaming
  process: implies temporary removal is not allowed, even if needed to
  to allow ::rename to succeed; they'd have to be part of the same
  filesystem transaction

- even if both of the above could somehow be satisfied, there are all
  sorts of race conditions that could enable pre-check to succeed, but
  that, even the checks were implemented fully and perfectly, would
  still enable the rename to fail after removal of a preexisting new.


Clearly something's gotta give.  Since ensuring the existence of "new"
throughout renaming is impossible if you can't rename to existing
filenames, I propose giving that up, though it's a very useful property.

I'd start by trying to rename old to new.  If that fails with EEXIST,
I'd rename new to a temporary name, and then try to rename old to new
and then remove the temporary name.

If renaming new to a temporary name fails, that means we can't remove it
either.

If renaming old to the now-vacated new still fails, that means we can't
rename old, so rename the temporary back to new.

If removing the temporary name for new fails, that means we couldn't
remove it to begin with, say because it's not empty, or because we lack
permission to remove it, so rename both back.


Knowing rename won't overwrite an existing pathname makes it easier to
pick a temporary name to use for the renaming: we know there's no risk
of overwriting files created concurrently, though we may have to try
other names if the rename-to-temp fails with EEXIST.

Creating a temporary directory sibling to new and picking a temporary
name in that directory enables us to detect early some cases of lack of
permission to remove new, that renaming it in the same directory
wouldn't hit.

Creating such a temporary directory also increases new/..'s link count
before moving it.  In a way, it checks for room for old/.., and it's no
worse than keeping new under the same parent, but picking a temporary
dir elsewhere would enable the renaming even if new/.. is already at the
limit.  Concurrent attempts to create directories in the same parent dir
could enable the operation to fail while also preventing the reversal of
the preparatory renames.  That would be a rarer occurrence if we rename
new to a temporary name under the same parent.


And all this doesn't even cover the case of moving old into itself,
which we'd also have to detect and prevent.


This feels more and more like a case for xfail until it gets fixed in
the kernel, where atomic filesystem operations belong :-(

Would a patch to add:

// { dg-xfail-if "::rename is not POSIX-compliant" { target *-*-rtems* } }

to rename.cc tests be acceptable?  I'm afraid I can't go further down
this rabbit hole, and my choices ATM seem to be limited to XFAIL
patches, whether accepted by the GCC community or carried internally.

-- 
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 <https://stallmansupport.org>