Skip to content

Commit

Permalink
Fix OpenSSL::SSL::Socket shutdown logic
Browse files Browse the repository at this point in the history
  • Loading branch information
@jhass
jhass committed May 30, 2016
1 parent 4feb758 commit d5fe700
Showing 1 changed file with 25 additions and 4 deletions.
29 changes: 25 additions & 4 deletions src/openssl/ssl/socket.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,31 @@ class OpenSSL::SSL::Socket

begin
loop do
ret = LibSSL.ssl_shutdown(@ssl)
break if ret == 1
raise OpenSSL::SSL::Error.new(@ssl, ret, "SSL_shutdown") if ret < 0
# ret == 0, retry
begin
ret = LibSSL.ssl_shutdown(@ssl)
break if ret == 1
raise OpenSSL::SSL::Error.new(@ssl, ret, "SSL_shutdown") if ret < 0

This comment has been minimized.

Sign in to view

Copy link
@asterite

asterite May 30, 2016

Member

@jhass raising an exception and catching is pretty expensive. Can we maybe check this without raising an exception? It might be a bit uglier/longer, but maybe speed is more important here.

This comment has been minimized.

Sign in to view

Copy link
@jhass

jhass May 30, 2016

Author Member

Did you look at the logic in OpenSSL::SSL:Error? Are you really sure you want to duplicate it?

This comment has been minimized.

Sign in to view

Copy link
@jhass

jhass May 30, 2016

Author Member

The other issue is that afaik you can't call LibSSL.ssl_get_error twice, neither LibCrypto.err_get_error. So if we fetch it here, we probably need the logic a third time in OpenSSL::SSL:Error taking the already fetched values...

This comment has been minimized.

Sign in to view

Copy link
@asterite

asterite May 30, 2016

Member

Don't worry, let's leave it like this. If we later find a way to optimize it (if we find it causes slowdowns) then we'll fix it then.

This comment has been minimized.

Sign in to view

Copy link
@ysbaddaden

ysbaddaden May 30, 2016

Contributor

The performance hit is only raising & rescueing, no?

begin
  case ret = LibSSL.ssl_shutdown(@ssl)
  when 1
    break
  when 0
    # retry
  else
    ex = OpenSSL::SSL::Error.new(@ssl, ret, "SSL_shutdown")
    raise ex unless ex.error.want_read? || ex.error.want_write?
  end
rescue e : Errno
  # ...
end

This comment has been minimized.

Sign in to view

Copy link
@jhass

jhass May 30, 2016 •
edited
Loading

Author Member

That's a bit better yeah, feel free to push from my side. However the common case will be that Errno is raised from OpenSSL::SSL::Error.new. Perhaps we should return either and override new instead, though OpenSSL::SSL::Error.new returning OpenSSL::SSL::Error|Errno is a bit ugly too.

This comment has been minimized.

Sign in to view

Copy link
@asterite

asterite May 30, 2016

Member

@ysbaddaden Yes, though creating an exception builds a backtrace from the stack. But I think catching an exception is more expensive because the stack needs to be walked and checked (as far as I remember, @waj knows this much better). But yes, this way should be slightly faster than always raising.

This comment has been minimized.

Sign in to view

Copy link
@ysbaddaden

ysbaddaden May 30, 2016

Contributor

Argh, I expected the backtrace to only be generated when raising, not when constructing the object. I verified and Exception.new already builds the callstack. Maybe we would like to change that?

This comment has been minimized.

Sign in to view

Copy link
@asterite

asterite May 30, 2016

Member

Yeah, I think it should be harmless, and in fact more correct:

def foo
  Exception.new
end

def bar(ex)
  raise ex
end

ex = foo
bar(ex)

Ruby says:

foo.cr:6:in `bar': Exception (Exception)
    from foo.cr:10:in `<main>'

Crystal says:

 (Exception)
[4466977538] *CallStack::unwind:Array(Pointer(Void)) +82
[4466977441] *CallStack#initialize<CallStack>:Array(Pointer(Void)) +17
[4466977400] *CallStack::new:CallStack +40
[4466982204] *Exception#initialize<Exception>:CallStack +28
[4466982140] *Exception::new:Exception +92
[4466972153] *foo:Exception +9
[4466961818] __crystal_main +890
[4466971976] main +40

I'll change it, should be pretty easy.
rescue e : Errno
case e.errno
when 0
# OpenSSL claimed an underlying syscall failed, but that didn't set any error state,
# assume we're done
break
when Errno::EAGAIN
# Ignore, shutdown did not complete yet
else
raise e
end
rescue e : OpenSSL::SSL::Error
case e.error
when .want_read?, .want_write?
# Ignore, shutdown did not complete yet
else
raise e
end
end

# ret == 0, retry, shutdown is not complete yet
end
rescue IO::Error
ensure
Expand Down

0 comments on commit d5fe700

Please sign in to comment.