Bug #15318: net/imap socket backward compatibility broken in ruby 2.5+

• Status: Rejected
• Assignee: shugo (Shugo Maeda)
• Target version: -
• Backport: 2.3: UNKNOWN, 2.4: UNKNOWN, 2.5: UNKNOWN

[karis10 (bug reporter)]

Hi,
I use net/pop and net/imap libraries and sometimes I need my pop/imap connections to be socksified or proxified (through TCPSocket initialize method modifications: https://github.com/Shopify/socksify-ruby). Upgrading to ruby 2.5.3 from 2.4.4 I've found that TCPSocket interface was changed in favor of raw sockets by this patch: https://bugs.ruby-lang.org/projects/ruby-trunk/repository/revisions/58549/diff/lib/net/imap.rb the reason was this issue: https://bugs.ruby-lang.org/issues/13379

And after migrating from 2.4.4 to 2.5.3 and a bit of debugging I've found that standard library behavior was internally silently broken. In 2.5.3 net/pop library still can be normally proxified, but not net/imap.

[karis10 (bug reporter)]

• Subject changed from net/imap socket backward compatibility broken between in ruby 2.5+ to net/imap socket backward compatibility broken in ruby 2.5+

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Feedback
• Assignee set to shugo (Shugo Maeda)

Could you elaborate how it is broken?

[ahorek (Pavel Rosický)]

https://github.com/ruby/ruby/commit/5bf395f4cbd43bf64abcf6cc19daf834d2a02046#diff-4b4baaf75652a369c39b0b76e81ad54d

Hi, the mentioned change was reverted and never released in any stable version.

[karis10 (bug reporter)]

ahorek (Pavel Rosický) wrote:

https://github.com/ruby/ruby/commit/5bf395f4cbd43bf64abcf6cc19daf834d2a02046#diff-4b4baaf75652a369c39b0b76e81ad54d

Hi, the mentioned change was reverted and never released in any stable version.

I was talking about this exactly change:

<       @sock = TCPSocket.open(@host, @port)
>       @sock = tcp_socket(@host, @port)

It's available in ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux] installed through rvm.

[karis10 (bug reporter)]

nobu (Nobuyoshi Nakada) wrote:

Could you elaborate how it is broken?

Sure.

require 'net/imap'
require 'net/pop'
require 'socksify'

Socksify::debug = true
Socksify::proxy('127.0.0.1', 1080) do |_|
  # in ruby 2.4.5p335 (2018-10-18 revision 65137) [x86_64-linux] connection will go throgh socks
  # in ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux] connection will be from current host, omitting socks without any error or warning
  imap = Net::IMAP.new('imap.host.com', 143)

  # in ruby 2.4.5p335 (2018-10-18 revision 65137) [x86_64-linux] connection will go throgh socks
  # in ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux] connection will go throgh socks
  pop = Net::POP3.new('pop3.host.com', 110)
end

[ahorek (Pavel Rosický)]

ok, IMAP uses Socket.tcp instead of TCPSocket, so if you patch TCPSocket directly it won't work anymore.

the intention was to get rid of Timeout.timeout {} and make these libraries fully async

there are simmilar pending changes for POP3 and other protocols:
https://bugs.ruby-lang.org/issues/12928
https://bugs.ruby-lang.org/issues/12435
I think the main blocker is that getaddrinfo isn't async and resolv.rb is much slower.

[karis10 (bug reporter)]

ahorek (Pavel Rosický) wrote:

ok, IMAP uses Socket.tcp instead of TCPSocket, so if you patch TCPSocket directly it won't work anymore.

the intention was to get rid of Timeout.timeout {} and make these libraries fully async

there are simmilar pending changes for POP3 and other protocols:
https://bugs.ruby-lang.org/issues/12928
https://bugs.ruby-lang.org/issues/12435
I think the main blocker is that getaddrinfo isn't async and resolv.rb is much slower.

I understand your intentions and don't know what are you going to do next, but it's not a good decision to create and then to break internal interfaces which is base for other programmers to override and extend standard library.

[ahorek (Pavel Rosický)]

It wasn't intentional, but I think you shouldn't depend on TCPSocket anyway.
1/ Net::IMAP doesn't expose @socket in a public api
2/ it isn't documented that Net::IMAP will always use TCPSocket

@nobu (Nobuyoshi Nakada) what's your opinion? we should at least unify net/* libraries.

any idea how to help socksify to use this pattern without patching TCPSocket directly?

Socksify::proxy('127.0.0.1', 1080) do |_|
  imap = Net::IMAP.new('imap.host.com', 143)
end

#12435 will also break other protocols and I think we can't avoid that without changing socksify's code...

[karis10 (bug reporter)]

ahorek (Pavel Rosický) wrote:

It wasn't intentional, but I think you shouldn't depend on TCPSocket anyway.
1/ Net::IMAP doesn't expose @socket in a public api
2/ it isn't documented that Net::IMAP will always use TCPSocket

One more comment about it if you don't mind. Internal interfaces being created for some reason, and there was a reason to create TCPSocket, such as proxification. We are talking about standard library, not ruby internal structures which is understandable can be rewritten and refactored. So it's normal to override and extend even standard library code through interface, it's how ruby offers code reuse and DRY. You are right, there is no documentation about TCPSocket and any guarantees it will remain so, but it's a bit late to break something after keeping it in a known way until 2.5 release. Thank you.

[normalperson (Eric Wong)]

[e-mail] wrote:

https://bugs.ruby-lang.org/issues/15318

"Socket.tcp" been part of the public Ruby API for around 10 years
already. socksify should be supporting it in addition to TCPSocket;
because there is code which uses Socket directly.

It should've been wrapping all calls to Socket.new and maybe
Socket.for_fd to intercept the AF_INET* && SOCK_STREAM ones, too.

[shugo (Shugo Maeda)]

• Status changed from Feedback to Rejected

normalperson (Eric Wong) wrote:

"Socket.tcp" been part of the public Ruby API for around 10 years
already. socksify should be supporting it in addition to TCPSocket;
because there is code which uses Socket directly.

Agreed.

@sock is a private API and Socket is recommended over TCPSocket, so I wouldn't like to
revert the change.