Feature #13379: [PATCH] safe IMAP connections

• Status: Closed
• Assignee: shugo (Shugo Maeda)
• Target version: -

[ahorek (Pavel Rosický)]

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

[ahorek (Pavel Rosický)]

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

[shugo (Shugo Maeda)]

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

[ahorek (Pavel Rosický)]

• File 0001-ssl_socket_connect-for-imap.patch 0001-ssl_socket_connect-for-imap.patch added
• File 0002-raise-Net-OpenTimeout.patch 0002-raise-Net-OpenTimeout.patch added

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.

[shugo (Shugo Maeda)]

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?

[ahorek (Pavel Rosický)]

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.

[normalperson (Eric Wong)]

[e-mail] 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.

[shyouhei (Shyouhei Urabe)]

• Status changed from Open to Assigned

[shugo (Shugo Maeda)]

• Status changed from Assigned to Closed

Applied in changeset trunk|r58549.

---

net/imap: handle timeouts

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

[shugo (Shugo Maeda)]

I've merged your patch. Thank you.

Create another ticket for resolv if it's ready.