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

File::Stat#executable? returns true when it shouldn't with root user #2690

Closed
dentarg opened this issue Jul 11, 2022 · 7 comments
Closed

File::Stat#executable? returns true when it shouldn't with root user #2690

dentarg opened this issue Jul 11, 2022 · 7 comments
Assignees
Labels

Comments

@dentarg
Copy link

dentarg commented Jul 11, 2022

TruffleRuby

(Started with docker run --rm -it -v $(pwd):/app -w /app ghcr.io/flavorjones/truffleruby:nightly-slim bash, it said Digest: sha256:97f4726094dee47d3e516c36f2bc0bf3980243017f0aa3d32fab4b1346e4f4ce in the output)

root@6536ed695d52:/app# ruby -v; ruby -rtempfile -e 'p File.stat(Tempfile.new.path).executable?'
truffleruby 22.3.0-dev-e01a7732, like ruby 3.0.3, GraalVM CE Native [x86_64-linux]
true

root@6536ed695d52:/app# touch /tmp/foo
root@6536ed695d52:/app# stat /tmp/foo
  File: /tmp/foo
  Size: 0         	Blocks: 0          IO Block: 4096   regular empty file
Device: 8fh/143d	Inode: 3711856     Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2022-07-11 09:21:24.795629000 +0000
Modify: 2022-07-11 09:21:24.795629000 +0000
Change: 2022-07-11 09:21:24.795629000 +0000
 Birth: 2022-07-11 09:21:24.795629000 +0000
root@6536ed695d52:/app# ruby -v; ruby -rtempfile -e 'p File.stat("/tmp/foo").executable?'
truffleruby 22.3.0-dev-e01a7732, like ruby 3.0.3, GraalVM CE Native [x86_64-linux]
true

MRI

(docker run --rm -it -v $PWD:/app -w /app ruby:3.0.3 bash, Digest: sha256:7c57b474163e01f1518ff830dffef023fbd014378edd414526562137edc1400f)

root@e24edb537478:/app# ruby -v; ruby -rtempfile -e 'p File.stat(Tempfile.new.path).executable?'
ruby 3.0.3p157 (2021-11-24 revision 3fb7d2cadc) [x86_64-linux]
false

root@e24edb537478:/app# touch /tmp/foo
root@e24edb537478:/app# stat /tmp/foo
  File: /tmp/foo
  Size: 0         	Blocks: 0          IO Block: 4096   regular empty file
Device: 99h/153d	Inode: 2265881     Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2022-07-11 09:23:34.345172000 +0000
Modify: 2022-07-11 09:23:34.345172000 +0000
Change: 2022-07-11 09:23:34.345172000 +0000
 Birth: 2022-07-11 09:23:34.345172000 +0000
root@e24edb537478:/app# ruby -v; ruby -rtempfile -e 'p File.stat("/tmp/foo").executable?'
ruby 3.0.3p157 (2021-11-24 revision 3fb7d2cadc) [x86_64-linux]
false
@eregon
Copy link
Member

eregon commented Jul 11, 2022

I cannot reproduce outside Docker:

$ touch /tmp/foo
$ jt -q ruby -v -rtempfile -e 'p File.stat("/tmp/foo").executable?'
truffleruby 22.3.0-dev-e01a7732, like ruby 3.0.3, Interpreted JVM [x86_64-linux]
false

@eregon
Copy link
Member

eregon commented Jul 11, 2022

But I can indeed reproduce with your instructions in Docker.
Running ruby as root is rarely a good idea though, that is probably what is causing the issue here.

In general I recommend trying outside Docker (e.g. use ruby-build to install truffleruby-dev), to avoid this root issue, it is quite different than what GitHub Actions does which does not use the root user.

@eregon
Copy link
Member

eregon commented Jul 11, 2022

This is a bug we inherited from Rubinius: https://github.com/rubinius/rubinius/blob/b7a755c83f3dd3f0c1f5e546f0e58fb61851ea44/core/stat.rb#L158
Looks like a mistake when porting https://github.com/ruby/ruby/blob/fae568edbe61f816237aab230ced2ecc07135c45/file.c#L6025-L6047

@andrykonchin Could you fix this?

BTW, File.executable?(path) is preferable in general to File.stat(path).executable? because it's faster and does not involve allocations. It can also be implemented more simply with eaccess()/access() (we should do that in TruffleRuby, like CRuby does it).

@eregon eregon added the bug label Jul 11, 2022
@eregon eregon changed the title File::Stat#executable? returns true when it shouldn't? File::Stat#executable? returns true when it shouldn't with root user Jul 11, 2022
@eregon
Copy link
Member

eregon commented Jul 11, 2022

Actually access() might slower than stat(), b250992, but access/eaccess is convenient here to check the right bit for us, so we should check if that's faster than truffleposix_stat_mode() + checking the bits. Either should be faster than going through File::Stat.

@andrykonchin
Copy link
Member

@eregon Yes, will work on it.

@eregon
Copy link
Member

eregon commented Jul 13, 2022

It's interesting actually, root can read or write any file, but it can only execute a file if one of the executable bits is set (no matter if it's on the user and root does not own the file), from my reading of CRuby's code handling this.

Regarding performance, it seems stat() is actually fastest on macOS like on Linux (compared to access/eaccess), probably due to being better cached.

@andrykonchin
Copy link
Member

Fixed in 78264ff. Thank you for reporting.

@eregon eregon added this to the 22.3.0 milestone Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants