Project

General

Profile

Feature #13379

[PATCH] safe IMAP connections

Added by ahorek (Pavel Rosický) about 2 years ago. Updated about 2 years ago.

Status:
Closed
Priority:
Normal
Target version:
-
[ruby-core:80440]

Description

Hi,
I found out that using the standard IMAP library isn't very safe. It can be forced to hang the whole application.

the problem is here

s = @sock.gets(CRLF)

-> the server accepted the connection but it didn't send any data. Now I need to reboot the server because my thread is blocked forever.

I have no other option but to use this

Timeout.timeout(timeout, Net::OpenTimeout) { Net::IMAP.new(host, port, ssl) }

which basically works, but I really don't want to create a new thread for each IMAP call, so I did these changes:
1/ replaced TCPSocket with Socket.tcp
2/ replaced sock.read and sock.gets with sock.read_nonblock

now it works as expected.

Patch
https://github.com/ruby/ruby/pull/1557

related issue, please consider merging this one as well
#12928


Files

imap.patch (3.29 KB) imap.patch ahorek (Pavel Rosický), 03/28/2017 10:34 PM
0002-raise-Net-OpenTimeout.patch (1.27 KB) 0002-raise-Net-OpenTimeout.patch ahorek (Pavel Rosický), 03/29/2017 08:23 PM
0001-ssl_socket_connect-for-imap.patch (1.01 KB) 0001-ssl_socket_connect-for-imap.patch ahorek (Pavel Rosický), 03/29/2017 08:23 PM

Associated revisions

Revision 8b51a725
Added by shugo (Shugo Maeda) about 2 years ago

net/imap: handle timeouts

Patch by Pavel Rosický. [Feature #13379] [ruby-core:80440]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@58549 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 58549
Added by shugo (Shugo Maeda) about 2 years ago

net/imap: handle timeouts

Patch by Pavel Rosický. [Feature #13379] [ruby-core:80440]

Revision 58549
Added by shugo (Shugo Maeda) about 2 years ago

net/imap: handle timeouts

Patch by Pavel Rosický. [Feature #13379] [ruby-core:80440]

Revision 58549
Added by shugo (Shugo Maeda) about 2 years ago

net/imap: handle timeouts

Patch by Pavel Rosický. [Feature #13379] [ruby-core:80440]

History

#1

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

  • Subject changed from Safe IMAP connections to [PATCH] safe IMAP connections

Updated by shugo (Shugo Maeda) about 2 years ago

  • Assignee set to shugo (Shugo Maeda)

ahorek (Pavel Rosický) wrote:

Hi,
I found out that using the standard IMAP library isn't very safe. It can be forced to hang the whole application.

the problem is here

s = @sock.gets(CRLF)

-> the server accepted the connection but it didn't send any data. Now I need to reboot the server because my thread is blocked forever.

I have no other option but to use this

Timeout.timeout(timeout, Net::OpenTimeout) { Net::IMAP.new(host, port, ssl) }

which basically works, but I really don't want to create a new thread for each IMAP call, so I did these changes:
1/ replaced TCPSocket with Socket.tcp
2/ replaced sock.read and sock.gets with sock.read_nonblock

now it works as expected.

Patch
https://github.com/ruby/ruby/pull/1557

Thanks for the patch, but it doesn't seem enough because:

  1. The :connect_timeout option of Socket.tcp doesn't work when DNS lookups by getaddrinfo block. A new thread by Timeout.timeout is needed to handle this case.
  2. TLS handshakes in start_tls_session may also block. Protocol#ssl_socket_connect can be used to handle this case.

And I'm not sure whether open_timeout should have the default value because a new thread is required.

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

Thanks for the review. I've fixed the second case.

The only blocker is getaddrinfo now. It's a lowlevel system api that always blocks. I'll take a look if I can rewrite the ruby api to use getaddrinfo_a or getaddrinfo_ex (no thread is required). It'll be in a separate request.

Updated by shugo (Shugo Maeda) about 2 years ago

ahorek (Pavel Rosický) wrote:

Thanks for the review. I've fixed the second case.

Thanks. The additional patches look fine.

The only blocker is getaddrinfo now. It's a lowlevel system api that always blocks. I'll take a look if I can rewrite the ruby api to use getaddrinfo_a or getaddrinfo_ex (no thread is required). It'll be in a separate request.

getaddrinfo_a() is glibc specific, so we need alternatives on other platforms such as FreeBSD.
Do you mean GetAddrInfoEx() of winsock by getaddrinfo_ex?

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

Yes, these APIs are very platform specific. I was just checking documentations and existing solutions. Maybe I'll use the Resolv api, but I need to check incompatibilities and also a performance impact.

Updated by normalperson (Eric Wong) about 2 years ago

pdahorek@seznam.cz wrote:

Yes, these APIs are very platform specific. I was just
checking documentations and existing solutions. Maybe I'll use
the Resolv api, but I need to check incompatibilities and also
a performance impact.

Yes, I actually prefer we use resolv more since it offers proper
timeout support whereas getaddrinfo does not.

Thanks.

#7

Updated by shyouhei (Shyouhei Urabe) about 2 years ago

  • Status changed from Open to Assigned
#8

Updated by shugo (Shugo Maeda) about 2 years ago

  • Status changed from Assigned to Closed

Applied in changeset trunk|r58549.


net/imap: handle timeouts

Patch by Pavel Rosický. [Feature #13379] [ruby-core:80440]

Updated by shugo (Shugo Maeda) about 2 years ago

I've merged your patch. Thank you.

Create another ticket for resolv if it's ready.

Also available in: Atom PDF