From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-2.8 required=3.0 tests=ALL_TRUSTED,AWL,BAYES_00, URIBL_BLOCKED shortcircuit=no autolearn=unavailable version=3.3.2 X-Original-To: yahns-public@yhbt.net Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 9C70220414 for ; Sat, 20 Feb 2016 02:54:31 +0000 (UTC) Date: Sat, 20 Feb 2016 02:54:31 +0000 From: Eric Wong To: yahns-public@yhbt.net Subject: [PATCH] fix output buffering with SSL_write Message-ID: <20160220025431.GA26844@dcvr.yhbt.net> References: <20160216111126.GA22532@dcvr.yhbt.net> <20160216111411.GA24123@dcvr.yhbt.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160216111411.GA24123@dcvr.yhbt.net> List-Id: The underlying SSL_write called by the OpenSSL socket when we use write_nonblock must get the same arguments after a call returns SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE. Ensure that by always passing a copy of the user-supplied buffer to OpenSSL::SSL::SSLSocket#write_nonblock and retaining our copy of the string internally as @ssl_blocked if we hit EAGAIN on the socket. String#dup is inexpensive in modern Ruby, as copying a non-embedded string is implemented using copy-on-write. We also prefer to use write_nonblock directly instead of using our kgio-dependent sendfile emulation layer to avoid allocating a new string on partial writes. ref: https://bugs.ruby-lang.org/issues/12085 http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/73882 http://mid.gmane.org/redmine.issue-12085.20160219020243.4b790a77f1cdd593@ruby-lang.org --- Eric Wong wrote: > --- a/lib/yahns/openssl_client.rb > +++ b/lib/yahns/openssl_client.rb > def kgio_trywrite(buf) > + ssl_blocked = @ssl_blocked > + if ! ssl_blocked.empty? && ssl_blocked.object_id != buf.object_id > + warn "bk: #{ssl_blocked.inspect}\n!= #{buf.inspect}" > + end > + warn "buf: #{buf.inspect}" if buf.bytesize <= 23 > + #warn "b: #{buf.object_id} #{buf.object_id == ssl_blocked.object_id}" > + buf = ssl_blocked.replace(buf) # buf and ssl_blocked may be the same obj > + buf.force_encoding Encoding::BINARY > rv = @ssl.write_nonblock(buf, exception: false) > - Integer === rv and > + case rv > + when :wait_readable, :wait_writable > + #warn "#{fileno} blocking #{buf.bytesize} #{buf.object_id}" > + return rv # do not clear ssl_blocked > + when Integer > rv = buf.bytesize == rv ? nil : buf.byteslice(rv, buf.bytesize) > + end > + ssl_blocked.clear The above call to String#clear interacts badly with the following: > --- a/lib/yahns/sendfile_compat.rb > +++ b/lib/yahns/sendfile_compat.rb > @@ -6,19 +6,29 @@ > module Yahns::SendfileCompat # :nodoc: > def trysendfile(io, offset, count) > case rv = kgio_trywrite(str) > when String # partial write, keep trying > - n += (str.size - rv.size) > + n += (str.bytesize - rv.size) > str = rv > when :wait_writable, :wait_readable > + if respond_to?(:ssl_blocked) > + #warn "empty: #{@ssl_blocked.empty?}" > + end > return n > 0 ? n : rv > when nil > - return n + str.size # yay! > + return n + str.bytesize # yay! > end while true > end > end ... because str.size becomes zero when String#clear got called Ooops :x lib/yahns/openssl_client.rb | 40 +++++++++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/lib/yahns/openssl_client.rb b/lib/yahns/openssl_client.rb index bf64255..cd7d210 100644 --- a/lib/yahns/openssl_client.rb +++ b/lib/yahns/openssl_client.rb @@ -1,3 +1,4 @@ +# -*- encoding: binary -*- # Copyright (C) 2013-2016 all contributors # License: GPL-3.0+ (https://www.gnu.org/licenses/gpl-3.0.txt) # frozen_string_literal: true @@ -7,8 +8,6 @@ # this is to be included into a Kgio::Socket-derived class # this requires Ruby 2.1 and later for "exception: false" module Yahns::OpenSSLClient # :nodoc: - include Yahns::SendfileCompat - def self.included(cls) # Forward these methods to OpenSSL::SSL::SSLSocket so hijackers # can rely on stdlib methods instead of ugly kgio stuff that @@ -37,15 +36,25 @@ def sync def yahns_init_ssl(ssl_ctx) @need_accept = true @ssl = OpenSSL::SSL::SSLSocket.new(self, ssl_ctx) + @ssl_blocked = nil end def kgio_trywrite(buf) - rv = @ssl.write_nonblock(buf, exception: false) - Integer === rv and - rv = buf.bytesize == rv ? nil : buf.byteslice(rv, buf.bytesize) + buf = @ssl_blocked = buf.dup + case rv = @ssl.write_nonblock(buf, exception: false) + when :wait_readable, :wait_writable + return rv # do not clear ssl_blocked + when Integer + rv = buf.bytesize == rv ? nil : buf.byteslice(rv, buf.bytesize - rv) + end + @ssl_blocked = nil rv end + def kgio_trywritev(buf) + kgio_trywrite(buf.join) + end + def kgio_syssend(buf, flags) kgio_trywrite(buf) end @@ -63,6 +72,27 @@ def kgio_tryread(len, buf) @ssl.read_nonblock(len, buf, exception: false) end + def trysendfile(io, offset, count) + return 0 if count == 0 + + unless buf = @ssl_blocked + count = 0x4000 if count > 0x4000 + buf = Thread.current[:yahns_sfbuf] ||= ''.dup + io.pos = offset + buf = io.read(count, buf) or return # nil for EOF + buf = @ssl_blocked = buf.dup + end + + # call write_nonblock directly since kgio_trywrite allocates + # an unnecessary string + case rv = @ssl.write_nonblock(buf, exception: false) + when :wait_readable, :wait_writable + return rv # do not clear ssl_blocked + end + @ssl_blocked = nil + rv + end + def close @ssl.close # flushes SSLSocket super # IO#close -- EW