Project

General

Profile

Bug #15318

net/imap socket backward compatibility broken in ruby 2.5+

Added by karis10 (bug reporter) over 2 years ago. Updated over 2 years ago.

Status:
Rejected
Priority:
Normal
Target version:
-
[ruby-core:89856]

Description

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.

#1

Updated by karis10 (bug reporter) over 2 years ago

  • 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+

Updated by nobu (Nobuyoshi Nakada) over 2 years ago

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

Could you elaborate how it is broken?

Updated by karis10 (bug reporter) over 2 years ago

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.

Updated by karis10 (bug reporter) over 2 years ago

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

Updated by ahorek (Pavel Rosický) over 2 years ago

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.

Updated by karis10 (bug reporter) over 2 years ago

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.

Updated by ahorek (Pavel Rosický) over 2 years ago

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...

Updated by karis10 (bug reporter) over 2 years ago

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.

Updated by normalperson (Eric Wong) over 2 years ago

karis10@f5foster.com 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.

Updated by shugo (Shugo Maeda) over 2 years ago

  • 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.

Also available in: Atom PDF