* [WIP OPTION #1] fix output buffering with SSL_write
@ 2016-02-16 11:11 Eric Wong
2016-02-16 11:14 ` [WIP OPTION #2] " Eric Wong
0 siblings, 1 reply; 4+ messages in thread
From: Eric Wong @ 2016-02-16 11:11 UTC (permalink / raw)
To: yahns-public
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [WIP OPTION #2] fix output buffering with SSL_write
2016-02-16 11:11 [WIP OPTION #1] fix output buffering with SSL_write Eric Wong
@ 2016-02-16 11:14 ` Eric Wong
2016-02-20 2:54 ` [PATCH] " Eric Wong
0 siblings, 1 reply; 4+ messages in thread
From: Eric Wong @ 2016-02-16 11:14 UTC (permalink / raw)
To: yahns-public
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] fix output buffering with SSL_write
2016-02-16 11:14 ` [WIP OPTION #2] " Eric Wong
@ 2016-02-20 2:54 ` Eric Wong
2016-02-20 21:33 ` Eric Wong
0 siblings, 1 reply; 4+ messages in thread
From: Eric Wong @ 2016-02-20 2:54 UTC (permalink / raw)
To: yahns-public
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] fix output buffering with SSL_write
2016-02-20 2:54 ` [PATCH] " Eric Wong
@ 2016-02-20 21:33 ` Eric Wong
0 siblings, 0 replies; 4+ messages in thread
From: Eric Wong @ 2016-02-20 21:33 UTC (permalink / raw)
To: yahns-public
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-20 21:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-16 11:11 [WIP OPTION #1] fix output buffering with SSL_write Eric Wong
2016-02-16 11:14 ` [WIP OPTION #2] " Eric Wong
2016-02-20 2:54 ` [PATCH] " Eric Wong
2016-02-20 21:33 ` Eric Wong
Code repositories for project(s) associated with this public inbox
https://yhbt.net/yahns.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).