New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes issue #549 (OpenSSL sockets spin indefinitely on timeout in handshake) #554

Merged
merged 1 commit into from Mar 16, 2013

Conversation

Projects
None yet
3 participants
@robinmessage

robinmessage commented Feb 27, 2013

This pull request fixes Issue #549 (OpenSSL sockets spin indefinitely on timeout in handshake)

The only place that uses the return value of flushData can go into an infinite loop, and does so in production.

Just swapping the return values from flushData fixes the problem, and doesn't have any knock on effects we can find.

@BanzaiMan

This comment has been minimized.

Show comment
Hide comment
@BanzaiMan

BanzaiMan Mar 15, 2013

Member

It would be great if you could come up with a test case. If not, I'd try.

Member

BanzaiMan commented Mar 15, 2013

It would be great if you could come up with a test case. If not, I'd try.

@robinmessage

This comment has been minimized.

Show comment
Hide comment
@robinmessage

robinmessage Mar 15, 2013

Hmm, I thought I had a working test case (below), which hangs indefinitely despite the timeout. However, the fix doesn't seem to fix this reliably, so there may be other issues.

I'll try and find a better reproduction. Meanwhile, could someone have a look at the logic of this patch as it would be nice to be able to get our production servers back onto the released gem?

require 'openssl'
require 'net/https'

# Run `nc -l 1234` in another window
client = Net::HTTP.new('localhost', 1234)
client.use_ssl = true
client.read_timeout = 1
client.open_timeout = 1

client.get("/")

robinmessage commented Mar 15, 2013

Hmm, I thought I had a working test case (below), which hangs indefinitely despite the timeout. However, the fix doesn't seem to fix this reliably, so there may be other issues.

I'll try and find a better reproduction. Meanwhile, could someone have a look at the logic of this patch as it would be nice to be able to get our production servers back onto the released gem?

require 'openssl'
require 'net/https'

# Run `nc -l 1234` in another window
client = Net::HTTP.new('localhost', 1234)
client.use_ssl = true
client.read_timeout = 1
client.open_timeout = 1

client.get("/")
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 16, 2013

Member

@robinmessage I tried your script and it did not appear to hang for me on JRuby master after several attempts.

ext-jruby-local ~/projects/jruby $ cat ossl_hang.rb 
require 'openssl'
require 'net/https'

# Run `nc -l 1234` in another window
client = Net::HTTP.new('localhost', 1234)
client.use_ssl = true
client.read_timeout = 1
client.open_timeout = 1

client.get("/")
ext-jruby-local ~/projects/jruby $ jruby ossl_hang.rb 
Timeout::Error: execution expired

I will have a look at the logic of the patch.

Member

headius commented Mar 16, 2013

@robinmessage I tried your script and it did not appear to hang for me on JRuby master after several attempts.

ext-jruby-local ~/projects/jruby $ cat ossl_hang.rb 
require 'openssl'
require 'net/https'

# Run `nc -l 1234` in another window
client = Net::HTTP.new('localhost', 1234)
client.use_ssl = true
client.read_timeout = 1
client.open_timeout = 1

client.get("/")
ext-jruby-local ~/projects/jruby $ jruby ossl_hang.rb 
Timeout::Error: execution expired

I will have a look at the logic of the patch.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 16, 2013

Member

@robinmessage Your patch does seem right...kinda one of those duh moments. The logic should be that it continues trying to flushData until there's none left to flush, so obviously it should return true when there's data left. We probably didn't run into it under normal circumstances because everything flushes ok.

I'll merge!

Member

headius commented Mar 16, 2013

@robinmessage Your patch does seem right...kinda one of those duh moments. The logic should be that it continues trying to flushData until there's none left to flush, so obviously it should return true when there's data left. We probably didn't run into it under normal circumstances because everything flushes ok.

I'll merge!

headius added a commit that referenced this pull request Mar 16, 2013

Merge pull request #554 from robinmessage/master
Fixes issue #549 (OpenSSL sockets spin indefinitely on timeout in handshake)

@headius headius merged commit 1c9c32f into jruby:master Mar 16, 2013

1 check passed

default The Travis build passed
Details
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 16, 2013

Member

I have pushed jruby-openssl-0.8.6 to RubyGems. Let me know how it feels.

Member

headius commented Mar 16, 2013

I have pushed jruby-openssl-0.8.6 to RubyGems. Let me know how it feels.

eregon added a commit that referenced this pull request Dec 27, 2017

Squashed 'spec/ruby/' changes from e2d0d1e..0fe33ac
0fe33ac Use the new platform_is pointer_size: SIZE guard
80a451e Use sizeof(VALUE) in C-API numeric specs
d89bf5e Limit leap second specs to Linux as they are not portable
d89c012 Allow more GC cycles for the reference to be vacated.
4864569 Fix indentation in language/case_spec.rb
6c7aa4e Add exclusion for RuboCop
3a94824 Move RuboCop to its own Travis CI job
cb7d7dd Add exclude for RuboCop
4c785e9 Add specs for $. verifying it only admits integer values.
f8ec979 Add spec for IO#syswrite with the fd set to nonblocking mode
411ba12 Add specs for case with interpolated regexps
79d5b55 Update "last match" specs to not be misleading ($9 is not an upper-bound).
01992ab Add specs for splat arg + block argument modifying the splat arg
48196e0 Fix Kernel#Complex spec
1afd800 Add missing space between functions
afbc53b Add specs for Bignum between max long and max unsigned long
256ab2c Add specs for rb_uint2inum
2886a6a Correct rescuing spelling in exception specs
8ec8e71 Add specs for ensuring proper handling of leap seconds.
eab3905 Add test for numeric arguments to File.utime.
bf54b1a spec_helper.rb: libruby.so iff enable-shared
a47e5de spec_helper.rb: must find libruby.so
2d980b6 Simplify Thread#[]= spec
0976280 fix threading bug.
4d57b63 Use syswrite to avoid potential buffering in IO#select spec
9dd2e86 Remove macOS on Travis CI
4dfad8a Update to latest releases
50765e3 Test with Ruby 2.5 on macOS
0013104 Add Ruby 2.5.0 to Travis CI
da06ed8 Merge pull request #572 from andrykonchin/range-initialize-no-longer-hides-exceptions
4fdb52c Struct: add spec for equality of similar classes
e42374e [Code review] Fix typo
01a68af Add specs for :keyword_init option of Struct.new
787a216 Add specs for Range#new
8622e98 Add specs for Method#===
1ba2970 Merge pull request #569 from andrykonchin/dir-glob-provides-new-optional-keyword-argument-base
7f32e5b Add specs for Dir.glob :base option
8079e3d Update specs to use frozen_error_class for FrozenError
db04c79 [Code review] Reduce the list of exceptions
4fa3652 [Code review] Change descriptions and add test case with raising not StandardError exceptions
0b2604e Add specs for Rational +, -, * and / when #coerce raises an exception
bd83c6d Add specs for Rational#<=> when #coerce raises an exception
fdeb25d Add specs for Float +, -, * and / when #coerce raises an exception
3a19bbc Add specs for Float <, <=, > and >= when #coerce raises an exception
7b8b88b Add specs for Integer +, -, *, / when #coerce raises an exception
4631f17 Add specs for Integer <,  <=, > and >= when #coerce raises an exception
36f98c7 Typo fix 😬
5dd77c8 Forgot guards again! 😬
07bc28a Add specs for Integer#{all|any|no}_bits?
db6c330 Add missed cases for String#casecmp?
82145c5 mathn is no longer part of the standard library
595645f Fix expectation for Hash#transform_keys!
4a07f3d Fix style
d35d74a Fix Struct#new to add the proper version guards
e8f89e9 Use the new frozen_error_class in Kernel#autoload spec
6c0655c The main Thread should have report_on_exception=true for consistency
dea8498 Set Thread.report_on_exception=true by default to report exceptions in Threads
3629c7d ignore lines (to catch up r61155).
43d24a4 struct.c: add keyword_init option to Struct.new
77deeeb catch up r61131.
efdfa74 Add link to Gitter
627151b Add specs for Time#at
e3286c0 Add specs for refinement and string interpolation
4a8d982 Update RuboCop to 0.52.0
69d18fe [Code review] Group test cases with context
ee54bf2 [Code review] Reorder test cases
0ddf2a4 Add specs for top-level constant lookup
6de75c0 Fix double spaces after #should
4135bc5 OpenBSD uses EISDIR for directory access
0fd75d2 OpenBSD doesn't have birthtime in File::Stat
7e0d43d OpenBSD uses EPROTONOSUPPORT instead of EAFNOSUPPORT
1b3ccfd OpenBSD uses US-ASCII for locale_charmap, similar to FreeBSD
1472757 OpenBSD does not allow getsid if pid is not in same session
b61b084 Merge pull request #548 from andrykonchin/set-to-s-and-include
317fd1b Improve specs for Hash#transform_values
be5eb4c Add specs for Hash#transform_keys [#547]
77228f8 Fix test label [ci-skip] [doc]
a820165 Add specs for Hash#slice [#552]
737999a Oops, forgot guards
2e3515f Fix spec. Definitely trying to go too fast today.
a609618 Fix spec
c4f267f Tweak specs
a487ab9 + String#delete_{suffix|prefix}
da46e71 Oops, fix file name
65c42e6 + Hash#slice
cd71d91 Finish specs for Hash#transform_keys!
4fb1f16 + Hash#transform_keys (incomplete).
75f3cad + Dir#children, Dir#each_child
fd0cb75 Fix b1acb45.
13c5170 These specs aren't doing what they are supposed to be doing [#554]
b1acb45 Enumerable#all? none? and one?: Basic specs [#550]
c76b47c Update specs for Enumerable#any? See ruby/ruby@a9770ba
7f24dcf [Code review] Use explicit exception class and remove excessive begin/end blocks
8289f63 [Code review] Add missing cases
c5ccbdf [Code review] Replace instance_eval with eval and correct SignalException test
17612fd Improve rescue/ensure specs
688e72f Correct test case for lazy checking of rescue expression
4ac8a7d Correct test case for rescue in method arguments
0253bc9 Add specs for rescue/ensure in do block
6faa445 Improve specs for Set#include?
d9422dc Add specs for Set#===
e8efda5 Add specs for Set#to_s
0c90201 Fix typos

git-subtree-dir: spec/ruby
git-subtree-split: 0fe33ac76d6c4f3666a357a1d80260c8c2facf26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment