Feature #17994
openClarify `IO.read` behavior and add `File.read` method
Description
IO.read
creates a subprocess when a given file name starts with a |
character.
irb(main):001:0> IO.read("| ls /etc/passwd")
=> "/etc/passwd\n"
To disable this feature, File.read
can be used.
irb(main):002:0> File.read("| ls /etc/passwd")
(irb):2:in `read': No such file or directory @ rb_sysopen - | ls /etc/passwd (Errno::ENOENT)
So, as far as I know, File.read
is more prefereable to IO.read
if a user want to just read a file.
However, in terms of the implementation, there is no definition of File.read
. File.read
invokes IO.read
because IO
is a superclass of File
, and IO.read
creates a subprocess only when its receiver is exactly the IO
class.
I think there are two problems in the current situation:
- The rdoc of
IO.read
does not explain the behavior to disable a subprocess invocation. - The rdoc does not have an entry for
File.read
.
I've created a PR to address the two issues by clarifying IO.read
behavior and defining File.read
as an alias to IO.read
.
Updated by jeremyevans0 (Jeremy Evans) over 3 years ago
My only concern is that File isn't the only subclasses of IO. There are also:
UNIXServer
UNIXSocket
UDPSocket
TCPServer
TCPSocket
IPSocket
Socket
BasicSocket
Do we want Socket.read
to handle |
specially (as IO.read
does), or should it operate as File.read
(as it does now)? If it currently works as File.read
, then switching to IO.read
behavior could potentially result in security issues. I doubt there is significant real-world usage of Socket.read
or similar, so maybe this is not a significant concern.
Honestly, I don't think read
makes sense as a class method on socket classes, so maybe we can avoid the issue by undefing it (after a deprecation period).
Updated by mame (Yusuke Endoh) over 3 years ago
Jeremy, thanks you for your comment.
jeremyevans0 (Jeremy Evans) wrote in #note-1:
Honestly, I don't think
read
makes sense as a class method on socket classes, so maybe we can avoid the issue by undefing it (after a deprecation period).
It makes sense. Because undefing all subclasses is a bit annoying (and we cannot change user-defined subclasses of IO), it may be good for IO.read
to raise an exception when the reciever is neither IO
or File
. I'd like to bring the deprecation to the agenda of the next dev-meeting.
BTW, do you have any concern against merging my current PR? It just clarifies the current behavior of IO.read and adds File.read
alias. We can implement the deprecation warnings later if it is approved.
Updated by jeremyevans0 (Jeremy Evans) over 3 years ago
mame (Yusuke Endoh) wrote in #note-2:
BTW, do you have any concern against merging my current PR? It just clarifies the current behavior of IO.read and adds
File.read
alias. We can implement the deprecation warnings later if it is approved.
Apologies, I should have reviewed the PR before responding. I see that it does not change the behavior, so I am in favor of merging it.
Updated by akr (Akira Tanaka) over 3 years ago
I feel adding File.read method is uselessly complex behavior:
Socket.read (no pipe) inherits IO.read (support pipe) but behaves as File.read (no pipe).
Also, adding method require more document maintenance cost.
So, I think updating the document of IO.read is best way to go.
I think disabling Socket.read has very few benefit over its effect:
I guess someone would propose disabling other class methods as well.
Updated by mame (Yusuke Endoh) over 3 years ago
I noticed that this issue is not only in IO.read
but also in IO.binread
, IO.write
, IO.binwrite
, and IO.readlines
.
Adding File.binread
etc. is clearly hard to maintain, so I agree that we should change only the doc of IO.read
etc.
I have no strong opinion about prohibiting Socket.read etc. IO.read
is a tricky class method because it changes its behavior depending on its receiver class, so it makes sense if the receiver is neither IO or File, but I'd like to keep the behavior if anyone is against the change.