Feature #20971: Deprecate `rb_path_check`

• Status: Assigned
• Assignee: matz (Yukihiro Matsumoto)
• Target version: -

[Earlopain (Earlopain _)]

With #16131, various code around $SAFE, taint, etc. has been deprecated and removed. GH PR https://github.com/ruby/ruby/pull/2476.

Now, rb_path_check still exists as part of the public API, with Ruby itself never using or testing it. I believe it should have been deprecated and was simply missed. Should it be deprecated today or is that not worth the effort?

Docs for it are pretty vague: https://github.com/ruby/ruby/blob/33f95d632dce42fac35da29eaed33f0a5a4f0dcb/include/ruby/internal/intern/hash.h#L289-L297

This function is mysterious. What it does is not immediately obvious. Also what it does seems platform dependent.

[nobu (Nobuyoshi Nakada)]

I think that rb_path_check is a separate thing from "object taintedness", and that the removal of path_tainted_p in env_aset in despite of it is in !OBJ_TAINTED side was unintentional.

@jeremyevans0 (Jeremy Evans) What do you think?

[jeremyevans0 (Jeremy Evans)]

From looking at the history, rb_path_check was originally used by rb_env path_tainted, presumably to check whether the PATH environment variable was tainted, so as not to trust it. That may be why it is in the hash.h header instead of the file.h header, even though it makes no sense in the hash.h header.

rb_path_check was used in MJIT, to check for unsafe header files, but that was the last usage I could see in CRuby. I'm guessing that is a reason it may not have been deprecated/removed when $SAFE/taint was deprecated/removed.

If we can do a gem codesearch for rb_path_check, and nothing important comes up, I am in favor of deprecating and then removing it. I looked through the first 5 pages of GitHub results and nothing of note came up, other than the fact that TruffleRuby does not implement the function.

[nobu (Nobuyoshi Nakada)]

I mean this warning went away since 2.7, which should be unrelated to tainted-ness.

$ ruby2.6 -w -rtmpdir -e 'Dir.mktmpdir("", "/tmp") {|w| File.chmod(0777, w); ENV["PATH"] = w}'
-e:1: warning: Insecure world writable dir /tmp/20241220-14749-10itz6k in PATH, mode 040777

[jeremyevans0 (Jeremy Evans)]

nobu (Nobuyoshi Nakada) wrote in #note-3:

I mean this warning went away since 2.7, which should be unrelated to tainted-ness.

I see what you mean. I would be OK with adding that warning back.

[Earlopain (Earlopain _)]

Interesting, I thought the removal of that warning was intentional, do other languages have similar warnings? I guess it doesn't matter if it already used to do that 🤷

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Assigned
• Assignee set to matz (Yukihiro Matsumoto)

I think it would be OK to remove that warning (and rb_patch_check etc totally), if it is recognized officially.

@matz (Yukihiro Matsumoto) What do you think?

[Eregon (Benoit Daloze)]

That warning is pretty annoying in e.g. single-user Docker images where permissions might be 777 for convenience yet without real danger.
See https://github.com/actions/virtual-environments/issues/267 for a real issue caused by the warning, worked around with https://github.com/ruby/ruby-builder/blob/master/.github/workflows/build.yml#L96

[Earlopain (Earlopain _)]

Yes, I am also coming from the background of docker. Ruby is being build with ENABLE_PATH_CHECK=0 to supress the warning, similar to the workflow shared above, and I noticed it doesn't actually do anything anymore on recent versions.

[matz (Yukihiro Matsumoto)]

As a UNIX user from ancient time, I feel a world writable directory is too dangerous to allow. But if everyone is OK to accept the new situation (especially with virtual environments), I don't strongly object.

Matz.

[Earlopain (Earlopain _)]

PR https://github.com/ruby/ruby/pull/12542

[nobu (Nobuyoshi Nakada)]

I heard that that warning would be problematic on WSL as well, Windows directories cannot be changed from 777.