#1045

Renaming a dir to the same name fails if it's not empty on overlay mounts

Openajanonopened 16d ago

Trying to use os.rename to rename a dir fails if it's not empty on overlay mounts:

$ t="$(mktemp -d)"
$ monty -m "${t}::/mnt::overlay" -c "import os; os.mkdir('/mnt/foo'); open('/mnt/foo/x','w').close(); os.rename('/mnt/foo','/mnt/foo')"
error: Traceback (most recent call last):
  File "<string>", line 1, in <module>
    import os; os.mkdir('/mnt/foo'); open('/mnt/foo/x','w').close(); os.rename('/mnt/foo','/mnt/foo')
                                                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
OSError: [Errno 39] Directory not empty: '/mnt/foo'
$ echo $?
1

CPython doesn't fail in the same case (at least on Linux):

$ t="$(mktemp -d)"
$ python -c "import os; os.mkdir('${t}/foo'); open('${t}/foo/x','w').close(); os.rename('${t}/foo','${t}/foo')"
$ echo $?
0

This is not documented in Python, but is documented in the POSIX definition of rename: https://pubs.opengroup.org/onlinepubs/9799919799/functions/rename.html

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.

I believe the fix is as simple as:

diff --git a/crates/monty-fs/src/overlay.rs b/crates/monty-fs/src/overlay.rs
index dd3375e6..5961ee7f 100644
--- a/crates/monty-fs/src/overlay.rs
+++ b/crates/monty-fs/src/overlay.rs
@@ -1082,6 +1082,11 @@ fn rename(
         None => host_is_dir(ctx.mount_dir, resolve_real(src_vpath, ctx)?.for_dir_op()),
     };

+    // A self-rename is a no-op on every POSIX system regardless of contents
+    if src_rel == dst_rel {
+        return Ok(MontyObject::none());
+    }
+
     reject_rename_type_mismatch(state, &dst_rel, src_is_dir, ctx, dst_vpath)?;

     // Renaming a directory onto an existing non-empty directory must fail,

This should be the correct place, as it must come after the reject_mount_root checks to keep the deliberate mount root errors, and it must be after the src_is_dir check to still disallow a symlinked source.

Changing it here would also fix os.replace and pathlib.Path.rename AFAICT.

Happy to draft a PR with that change.

Activity

2
jaheba15d ago

Thanks for the issue and the proposed fix. It would be great, if you open a PR for this.

Changing it here would also fix os.replace and pathlib.Path.rename AFAICT.

They should be all the same, but we can just add tests?

ajanon8d ago

Hey @jaheba! Sorry for the dealy.

I just submitted https://github.com/pydantic/monty/pull/1109 to fix this.