Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rm doesn't handly restrictive permissions #47650

Closed
maleadt opened this issue Nov 21, 2022 · 4 comments
Closed

rm doesn't handly restrictive permissions #47650

maleadt opened this issue Nov 21, 2022 · 4 comments
Labels
bug Indicates an unexpected problem or unintended behavior filesystem Underlying file system and functions that use it

Comments

@maleadt
Copy link
Member

maleadt commented Nov 21, 2022

MWE:

mkpath("/tmp/work/work")
chmod("/tmp/work/work", 0o200)
rm("/tmp/work"; recursive=true)

... this throws:

ERROR: LoadError: IOError: readdir("/tmp/work/work"): permission denied (EACCES)
Stacktrace:
 [1] uv_error
   @ ./libuv.jl:97 [inlined]
 [2] readdir(dir::String; join::Bool, sort::Bool)
   @ Base.Filesystem ./file.jl:865
 [3] readdir
   @ ./file.jl:858 [inlined]
 [4] rm(path::String; force::Bool, recursive::Bool)
   @ Base.Filesystem ./file.jl:293
 [5] rm(path::String; force::Bool, recursive::Bool)
   @ Base.Filesystem ./file.jl:294
 [6] top-level scope
   @ /tmp/wip.jl:4
in expression starting at /tmp/wip.jl:4

... whereas rm just works:

execve("/usr/bin/rm", ["rm", "-r", "/tmp/work"], 0x7fff236280e0 /* 59 vars */) = 0
...
newfstatat(4, "work", {st_mode=S_IFDIR|0200, st_size=40, ...}, AT_SYMLINK_NOFOLLOW) = 0
openat(4, "work", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW|O_DIRECTORY) = -1 EACCES (Permission denied)
newfstatat(4, "work", {st_mode=S_IFDIR|0200, st_size=40, ...}, AT_SYMLINK_NOFOLLOW) = 0
faccessat2(4, "work", W_OK, AT_EACCESS) = 0
openat(4, "work", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY) = -1 EACCES (Permission denied)
newfstatat(4, "work", {st_mode=S_IFDIR|0200, st_size=40, ...}, AT_SYMLINK_NOFOLLOW) = 0
faccessat2(4, "work", W_OK, AT_EACCESS) = 0
unlinkat(4, "work", AT_REMOVEDIR)       = 0
close(4)                                = 0

Setting force=true "fixes" this, but that shouldn't be required (as rm doesn't need it), and also doesn't work when rm is invoked from, say, the mktempdir clean-up handle:

mktempdir() do dir
    mkpath("$dir/work/work")
    chmod("$dir/work/work", 0o200)
end
┌ Error: mktempdir cleanup
│   exception =
│    IOError: readdir("/tmp/jl_paLsYX/work/work"): permission denied (EACCES)
│    Stacktrace:
│      [1] uv_error
│        @ ./libuv.jl:97 [inlined]
│      [2] readdir(dir::String; join::Bool, sort::Bool)
│        @ Base.Filesystem ./file.jl:865
│      [3] readdir
│        @ ./file.jl:858 [inlined]
│      [4] rm(path::String; force::Bool, recursive::Bool)
│        @ Base.Filesystem ./file.jl:293
│      [5] rm(path::String; force::Bool, recursive::Bool) (repeats 2 times)
│        @ Base.Filesystem ./file.jl:294
│      [6] mktempdir(fn::var"#2#3", parent::String; prefix::String)
│        @ Base.Filesystem ./file.jl:769
│      [7] mktempdir(fn::Function, parent::String) (repeats 2 times)
│        @ Base.Filesystem ./file.jl:760
│      [8] top-level scope
│        @ /tmp/wip.jl:2
│      [9] include(mod::Module, _path::String)
│        @ Base ./Base.jl:419
│     [10] exec_options(opts::Base.JLOptions)
│        @ Base ./client.jl:303
│     [11] _start()
│        @ Base ./client.jl:522
└ @ Base.Filesystem file.jl:772

Files like this do exist in the wild; work/work here is similar to what overlayfs mounts create (albeit with an even more restrictive chmod of 000, which I didn't use here because it makes rm prompt for confirmation).

@maleadt maleadt added bug Indicates an unexpected problem or unintended behavior filesystem Underlying file system and functions that use it labels Nov 21, 2022
maleadt added a commit to maleadt/Sandbox.jl that referenced this issue Nov 21, 2022
maleadt added a commit to maleadt/Sandbox.jl that referenced this issue Nov 21, 2022
maleadt added a commit to maleadt/Sandbox.jl that referenced this issue Nov 21, 2022
maleadt added a commit to maleadt/Sandbox.jl that referenced this issue Nov 21, 2022
@staticfloat
Copy link
Member

I added the fix for force == true in #39906, but I see now that we should indeed do this regardless of whether force is set to true or not. Fix in #47668

@maleadt
Copy link
Member Author

maleadt commented Nov 29, 2022

Fixed now.

FWIW, there's even corner cases that rm doesn't handle 🙂

shell> rm -rf /tmp/jl_MNDexY
rm: cannot remove '/tmp/jl_MNDexY/work/home/pkgeval/.julia/compiled/work': Permission denied

@maleadt maleadt closed this as completed Nov 29, 2022
@maleadt
Copy link
Member Author

maleadt commented Mar 31, 2023

Another case:

julia> mkpath("/tmp/work/work")
"/tmp/work/work"

julia> touch("/tmp/work/work/work")
"/tmp/work/work/work"

julia> chmod("/tmp/work/work", 0o200)
"/tmp/work/work"

julia> rm("/tmp/work"; recursive=true)
ERROR: IOError: rm("/tmp/work/work"): directory not empty (ENOTEMPTY)
Stacktrace:
 [1] uv_error
   @ ./libuv.jl:100 [inlined]
 [2] rm(path::String; force::Bool, recursive::Bool)
   @ Base.Filesystem ./file.jl:306
 [3] rm(path::String; force::Bool, recursive::Bool)
   @ Base.Filesystem ./file.jl:294
 [4] top-level scope
   @ REPL[22]:1

shell> rm -rf /tmp/work
rm: cannot remove '/tmp/work/work': Permission denied

Since rm doesn't handle this either, I'm not entirely sure we should. But that means that with overlayfs mounts we always need to chmod before removing the workdir (cc @staticfloat).

@staticfloat
Copy link
Member

Yeah, when dealing with overlay mounts, using chmod is what I typically do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior filesystem Underlying file system and functions that use it
Projects
None yet
Development

No branches or pull requests

2 participants