Add Dir.fchdir #7135
Add Dir.fchdir #7135
Conversation
| return rb_ensure(fchdir_yield, (VALUE)&args, fchdir_restore, (VALUE)&args); | ||
| } | ||
| else { | ||
| int r = (int)(VALUE)rb_thread_call_without_gvl(nogvl_fchdir, &fd, |
There was a problem hiding this comment.
Why do we yield the GVL here? can fchdir actually block for an appreciable amount of time on any actual system? Does it even do any I/O?
(I'm guessing the answer is "NFS" or something like that...)
In any case, if the block-not-given form yields the GVL to do the chdir, should the block-given form also do so?
There was a problem hiding this comment.
I don't know whether fchdir can block. Since I'm not sure it is nonblocking in all cases, I decided to be conservative and follow what we do for chdir.
In terms of the block form not yielding the GVL, I'm not sure why it doesn't do a nogvl call in the block case. If this was an issue for fchdir, it would be an issue for chdir as well.
| pwd = Dir.pwd | ||
| dir = Dir.new(DirSpecs.mock_dir) | ||
| @dirs << dir | ||
| Dir.fchdir(dir.fileno) { Dir.pwd }.should == DirSpecs.mock_dir |
There was a problem hiding this comment.
Sure. We can support that if people want.
There was a problem hiding this comment.
Rather I'd suggest making the argument Dir only and falling back to Dir.chidir(dir.path).
There was a problem hiding this comment.
The primary purpose of Dir.fchdir is to work with directory file descriptor integers. Currently we don't have Dir.fdopen (something to create a Dir object from a file descriptor integer), so requiring Dir objects would make it impossible to use this method with file descriptors passed through UNIX sockets or from parent processes.
Maybe we could make Dir.chdir accept Dir objects, and call Dir.fchdir with the file descriptor if directory file descriptors are supported, or use dir.path if not? That seems like it should be a separate pull request, though.
ab732e7 to
a1f699b
Compare
This is useful for passing directory file descriptors over UNIX sockets or to child processes to avoid TOCTOU vulnerabilities. The implementation follows the Dir.chdir code. This will raise NotImplementedError on platforms not supporting both fchdir and dirfd. Implements [Feature #19347]
This uses Dir.fchdir if supported, or Dir.chdir otherwise. Implements [Feature #19347]
This returns a Dir instance for the given directory file descriptor. If fdopendir is not supported, this raises NotImplementedError. Implements [Feature #19347]
| rescue NotImplementedError | ||
| false | ||
| rescue Exception | ||
| true |
There was a problem hiding this comment.
Should it be false here? If it raises an exception it's not supported, right?
There was a problem hiding this comment.
I changed it to false in ruby/spec: ruby/spec@c075975
There was a problem hiding this comment.
If it isn't supported, it should be raising NotImplementedError. If it raises another exception, that indicates it is supported, but the support is broken, which means the specs should rightfully fail for it.
I don't like the change in ruby/spec@c075975, because it allows a ruby implementation that advertises Ruby 3.3 version to not implement the method. If a ruby implementation says it implements Ruby 3.3, it must implement the method, even if the method always raises NotImplementedError.
There was a problem hiding this comment.
Note that at least one spec will fail in that case, because if has_fchdir=false, then the raises NotImplementedError spec will still fail if it's not a NotImplementedError.
But you are right, we shouldn't rescue NoMethodError there. That should be handled by guard or so.
I think we can actually just use Dir.respond_to?(:fchdir) and that will also correctly tell us if it's available on that platform or not. I'll do that change.
There was a problem hiding this comment.
(the trick is respond_to? returns false for a method defined as rb_notimplement)
There was a problem hiding this comment.
That works for me, thanks!
This is useful for passing directory file descriptors over UNIX sockets or to child processes to avoid TOCTOU vulnerabilities.
The implementation follows the Dir.chdir code.
This will raise NotImplementedError on platforms not supporting both fchdir and dirfd.