Project

General

Profile

Backport #8240

SSLSocket breaks other connections or files on GC

Added by shugo (Shugo Maeda) over 6 years ago. Updated over 6 years ago.

Status:
Closed
Priority:
Normal
[ruby-core:54132]

Description

When an OpenSSL::SSL::SSLSocket is recycled by GC, SSL_shutdown() is called,
and SSL_shutdown() sends a close-notify alert message.
However at the GC time, the original socket might have already been closed,
and thus its file descriptor might be reused for another socket or file.

This problem can be reproduced as follows:

$ cat t.rb
require "socket"
require "openssl"

loop do
sock = TCPSocket.new("localhost", 443)
GC.start
ssl = OpenSSL::SSL::SSLSocket.new(sock)
ssl.connect
sock.close
end
$ ruby -v t.rb
ruby 2.1.0dev (2013-04-08 trunk 40183) [i686-linux]
t.rb:8:in connect': SSL_connect SYSCALL returned=5 errno=0 state=unknown state (OpenSSL::SSL::SSLError)
from t.rb:8:in
block in '
from t.rb:4:in loop'
from t.rb:4:in
'

An SSLError is raised because a close-notify alert message is sent to the server by GC
instead of a client hello message.
If the file descriptor is reused for a file, not a socket, the file would get broken.
This problem occurs rarely, but its impact is very serious.

IMHO, the free function of a DATA object should not do any task other than resource release.

Furthermore, SSLSocket#close calls SSL_shutdown(), but the original socket might have been closed,
in which case SSL_shutdown() (and @io.close) should not be called either.

The attached patch fixes these problems.


Files

openssl_invalid_shutdown_fix.diff (1.83 KB) openssl_invalid_shutdown_fix.diff shugo (Shugo Maeda), 04/09/2013 04:12 PM

Related issues

Related to Ruby master - Bug #7584: Ruby hangs when shutting down an ssl connection in gc finalizationClosed12/18/2012Actions
Related to Backport193 - Backport #8267: Please backport r40304 to avoid invalid SSL_shutdown()Closed04/15/2013Actions

Associated revisions

Revision d6b1ab91
Added by emboss over 6 years ago

  • ext/openssl/ossl_ssl.c: Correct shutdown behavior w.r.t GC.

  • test/openssl/test_ssl.rb: Add tests to verify correct behavior.

[Bug #8240] Patch provided by Shugo Maeda. Thanks!

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

Revision 40304
Added by emboss over 6 years ago

  • ext/openssl/ossl_ssl.c: Correct shutdown behavior w.r.t GC.

  • test/openssl/test_ssl.rb: Add tests to verify correct behavior.

[Bug #8240] Patch provided by Shugo Maeda. Thanks!

Revision 40304
Added by emboss over 6 years ago

  • ext/openssl/ossl_ssl.c: Correct shutdown behavior w.r.t GC.

  • test/openssl/test_ssl.rb: Add tests to verify correct behavior.

[Bug #8240] Patch provided by Shugo Maeda. Thanks!

Revision 40304
Added by emboss over 6 years ago

  • ext/openssl/ossl_ssl.c: Correct shutdown behavior w.r.t GC.

  • test/openssl/test_ssl.rb: Add tests to verify correct behavior.

[Bug #8240] Patch provided by Shugo Maeda. Thanks!

Revision 40304
Added by emboss over 6 years ago

  • ext/openssl/ossl_ssl.c: Correct shutdown behavior w.r.t GC.

  • test/openssl/test_ssl.rb: Add tests to verify correct behavior.

[Bug #8240] Patch provided by Shugo Maeda. Thanks!

Revision 40304
Added by emboss over 6 years ago

  • ext/openssl/ossl_ssl.c: Correct shutdown behavior w.r.t GC.

  • test/openssl/test_ssl.rb: Add tests to verify correct behavior.

[Bug #8240] Patch provided by Shugo Maeda. Thanks!

Revision 40304
Added by emboss over 6 years ago

  • ext/openssl/ossl_ssl.c: Correct shutdown behavior w.r.t GC.

  • test/openssl/test_ssl.rb: Add tests to verify correct behavior.

[Bug #8240] Patch provided by Shugo Maeda. Thanks!

Revision 2e607b7f
Added by nagachika (Tomoyuki Chikanaga) over 6 years ago

merge revision(s) 40304: [Backport #8240]

    * ext/openssl/ossl_ssl.c: Correct shutdown behavior w.r.t GC.

    * test/openssl/test_ssl.rb: Add tests to verify correct behavior.
    [Bug #8240] Patch provided by Shugo Maeda. Thanks!

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

Revision 40387
Added by nagachika (Tomoyuki Chikanaga) over 6 years ago

merge revision(s) 40304: [Backport #8240]

* ext/openssl/ossl_ssl.c: Correct shutdown behavior w.r.t GC.

* test/openssl/test_ssl.rb: Add tests to verify correct behavior.
[Bug #8240] Patch provided by Shugo Maeda. Thanks!

Revision 5db522e5
Added by usa (Usaku NAKAMURA) over 6 years ago

merge revision(s) 40304: [Backport #8267]

    * ext/openssl/ossl_ssl.c: Correct shutdown behavior w.r.t GC.

    * test/openssl/test_ssl.rb: Add tests to verify correct behavior.
    [Bug #8240] Patch provided by Shugo Maeda. Thanks!

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

History

Updated by bpot (Bob Potter) over 6 years ago

Cool! This should also resolve #7584

#2

Updated by Anonymous over 6 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r40304.
Shugo, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • ext/openssl/ossl_ssl.c: Correct shutdown behavior w.r.t GC.

  • test/openssl/test_ssl.rb: Add tests to verify correct behavior.

[Bug #8240] Patch provided by Shugo Maeda. Thanks!

#3

Updated by nagachika (Tomoyuki Chikanaga) over 6 years ago

  • Tracker changed from Bug to Backport
  • Project changed from Ruby master to Backport200
  • Category deleted (ext)
  • Status changed from Closed to Assigned
  • Assignee changed from MartinBosslet (Martin Bosslet) to nagachika (Tomoyuki Chikanaga)
  • Target version deleted (2.1.0)
#4

Updated by nagachika (Tomoyuki Chikanaga) over 6 years ago

  • Status changed from Assigned to Closed

This issue was solved with changeset r40387.
Shugo, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


merge revision(s) 40304: [Backport #8240]

* ext/openssl/ossl_ssl.c: Correct shutdown behavior w.r.t GC.

* test/openssl/test_ssl.rb: Add tests to verify correct behavior.
[Bug #8240] Patch provided by Shugo Maeda. Thanks!

Also available in: Atom PDF