[WIP OPTION #1] fix output buffering with SSL_write

[WIP OPTION #1] fix output buffering with SSL_write

[Eric Wong]

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.

FIXME: this probably breaks badly for embedded strings
(but it's tricky to test...)
---
 lib/yahns/openssl_client.rb      |  5 +++++
 lib/yahns/proxy_http_response.rb |  1 +
 lib/yahns/sendfile_compat.rb     | 23 +++++++++++++++++------
 lib/yahns/wbuf.rb                | 14 +++++++++++++-
 4 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/lib/yahns/openssl_client.rb b/lib/yahns/openssl_client.rb
index bf64255..c5b5c8a 100644
--- a/lib/yahns/openssl_client.rb
+++ b/lib/yahns/openssl_client.rb
@@ -37,6 +37,7 @@ def sync
   def yahns_init_ssl(ssl_ctx)
     @need_accept = true
     @ssl = OpenSSL::SSL::SSLSocket.new(self, ssl_ctx)
+    @ssl_blocked = false
   end
 
   def kgio_trywrite(buf)
@@ -46,6 +47,10 @@ def kgio_trywrite(buf)
     rv
   end
 
+  def kgio_trywritev(buf)
+    abort 'BUG: we cannot use writev with OpenSSL'
+  end
+
   def kgio_syssend(buf, flags)
     kgio_trywrite(buf)
   end
diff --git a/lib/yahns/proxy_http_response.rb b/lib/yahns/proxy_http_response.rb
index 0a7e722..287d039 100644
--- a/lib/yahns/proxy_http_response.rb
+++ b/lib/yahns/proxy_http_response.rb
@@ -11,6 +11,7 @@ module Yahns::HttpResponse # :nodoc:
   # it may return a newly-created wbuf or nil
   def proxy_write(wbuf, buf, alive)
     unless wbuf
+      buf = buf.join if Array === buf && respond_to?(:yahns_init_ssl)
       # no write buffer, try to write directly to the client socket
       case rv = String === buf ? kgio_trywrite(buf) : kgio_trywritev(buf)
       when nil then return # done writing buf, likely
diff --git a/lib/yahns/sendfile_compat.rb b/lib/yahns/sendfile_compat.rb
index 8bd4622..4842cda 100644
--- a/lib/yahns/sendfile_compat.rb
+++ b/lib/yahns/sendfile_compat.rb
@@ -4,21 +4,32 @@
 # frozen_string_literal: true
 
 module Yahns::SendfileCompat # :nodoc:
+  attr_accessor :ssl_blocked
+
   def trysendfile(io, offset, count)
     return 0 if count == 0
-    count = 0x4000 if count > 0x4000
-    buf = Thread.current[:yahns_sfbuf] ||= ''.dup
-    io.pos = offset
-    str = io.read(count, buf) or return # nil for EOF
+
+    if respond_to?(:yahns_init_ssl) && @ssl_blocked
+      str = @ssl_blocked
+      @ssl_blocked = false
+    else
+      count = 0x4000 if count > 0x4000
+      buf = Thread.current[:yahns_sfbuf] ||= ''.dup
+      io.pos = offset
+      str = io.read(count, buf) or return # nil for EOF
+    end
+
     n = 0
     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
+      @ssl_blocked = str
+      Thread.current[:yahns_sfbuf] = ''.dup
       return n > 0 ? n : rv
     when nil
-      return n + str.size # yay!
+      return n + str.bytesize # yay!
     end while true
   end
 end
diff --git a/lib/yahns/wbuf.rb b/lib/yahns/wbuf.rb
index 1b4ce6e..3dddb95 100644
--- a/lib/yahns/wbuf.rb
+++ b/lib/yahns/wbuf.rb
@@ -48,6 +48,9 @@ def wbuf_writev(buf)
   end
 
   def wbuf_write(c, buf)
+    ssl = c.respond_to?(:yahns_init_ssl)
+    buf = buf.join if ssl && Array === buf
+
     # try to bypass the VFS layer and write directly to the socket
     # if we're all caught up
     case rv = String === buf ? c.kgio_trywrite(buf) : c.kgio_trywritev(buf)
@@ -60,7 +63,16 @@ def wbuf_write(c, buf)
     end until @busy
 
     @tmpio ||= Yahns::TmpIO.new(@tmpdir)
-    @sf_count += String === buf ? @tmpio.write(buf) : wbuf_writev(buf)
+    if @sf_count == 0 && c.respond_to?(:yahns_init_ssl)
+      # We need to maintain a consistent RSTRING_PTR and MRI strings are CoW
+      # XXX: this breaks if we have embedded strings!
+      c.ssl_blocked = buf.dup
+      # we need to maintain placeholder in tmpio so
+      # sf_offset remains consistent for wbuf_flush
+      @sf_count = @tmpio.write(buf)
+    else
+      @sf_count += String === buf ? @tmpio.write(buf) : wbuf_writev(buf)
+    end
 
     # we spent some time copying to the FS, try to write to
     # the socket again in case some space opened up...
-- 
EW

[WIP OPTION #2] fix output buffering with SSL_write

[Eric Wong]

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.

Theoretically this works better than option #1 w.r.t
embedded strings.  But it falls down badly when I run:

  git clone --mirror https://yhbt.net/yahns-public

To clone the mailing list archives.  Plain HTTP works fine.
So I'm running Option #1 on https://yhbt.net/, for now...

Sleepy and tired now :<
---
 lib/yahns/openssl_client.rb      | 21 ++++++++++++++++++++-
 lib/yahns/proxy_http_response.rb |  1 +
 lib/yahns/sendfile_compat.rb     | 22 ++++++++++++++++------
 lib/yahns/wbuf.rb                |  3 +++
 4 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/lib/yahns/openssl_client.rb b/lib/yahns/openssl_client.rb
index bf64255..4587483 100644
--- a/lib/yahns/openssl_client.rb
+++ b/lib/yahns/openssl_client.rb
@@ -37,15 +37,34 @@ def sync
   def yahns_init_ssl(ssl_ctx)
     @need_accept = true
     @ssl = OpenSSL::SSL::SSLSocket.new(self, ssl_ctx)
+    @ssl_blocked = ''.dup
   end
 
   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
     rv
   end
 
+  def kgio_trywritev(buf)
+    abort 'BUG: we cannot use writev with OpenSSL'
+  end
+
   def kgio_syssend(buf, flags)
     kgio_trywrite(buf)
   end
diff --git a/lib/yahns/proxy_http_response.rb b/lib/yahns/proxy_http_response.rb
index 0a7e722..287d039 100644
--- a/lib/yahns/proxy_http_response.rb
+++ b/lib/yahns/proxy_http_response.rb
@@ -11,6 +11,7 @@ module Yahns::HttpResponse # :nodoc:
   # it may return a newly-created wbuf or nil
   def proxy_write(wbuf, buf, alive)
     unless wbuf
+      buf = buf.join if Array === buf && respond_to?(:yahns_init_ssl)
       # no write buffer, try to write directly to the client socket
       case rv = String === buf ? kgio_trywrite(buf) : kgio_trywritev(buf)
       when nil then return # done writing buf, likely
diff --git a/lib/yahns/sendfile_compat.rb b/lib/yahns/sendfile_compat.rb
index 8bd4622..e382f18 100644
--- 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)
     return 0 if count == 0
-    count = 0x4000 if count > 0x4000
-    buf = Thread.current[:yahns_sfbuf] ||= ''.dup
-    io.pos = offset
-    str = io.read(count, buf) or return # nil for EOF
+
+    if respond_to?(:yahns_init_ssl) && !@ssl_blocked.empty?
+      str = @ssl_blocked
+      #warn "#{fileno} unblocking #{str.bytesize} #{str.object_id}"
+    else
+      count = 0x4000 if count > 0x4000
+      buf = Thread.current[:yahns_sfbuf] ||= ''.dup
+      io.pos = offset
+      str = io.read(count, buf) or return # nil for EOF
+    end
+
     n = 0
     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
diff --git a/lib/yahns/wbuf.rb b/lib/yahns/wbuf.rb
index 1b4ce6e..ff67e0c 100644
--- a/lib/yahns/wbuf.rb
+++ b/lib/yahns/wbuf.rb
@@ -48,6 +48,9 @@ def wbuf_writev(buf)
   end
 
   def wbuf_write(c, buf)
+    ssl = c.respond_to?(:yahns_init_ssl)
+    buf = buf.join if ssl && Array === buf
+
     # try to bypass the VFS layer and write directly to the socket
     # if we're all caught up
     case rv = String === buf ? c.kgio_trywrite(buf) : c.kgio_trywritev(buf)
-- 
EW

[PATCH] fix output buffering with SSL_write

[Eric Wong]

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 <e@80x24.org> wrote:
  > --- a/lib/yahns/openssl_client.rb
  > +++ b/lib/yahns/openssl_client.rb

  <snip>

  >    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)

  <snip>

  >      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 <yahns-public@yhbt.net>
 # 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

Re: [PATCH] fix output buffering with SSL_write

[Eric Wong]

Eric Wong <e@80x24.org> wrote:
>  lib/yahns/openssl_client.rb | 40 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)

Btw, reproducing this as a standalone test case over loopback
seems difficult, even with small socket buffers.
It could also be cipher/algorithm-dependent...

I never hit problems with v1.12.0 when git-cloning
https://yhbt.net/yahns-public locally on the server, only from a
remote connection.  Same thing with largish static files.

So I'm tempted to release it as-is without an additional test case.