From 0d5a194a9c8c352db7b8391a611e79239182935d Mon Sep 17 00:00:00 2001 From: kares Date: Thu, 26 Mar 2026 08:07:38 +0100 Subject: [PATCH 1/4] [fix] read_nonblock(exception: false) throwing on TLS 1.3 propagate exception flag through read() and readAndUnwrap() --- .../java/org/jruby/ext/openssl/SSLSocket.java | 62 ++- src/test/ruby/ssl/test_read_nonblock_tls13.rb | 505 ++++++++++++++++++ 2 files changed, 552 insertions(+), 15 deletions(-) create mode 100644 src/test/ruby/ssl/test_read_nonblock_tls13.rb diff --git a/src/main/java/org/jruby/ext/openssl/SSLSocket.java b/src/main/java/org/jruby/ext/openssl/SSLSocket.java index 0a2f801e..b70a0ce7 100644 --- a/src/main/java/org/jruby/ext/openssl/SSLSocket.java +++ b/src/main/java/org/jruby/ext/openssl/SSLSocket.java @@ -539,8 +539,18 @@ public void wakeup() { } } - private static final int READ_WOULD_BLOCK_RESULT = Integer.MIN_VALUE + 1; - private static final int WRITE_WOULD_BLOCK_RESULT = Integer.MIN_VALUE + 2; + // Legitimate return values are -1 (EOF) and >= 0 (byte counts), so any value < -1 is safely in sentinel territory. + private static final int READ_WOULD_BLOCK_RESULT = -2; + private static final int WRITE_WOULD_BLOCK_RESULT = -3; + + private static boolean isWouldBlockResult(final int result) { + return result < -1; + } + + private RubySymbol wouldBlockSymbol(final int result) { + assert isWouldBlockResult(result) : "unexpected result: " + result; + return getRuntime().newSymbol(result == READ_WOULD_BLOCK_RESULT ? "wait_readable" : "wait_writable"); + } private static void readWouldBlock(final Ruby runtime, final boolean exception, final int[] result) { if ( exception ) throw newSSLErrorWaitReadable(runtime, "read would block"); @@ -552,10 +562,6 @@ private static void writeWouldBlock(final Ruby runtime, final boolean exception, result[0] = WRITE_WOULD_BLOCK_RESULT; } - private void doHandshake(final boolean blocking) throws IOException { - doHandshake(blocking, true); - } - // might return :wait_readable | :wait_writable in case (true, false) private IRubyObject doHandshake(final boolean blocking, final boolean exception) throws IOException { while (true) { @@ -577,7 +583,11 @@ private IRubyObject doHandshake(final boolean blocking, final boolean exception) doTasks(); break; case NEED_UNWRAP: - if (readAndUnwrap(blocking) == -1 && handshakeStatus != SSLEngineResult.HandshakeStatus.FINISHED) { + int unwrapResult = readAndUnwrap(blocking, exception); + if (isWouldBlockResult(unwrapResult)) { + return wouldBlockSymbol(unwrapResult); + } + if (unwrapResult == -1 && handshakeStatus != SSLEngineResult.HandshakeStatus.FINISHED) { throw new SSLHandshakeException("Socket closed"); } // during initialHandshake, calling readAndUnwrap that results UNDERFLOW does not mean writable. @@ -703,12 +713,16 @@ public int write(ByteBuffer src, boolean blocking) throws SSLException, IOExcept } public int read(final ByteBuffer dst, final boolean blocking) throws IOException { + return read(dst, blocking, true); + } + + private int read(final ByteBuffer dst, final boolean blocking, final boolean exception) throws IOException { if ( initialHandshake ) return 0; if ( engine.isInboundDone() ) return -1; if ( ! appReadData.hasRemaining() ) { - int appBytesProduced = readAndUnwrap(blocking); - if (appBytesProduced == -1 || appBytesProduced == 0) { + final int appBytesProduced = readAndUnwrap(blocking, exception); + if (appBytesProduced == -1 || appBytesProduced == 0 || isWouldBlockResult(appBytesProduced)) { return appBytesProduced; } } @@ -718,7 +732,15 @@ public int read(final ByteBuffer dst, final boolean blocking) throws IOException return limit; } - private int readAndUnwrap(final boolean blocking) throws IOException { + /** + * @param blocking whether to block on I/O + * @param exception when false, returns {@link #READ_WOULD_BLOCK_RESULT} or + * {@link #WRITE_WOULD_BLOCK_RESULT} instead of throwing if the + * post-handshake processing would block + * @return application bytes available, -1 on EOF/close, 0 when no app data + * produced, or a WOULD_BLOCK sentinel when would-block with exception=false + */ + private int readAndUnwrap(final boolean blocking, final boolean exception) throws IOException { final int bytesRead = socketChannelImpl().read(netReadData); if ( bytesRead == -1 ) { if ( ! netReadData.hasRemaining() || @@ -767,7 +789,11 @@ private int readAndUnwrap(final boolean blocking) throws IOException { handshakeStatus == SSLEngineResult.HandshakeStatus.NEED_TASK || handshakeStatus == SSLEngineResult.HandshakeStatus.NEED_WRAP || handshakeStatus == SSLEngineResult.HandshakeStatus.FINISHED ) ) { - doHandshake(blocking); + IRubyObject ex = doHandshake(blocking, exception); + if ( ex != null ) { // :wait_readable | :wait_writable + // TODO needs refactoring to avoid Symbol -> int -> Symbol + return "wait_writable".equals(ex.asJavaString()) ? WRITE_WOULD_BLOCK_RESULT : READ_WOULD_BLOCK_RESULT; + } } return appReadData.remaining(); } @@ -843,12 +869,18 @@ private IRubyObject sysreadImpl(final ThreadContext context, final IRubyObject l if ( engine == null ) { read = socketChannelImpl().read(dst); } else { - read = read(dst, blocking); + read = read(dst, blocking, exception); } - if ( read == -1 ) { - if ( exception ) throw runtime.newEOFError(); - return context.nil; + switch ( read ) { + case -1 : + if ( exception ) throw runtime.newEOFError(); + return context.nil; + // Post-handshake processing (e.g. TLS 1.3 NewSessionTicket) signaled would-block + case READ_WOULD_BLOCK_RESULT : + return runtime.newSymbol("wait_readable"); + case WRITE_WOULD_BLOCK_RESULT : + return runtime.newSymbol("wait_writable"); } if ( read == 0 && status == SSLEngineResult.Status.BUFFER_UNDERFLOW ) { diff --git a/src/test/ruby/ssl/test_read_nonblock_tls13.rb b/src/test/ruby/ssl/test_read_nonblock_tls13.rb new file mode 100644 index 00000000..bd39b1a5 --- /dev/null +++ b/src/test/ruby/ssl/test_read_nonblock_tls13.rb @@ -0,0 +1,505 @@ +# frozen_string_literal: false +# Regression tests for: +# https://github.com/jruby/jruby-openssl/issues/271 +# https://github.com/jruby/jruby-openssl/issues/305 +# https://github.com/jruby/jruby-openssl/issues/317 +# +# Root cause: readAndUnwrap() in SSLSocket.java called doHandshake(blocking) +# (the 1-arg overload that hardcodes exception=true) when processing TLS 1.3 +# post-handshake records (NewSessionTicket). When selectNow()==0 inside that +# doHandshake, it threw SSLErrorWaitReadable even when the caller passed +# exception:false, violating the non-blocking contract. +# +# Fix: readAndUnwrap now accepts and forwards the exception flag to +# doHandshake(blocking, exception), and returns a WOULD_BLOCK sentinel +# that propagates back through read() and sysreadImpl() as :wait_readable. +# +require File.expand_path('test_helper', File.dirname(__FILE__)) + +class TestReadNonblockTLS13 < TestCase + + include SSLTestHelper + + # ── helpers ────────────────────────────────────────────────────────── + + # Set up a TLS 1.3 server where the server does NOT read, so the + # client's send buffer saturates. Yields |ssl, port| to the block. + # This forces selectNow()==0 inside doHandshake when processing + # TLS 1.3 post-handshake records. + def with_saturated_tls13_client + require 'socket' + + tcp_server = TCPServer.new("127.0.0.1", 0) + port = tcp_server.local_address.ip_port + + server_ctx = OpenSSL::SSL::SSLContext.new + server_ctx.cert = @svr_cert + server_ctx.key = @svr_key + server_ctx.ssl_version = "TLSv1_3" + + ssl_server = OpenSSL::SSL::SSLServer.new(tcp_server, server_ctx) + ssl_server.start_immediately = true + + server_ready = Queue.new + server_thread = Thread.new do + Thread.current.report_on_exception = false + begin + ssl_conn = ssl_server.accept + server_ready << :ready + # Do NOT read — the client's send buffer will fill up + sleep 5 + ssl_conn.close rescue nil + rescue + server_ready << :error + end + end + + begin + sock = TCPSocket.new("127.0.0.1", port) + sock.setsockopt(Socket::SOL_SOCKET, Socket::SO_SNDBUF, 4096) + sock.setsockopt(Socket::SOL_SOCKET, Socket::SO_RCVBUF, 4096) + + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + + server_ready.pop # wait for server accept + + # Saturate send buffer + chunk = "X" * 16384 + 100.times do + begin + ssl.write_nonblock(chunk) + rescue IO::WaitWritable, OpenSSL::SSL::SSLErrorWaitWritable + break + rescue + break + end + end + + yield ssl + ensure + ssl.close rescue nil + sock.close rescue nil + tcp_server.close rescue nil + server_thread.kill rescue nil + server_thread.join(2) rescue nil + end + end + + # Same as above but for TLS 1.2 (control) + def with_saturated_tls12_client + require 'socket' + + tcp_server = TCPServer.new("127.0.0.1", 0) + port = tcp_server.local_address.ip_port + + server_ctx = OpenSSL::SSL::SSLContext.new + server_ctx.cert = @svr_cert + server_ctx.key = @svr_key + server_ctx.max_version = OpenSSL::SSL::TLS1_2_VERSION + + ssl_server = OpenSSL::SSL::SSLServer.new(tcp_server, server_ctx) + ssl_server.start_immediately = true + + server_ready = Queue.new + server_thread = Thread.new do + Thread.current.report_on_exception = false + begin + ssl_conn = ssl_server.accept + server_ready << :ready + sleep 5 + ssl_conn.close rescue nil + rescue + server_ready << :error + end + end + + begin + sock = TCPSocket.new("127.0.0.1", port) + sock.setsockopt(Socket::SOL_SOCKET, Socket::SO_SNDBUF, 4096) + sock.setsockopt(Socket::SOL_SOCKET, Socket::SO_RCVBUF, 4096) + + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + + server_ready.pop + + chunk = "X" * 16384 + 100.times do + begin + ssl.write_nonblock(chunk) + rescue IO::WaitWritable, OpenSSL::SSL::SSLErrorWaitWritable + break + rescue + break + end + end + + yield ssl + ensure + ssl.close rescue nil + sock.close rescue nil + tcp_server.close rescue nil + server_thread.kill rescue nil + server_thread.join(2) rescue nil + end + end + + # ── TLS 1.3 + saturated buffer (the exact production bug scenario) ── + + # Core reproducer: exception:false must return :wait_readable, not throw. + def test_read_nonblock_exception_false_saturated_tls13 + with_saturated_tls13_client do |ssl| + assert_equal "TLSv1.3", ssl.ssl_version + + result = ssl.read_nonblock(1024, exception: false) + assert_equal :wait_readable, result + end + end + + # exception:true must raise SSLErrorWaitReadable (not EAGAIN or IOError). + def test_read_nonblock_exception_true_saturated_tls13 + with_saturated_tls13_client do |ssl| + assert_equal "TLSv1.3", ssl.ssl_version + + raised = assert_raise(OpenSSL::SSL::SSLErrorWaitReadable) do + ssl.read_nonblock(1024) + end + assert_equal "read would block", raised.message + end + end + + # httprb code path: read_nonblock with buffer + exception:false + def test_read_nonblock_with_buffer_exception_false_saturated_tls13 + with_saturated_tls13_client do |ssl| + buf = '' + result = ssl.read_nonblock(1024, buf, exception: false) + assert_equal :wait_readable, result + end + end + + # Calling through sysread_nonblock directly (as some gems do) + def test_sysread_nonblock_exception_false_saturated_tls13 + with_saturated_tls13_client do |ssl| + result = ssl.send(:sysread_nonblock, 1024, exception: false) + assert_equal :wait_readable, result + end + end + + # Multiple consecutive read_nonblock calls must all return :wait_readable + def test_read_nonblock_repeated_calls_saturated_tls13 + with_saturated_tls13_client do |ssl| + 5.times do |i| + result = ssl.read_nonblock(1024, exception: false) + assert_equal :wait_readable, result, "iteration #{i}" + end + end + end + + # ── TLS 1.2 + saturated buffer (control — no post-handshake messages) ─ + + # TLS 1.2 has no post-handshake messages, so the bug path is never hit. + def test_read_nonblock_exception_false_saturated_tls12 + with_saturated_tls12_client do |ssl| + assert_equal "TLSv1.2", ssl.ssl_version + + result = ssl.read_nonblock(1024, exception: false) + assert_equal :wait_readable, result + end + end + + def test_read_nonblock_exception_true_saturated_tls12 + with_saturated_tls12_client do |ssl| + assert_equal "TLSv1.2", ssl.ssl_version + + assert_raise(OpenSSL::SSL::SSLErrorWaitReadable) do + ssl.read_nonblock(1024) + end + end + end + + # ── TLS 1.3 normal (unsaturated) tests ───────────────────────────── + + def test_read_nonblock_exception_false_tls13 + ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + assert_equal "TLSv1.3", ssl.ssl_version + + 10.times do + result = ssl.read_nonblock(1024, exception: false) + assert [:wait_readable, String].any? { |t| t === result }, + "Expected :wait_readable or String, got #{result.inspect}" + break if result == :wait_readable + end + ssl.close + end + end + + def test_read_nonblock_exception_true_tls13 + ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + assert_equal "TLSv1.3", ssl.ssl_version + + assert_raise(OpenSSL::SSL::SSLErrorWaitReadable) do + 10.times { ssl.read_nonblock(1024) } + end + ssl.close + end + end + + # ── TLS 1.2 normal (unsaturated) tests (control) ─────────────────── + + def test_read_nonblock_exception_false_tls12 + ctx_proc = Proc.new { |ctx| ctx.max_version = OpenSSL::SSL::TLS1_2_VERSION } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + assert_equal "TLSv1.2", ssl.ssl_version + + result = ssl.read_nonblock(1024, exception: false) + assert_equal :wait_readable, result + ssl.close + end + end + + # ── Data round-trip: TLS 1.3 read/write still works ──────────────── + + def test_write_read_roundtrip_tls13 + ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + assert_equal "TLSv1.3", ssl.ssl_version + + ssl.write("hello\n") + # Wait for echo data to arrive with a generous timeout + IO.select([ssl], nil, nil, 5) + # The first read_nonblock may consume a post-handshake message; + # retry until we get the application data. + data = nil + 10.times do + begin + data = ssl.read_nonblock(1024) + break + rescue OpenSSL::SSL::SSLErrorWaitReadable + IO.select([ssl], nil, nil, 2) + end + end + assert_equal "hello\n", data + + ssl.close + end + end + + def test_write_read_roundtrip_nonblock_exception_false_tls13 + ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + + ssl.write("world\n") + # Wait for echo data to arrive + IO.select([ssl], nil, nil, 5) + + # Read with exception:false — might get :wait_readable first if + # the engine is processing a post-handshake record. + result = nil + 10.times do + result = ssl.read_nonblock(1024, exception: false) + if result == :wait_readable + IO.select([ssl], nil, nil, 2) + next + end + break + end + assert_kind_of String, result + assert_equal "world\n", result + + # No more data — should return :wait_readable + result = ssl.read_nonblock(1024, exception: false) + assert_equal :wait_readable, result + + ssl.close + end + end + + # ── Post-write read with saturated buffer ─────────────────────────── + + # After a write+read cycle the post-handshake messages are consumed; + # a subsequent read_nonblock should simply return :wait_readable. + def test_read_nonblock_after_write_tls13 + ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + + ssl.write("test\n") + sleep 0.1 + begin; ssl.read_nonblock(1024); rescue OpenSSL::SSL::SSLErrorWaitReadable; end + + result = ssl.read_nonblock(1024, exception: false) + assert_equal :wait_readable, result + ssl.close + end + end + + # ── connect_nonblock + read_nonblock ──────────────────────────────── + + def test_read_nonblock_with_connect_nonblock_tls13 + ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + + begin + ssl.connect_nonblock + rescue IO::WaitReadable + IO.select([ssl]); retry + rescue IO::WaitWritable + IO.select(nil, [ssl]); retry + end + + assert_equal "TLSv1.3", ssl.ssl_version + sleep 0.05 + + 10.times do + result = ssl.read_nonblock(1024, exception: false) + assert [:wait_readable, String].any? { |t| t === result }, + "Expected :wait_readable or String, got #{result.inspect}" + break if result == :wait_readable + end + ssl.close + end + end + + # connect_nonblock + saturated buffer + read_nonblock + def test_read_nonblock_connect_nonblock_saturated_tls13 + require 'socket' + + tcp_server = TCPServer.new("127.0.0.1", 0) + port = tcp_server.local_address.ip_port + + server_ctx = OpenSSL::SSL::SSLContext.new + server_ctx.cert = @svr_cert + server_ctx.key = @svr_key + server_ctx.ssl_version = "TLSv1_3" + + ssl_server = OpenSSL::SSL::SSLServer.new(tcp_server, server_ctx) + ssl_server.start_immediately = true + + server_ready = Queue.new + server_thread = Thread.new do + Thread.current.report_on_exception = false + begin + ssl_conn = ssl_server.accept + server_ready << :ready + sleep 5 + ssl_conn.close rescue nil + rescue + server_ready << :error + end + end + + begin + sock = TCPSocket.new("127.0.0.1", port) + sock.setsockopt(Socket::SOL_SOCKET, Socket::SO_SNDBUF, 4096) + sock.setsockopt(Socket::SOL_SOCKET, Socket::SO_RCVBUF, 4096) + + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + + begin + ssl.connect_nonblock + rescue IO::WaitReadable + IO.select([ssl]); retry + rescue IO::WaitWritable + IO.select(nil, [ssl]); retry + end + + assert_equal "TLSv1.3", ssl.ssl_version + server_ready.pop + + chunk = "X" * 16384 + 100.times do + begin + ssl.write_nonblock(chunk) + rescue IO::WaitWritable, OpenSSL::SSL::SSLErrorWaitWritable + break + rescue + break + end + end + + result = ssl.read_nonblock(1024, exception: false) + assert_equal :wait_readable, result + ensure + ssl.close rescue nil + sock.close rescue nil + tcp_server.close rescue nil + server_thread.kill rescue nil + server_thread.join(2) rescue nil + end + end + + # ── Concurrent stress ────────────────────────────────────────────── + + def test_read_nonblock_tls13_concurrent_stress + ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + errors = Queue.new + + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + threads = 5.times.map do |t| + Thread.new do + 20.times do |i| + begin + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + + 5.times do + result = ssl.read_nonblock(1024, exception: false) + break if result == :wait_readable + end + rescue OpenSSL::SSL::SSLErrorWaitReadable + errors << "Thread #{t} iter #{i}: SSLErrorWaitReadable thrown with exception:false" + rescue Errno::EAGAIN + errors << "Thread #{t} iter #{i}: EAGAIN thrown with exception:false" + rescue + # Other errors (connection reset, etc.) are acceptable + ensure + ssl.close rescue nil + sock.close rescue nil + end + end + end + end + + threads.each { |t| t.join(10) } + end + + collected = [] + collected << errors.pop until errors.empty? + assert collected.empty?, "Got #{collected.size} exception leaks:\n#{collected.first(5).join("\n")}" + end + +end From 08c2017ce1c79cc9980b90a45c47f5fb0bd339d0 Mon Sep 17 00:00:00 2001 From: kares Date: Thu, 26 Mar 2026 08:51:16 +0100 Subject: [PATCH 2/4] [refactor] avoid wasted iteration on TLS 1.3 post-handshake `read==0` guard only called waitSelect when `status==BUFFER_UNDERFLOW` after `readAndUnwrap` processes a TLS 1.3 NewSessionTicket (status=OK, zero app bytes, no buffered network data), the guard was skipped causing an unnecessary extra loop through `readAndUnwrap` before `BUFFER_UNDERFLOW` was reached on the second pass --- .../java/org/jruby/ext/openssl/SSLSocket.java | 13 ++- src/test/ruby/ssl/test_read_nonblock_tls13.rb | 91 ++++++++++++++----- 2 files changed, 78 insertions(+), 26 deletions(-) diff --git a/src/main/java/org/jruby/ext/openssl/SSLSocket.java b/src/main/java/org/jruby/ext/openssl/SSLSocket.java index b70a0ce7..0c87ebe9 100644 --- a/src/main/java/org/jruby/ext/openssl/SSLSocket.java +++ b/src/main/java/org/jruby/ext/openssl/SSLSocket.java @@ -883,11 +883,14 @@ private IRubyObject sysreadImpl(final ThreadContext context, final IRubyObject l return runtime.newSymbol("wait_writable"); } - if ( read == 0 && status == SSLEngineResult.Status.BUFFER_UNDERFLOW ) { - // If we didn't get any data back because we only read in a partial TLS record, - // instead of spinning until the rest comes in, call waitSelect to either block - // until the rest is available, or throw a "read would block" error if we are in - // non-blocking mode. + if ( read == 0 && netReadData.position() == 0 ) { + // If we didn't get any data back and there is no buffered network data left to process, + // wait for more data from the network instead of spinning until it arrives. + // In blocking mode this blocks; in non-blocking mode it raises/returns "read would block". + // + // We check netReadData.position() rather than status == BUFFER_UNDERFLOW because readAndUnwrap + // may have successfully consumed a non-application record (e.g. a TLS 1.3 NewSessionTicket) + // leaving status == OK with zero app bytes produced and nothing left in the network buffer. final Object ex = waitSelect(SelectionKey.OP_READ, blocking, exception); if ( ex instanceof IRubyObject ) return (IRubyObject) ex; // :wait_readable } diff --git a/src/test/ruby/ssl/test_read_nonblock_tls13.rb b/src/test/ruby/ssl/test_read_nonblock_tls13.rb index bd39b1a5..f854c470 100644 --- a/src/test/ruby/ssl/test_read_nonblock_tls13.rb +++ b/src/test/ruby/ssl/test_read_nonblock_tls13.rb @@ -1,19 +1,5 @@ # frozen_string_literal: false -# Regression tests for: -# https://github.com/jruby/jruby-openssl/issues/271 -# https://github.com/jruby/jruby-openssl/issues/305 -# https://github.com/jruby/jruby-openssl/issues/317 -# -# Root cause: readAndUnwrap() in SSLSocket.java called doHandshake(blocking) -# (the 1-arg overload that hardcodes exception=true) when processing TLS 1.3 -# post-handshake records (NewSessionTicket). When selectNow()==0 inside that -# doHandshake, it threw SSLErrorWaitReadable even when the caller passed -# exception:false, violating the non-blocking contract. -# -# Fix: readAndUnwrap now accepts and forwards the exception flag to -# doHandshake(blocking, exception), and returns a WOULD_BLOCK sentinel -# that propagates back through read() and sysreadImpl() as :wait_readable. -# + require File.expand_path('test_helper', File.dirname(__FILE__)) class TestReadNonblockTLS13 < TestCase @@ -26,8 +12,7 @@ class TestReadNonblockTLS13 < TestCase # client's send buffer saturates. Yields |ssl, port| to the block. # This forces selectNow()==0 inside doHandshake when processing # TLS 1.3 post-handshake records. - def with_saturated_tls13_client - require 'socket' + def with_saturated_tls13_client; require 'socket' tcp_server = TCPServer.new("127.0.0.1", 0) port = tcp_server.local_address.ip_port @@ -88,8 +73,7 @@ def with_saturated_tls13_client end # Same as above but for TLS 1.2 (control) - def with_saturated_tls12_client - require 'socket' + def with_saturated_tls12_client; require 'socket' tcp_server = TCPServer.new("127.0.0.1", 0) port = tcp_server.local_address.ip_port @@ -392,8 +376,7 @@ def test_read_nonblock_with_connect_nonblock_tls13 end # connect_nonblock + saturated buffer + read_nonblock - def test_read_nonblock_connect_nonblock_saturated_tls13 - require 'socket' + def test_read_nonblock_connect_nonblock_saturated_tls13; require 'socket' tcp_server = TCPServer.new("127.0.0.1", 0) port = tcp_server.local_address.ip_port @@ -502,4 +485,70 @@ def test_read_nonblock_tls13_concurrent_stress assert collected.empty?, "Got #{collected.size} exception leaks:\n#{collected.first(5).join("\n")}" end + # ── Wasted iteration detection ───────────────────────────────────── + # + # TLS 1.3 post-handshake record (NewSessionTicket) that produces 0 app bytes, status is OK + # + # We detect this by inspecting the internal `status` field after read_nonblock returns :wait_readable. + # If status is BUFFER_UNDERFLOW, the extra iteration occurred. If status is OK, sysreadImpl handled + # the read==0/status==OK case directly. + def test_internal_no_wasted_readAndUnwrap_iteration_tls13; require 'socket' + + tcp_server = TCPServer.new("127.0.0.1", 0) + port = tcp_server.local_address.ip_port + + server_ctx = OpenSSL::SSL::SSLContext.new + server_ctx.cert = @svr_cert + server_ctx.key = @svr_key + server_ctx.ssl_version = "TLSv1_3" + + ssl_server = OpenSSL::SSL::SSLServer.new(tcp_server, server_ctx) + ssl_server.start_immediately = true + + server_thread = Thread.new do + Thread.current.report_on_exception = false + begin + conn = ssl_server.accept + sleep 5 + conn.close rescue nil + rescue + end + end + + begin + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + assert_equal "TLSv1.3", ssl.ssl_version + + # Wait for the server's NewSessionTicket to arrive on the wire + # after the blocking connect has finished. + sleep 0.1 + + # Access the private `status` field via Java reflection + java_cls = Java::OrgJrubyExtOpenssl::SSLSocket.java_class + status_field = java_cls.declared_field("status") + status_field.accessible = true + java_ssl = ssl.to_java(Java::OrgJrubyExtOpenssl::SSLSocket) + + result = ssl.read_nonblock(1024, exception: false) + assert_equal :wait_readable, result + + status_after = status_field.value(java_ssl).to_s + # If sysreadImpl properly handles read==0 with any status (not just BUFFER_UNDERFLOW), + # only one readAndUnwrap call is made and status stays OK. + assert_equal "OK", status_after, + "Expected status OK (single readAndUnwrap call) but got #{status_after} " \ + "(extra wasted iteration through readAndUnwrap occurred)" + + ssl.close + ensure + ssl.close rescue nil + sock.close rescue nil + tcp_server.close rescue nil + server_thread.kill rescue nil + server_thread.join(2) rescue nil + end + end if defined?(JRUBY_VERSION) end From 02ce78d01539a0ffadb71d5140647e8d45f012b2 Mon Sep 17 00:00:00 2001 From: kares Date: Thu, 26 Mar 2026 12:16:46 +0100 Subject: [PATCH 3/4] [test] buffered I/O tests for TLS 1.3 and TLS 1.2 SSL read paths Cover normal (non-error) data flow through the code paths changed by the read_nonblock exception fix and the netReadData.position() guard: - Multi-chunk read_nonblock: 30KB echo in 1KB chunks (TLS 1.3 + 1.2) - Multi-chunk with exception:false (TLS 1.3) - Partial read_nonblock: read 5 bytes then remainder from appReadData - Multiple puts/gets cycles: 10 rounds on a single connection - sysread/syswrite round-trip: 3 blocking cycles with exact byte counts - Large server write: server sends 48KB, client reads in 4KB chunks; regression test for netReadData.position()==0 guard (TLS 1.3 + 1.2) --- src/test/ruby/ssl/test_read_nonblock_tls13.rb | 365 +++++++++++++++++- 1 file changed, 356 insertions(+), 9 deletions(-) diff --git a/src/test/ruby/ssl/test_read_nonblock_tls13.rb b/src/test/ruby/ssl/test_read_nonblock_tls13.rb index f854c470..084d2761 100644 --- a/src/test/ruby/ssl/test_read_nonblock_tls13.rb +++ b/src/test/ruby/ssl/test_read_nonblock_tls13.rb @@ -20,7 +20,7 @@ def with_saturated_tls13_client; require 'socket' server_ctx = OpenSSL::SSL::SSLContext.new server_ctx.cert = @svr_cert server_ctx.key = @svr_key - server_ctx.ssl_version = "TLSv1_3" + server_ctx.min_version = server_ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION ssl_server = OpenSSL::SSL::SSLServer.new(tcp_server, server_ctx) ssl_server.start_immediately = true @@ -207,7 +207,7 @@ def test_read_nonblock_exception_true_saturated_tls12 # ── TLS 1.3 normal (unsaturated) tests ───────────────────────────── def test_read_nonblock_exception_false_tls13 - ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| sock = TCPSocket.new("127.0.0.1", port) ssl = OpenSSL::SSL::SSLSocket.new(sock) @@ -226,7 +226,7 @@ def test_read_nonblock_exception_false_tls13 end def test_read_nonblock_exception_true_tls13 - ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| sock = TCPSocket.new("127.0.0.1", port) ssl = OpenSSL::SSL::SSLSocket.new(sock) @@ -261,7 +261,7 @@ def test_read_nonblock_exception_false_tls12 # ── Data round-trip: TLS 1.3 read/write still works ──────────────── def test_write_read_roundtrip_tls13 - ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| sock = TCPSocket.new("127.0.0.1", port) ssl = OpenSSL::SSL::SSLSocket.new(sock) @@ -290,7 +290,7 @@ def test_write_read_roundtrip_tls13 end def test_write_read_roundtrip_nonblock_exception_false_tls13 - ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| sock = TCPSocket.new("127.0.0.1", port) ssl = OpenSSL::SSL::SSLSocket.new(sock) @@ -328,7 +328,7 @@ def test_write_read_roundtrip_nonblock_exception_false_tls13 # After a write+read cycle the post-handshake messages are consumed; # a subsequent read_nonblock should simply return :wait_readable. def test_read_nonblock_after_write_tls13 - ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| sock = TCPSocket.new("127.0.0.1", port) ssl = OpenSSL::SSL::SSLSocket.new(sock) @@ -348,7 +348,7 @@ def test_read_nonblock_after_write_tls13 # ── connect_nonblock + read_nonblock ──────────────────────────────── def test_read_nonblock_with_connect_nonblock_tls13 - ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| sock = TCPSocket.new("127.0.0.1", port) ssl = OpenSSL::SSL::SSLSocket.new(sock) @@ -384,7 +384,7 @@ def test_read_nonblock_connect_nonblock_saturated_tls13; require 'socket' server_ctx = OpenSSL::SSL::SSLContext.new server_ctx.cert = @svr_cert server_ctx.key = @svr_key - server_ctx.ssl_version = "TLSv1_3" + server_ctx.min_version = server_ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION ssl_server = OpenSSL::SSL::SSLServer.new(tcp_server, server_ctx) ssl_server.start_immediately = true @@ -446,7 +446,7 @@ def test_read_nonblock_connect_nonblock_saturated_tls13; require 'socket' # ── Concurrent stress ────────────────────────────────────────────── def test_read_nonblock_tls13_concurrent_stress - ctx_proc = Proc.new { |ctx| ctx.ssl_version = "TLSv1_3" } + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } errors = Queue.new start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| @@ -485,6 +485,353 @@ def test_read_nonblock_tls13_concurrent_stress assert collected.empty?, "Got #{collected.size} exception leaks:\n#{collected.first(5).join("\n")}" end + # ── Buffered I/O: multi-chunk read_nonblock ────────────────────── + # + # Write a large payload (bigger than one TLS record ~16KB), read it + # back in small read_nonblock chunks. Exercises: + # - appReadData having leftover bytes across read() calls + # - netReadData having multiple TLS records + # - the netReadData.position()==0 guard NOT firing when there IS data + + def test_multi_chunk_read_nonblock_tls13 + large = "A" * 1024 + "\n" # each line is 1025 bytes + total_lines = 30 # ~30KB total, exceeds one TLS record + payload = large * total_lines + + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + assert_equal "TLSv1.3", ssl.ssl_version + + # Write the payload — the echo server will echo each line back + ssl.write(payload) + + # Read it all back in small non-blocking chunks + received = +"" + deadline = Time.now + 5 + while received.bytesize < payload.bytesize && Time.now < deadline + begin + chunk = ssl.read_nonblock(1024) + received << chunk + rescue OpenSSL::SSL::SSLErrorWaitReadable + IO.select([ssl], nil, nil, 1) + end + end + + assert_equal payload.bytesize, received.bytesize + assert_equal payload, received + ssl.close + end + end + + def test_multi_chunk_read_nonblock_tls12 + large = "A" * 1024 + "\n" + total_lines = 30 + payload = large * total_lines + + ctx_proc = Proc.new { |ctx| ctx.max_version = OpenSSL::SSL::TLS1_2_VERSION } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + assert_equal "TLSv1.2", ssl.ssl_version + + ssl.write(payload) + + received = +"" + deadline = Time.now + 5 + while received.bytesize < payload.bytesize && Time.now < deadline + begin + chunk = ssl.read_nonblock(1024) + received << chunk + rescue OpenSSL::SSL::SSLErrorWaitReadable + IO.select([ssl], nil, nil, 1) + end + end + + assert_equal payload.bytesize, received.bytesize + assert_equal payload, received + ssl.close + end + end + + # ── Buffered I/O: multi-chunk with exception:false ───────────────── + + def test_multi_chunk_read_nonblock_exception_false_tls13 + large = "B" * 1024 + "\n" + total_lines = 30 + payload = large * total_lines + + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + + ssl.write(payload) + + received = +"" + deadline = Time.now + 5 + while received.bytesize < payload.bytesize && Time.now < deadline + result = ssl.read_nonblock(1024, exception: false) + case result + when :wait_readable + IO.select([ssl], nil, nil, 1) + when :wait_writable + IO.select(nil, [ssl], nil, 1) + when String + received << result + end + end + + assert_equal payload.bytesize, received.bytesize + assert_equal payload, received + ssl.close + end + end + + # ── Buffered I/O: partial read_nonblock ──────────────────────────── + # + # Adapted from MRI's test_read_nonblock_without_session pattern. + # Write data, read a small amount (leaves data in appReadData buffer), + # then read the rest. + + def test_partial_read_nonblock_tls13 + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + assert_equal "TLSv1.3", ssl.ssl_version + + ssl.write("hello world\n") + IO.select([ssl], nil, nil, 5) + + # Read just 5 bytes — the rest stays in appReadData buffer + first = nil + 10.times do + begin + first = ssl.read_nonblock(5) + break + rescue OpenSSL::SSL::SSLErrorWaitReadable + IO.select([ssl], nil, nil, 2) + end + end + assert_equal "hello", first + + # Read the rest — should come from the buffer, no network I/O needed + rest = ssl.read_nonblock(100) + assert_equal " world\n", rest + + # Nothing left + result = ssl.read_nonblock(100, exception: false) + assert_equal :wait_readable, result + + ssl.close + end + end + + # ── Buffered I/O: multiple write+read cycles ─────────────────────── + # + # Adapted from MRI's test_parallel pattern (single connection version). + # Verifies the engine state stays clean across many exchanges after + # TLS 1.3 post-handshake processing. + + def test_multiple_write_read_cycles_tls13 + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + assert_equal "TLSv1.3", ssl.ssl_version + + str = "x" * 1000 + "\n" + 10.times do |i| + ssl.puts(str) + response = ssl.gets + assert_equal str, response, "cycle #{i}: data mismatch" + end + + ssl.close + end + end + + def test_multiple_write_read_cycles_tls12 + ctx_proc = Proc.new { |ctx| ctx.max_version = OpenSSL::SSL::TLS1_2_VERSION } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + assert_equal "TLSv1.2", ssl.ssl_version + + str = "x" * 1000 + "\n" + 10.times do |i| + ssl.puts(str) + response = ssl.gets + assert_equal str, response, "cycle #{i}: data mismatch" + end + + ssl.close + end + end + + # ── Buffered I/O: sysread/syswrite round-trip ────────────────────── + # + # Adapted from MRI's test_sysread_and_syswrite: multiple cycles of + # syswrite/sysread with exact byte counts. Exercises the blocking + # sysreadImpl path on TLS 1.3. + + def test_sysread_syswrite_tls13 + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, :ctx_proc => ctx_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + assert_equal "TLSv1.3", ssl.ssl_version + + str = "x" * 100 + "\n" + + # Cycle 1: basic syswrite/sysread + ssl.syswrite(str) + newstr = ssl.sysread(str.bytesize) + assert_equal str, newstr + + # Cycle 2: sysread into a buffer + buf = String.new + ssl.syswrite(str) + assert_same buf, ssl.sysread(str.bytesize, buf) + assert_equal str, buf + + # Cycle 3: another round + ssl.syswrite(str) + assert_equal str, ssl.sysread(str.bytesize) + + ssl.close + end + end + + # ── Buffered I/O: large payload to exercise netReadData leftovers ── + # + # The server writes a large payload in one shot. The client reads + # it in small read_nonblock chunks. When socketChannelImpl().read() + # pulls in multiple TLS records at once, netReadData has leftover + # bytes (position > 0) after the first unwrap. The sysreadImpl loop + # must continue processing (NOT call waitSelect) when netReadData + # still has data. + # + # This is the critical regression test for the + # netReadData.position()==0 guard — it must NOT wait when there's + # still buffered network data. + + def test_large_server_write_small_client_reads_tls13 + # Custom server_proc: read a size header, then send that many bytes + server_proc = Proc.new do |context, ssl| + begin + line = ssl.gets # read the request + if line && line.strip =~ /^SEND (\d+)$/ + size = $1.to_i + data = "Z" * size + "\n" + ssl.write(data) + end + rescue IOError, OpenSSL::SSL::SSLError + ensure + ssl.close rescue nil + end + end + + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, + :ctx_proc => ctx_proc, :server_proc => server_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + assert_equal "TLSv1.3", ssl.ssl_version + + # Ask the server to send 48KB — this will be split across multiple + # TLS records (~16KB each), giving us netReadData with leftover bytes. + expected_size = 48 * 1024 + ssl.puts("SEND #{expected_size}") + + received = +"" + expected_total = expected_size + 1 # +1 for the trailing "\n" + deadline = Time.now + 5 + while received.bytesize < expected_total && Time.now < deadline + result = ssl.read_nonblock(4096, exception: false) + case result + when :wait_readable + IO.select([ssl], nil, nil, 1) + when :wait_writable + IO.select(nil, [ssl], nil, 1) + when String + received << result + end + end + + assert_equal expected_total, received.bytesize, + "Expected #{expected_total} bytes but got #{received.bytesize}" + assert_equal "Z" * expected_size + "\n", received + ssl.close + end + end + + def test_large_server_write_small_client_reads_tls12 + server_proc = Proc.new do |context, ssl| + begin + line = ssl.gets + if line && line.strip =~ /^SEND (\d+)$/ + size = $1.to_i + data = "Z" * size + "\n" + ssl.write(data) + end + rescue IOError, OpenSSL::SSL::SSLError + ensure + ssl.close rescue nil + end + end + + ctx_proc = Proc.new { |ctx| ctx.max_version = OpenSSL::SSL::TLS1_2_VERSION } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, + :ctx_proc => ctx_proc, :server_proc => server_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + assert_equal "TLSv1.2", ssl.ssl_version + + expected_size = 48 * 1024 + ssl.puts("SEND #{expected_size}") + + received = +"" + expected_total = expected_size + 1 + deadline = Time.now + 5 + while received.bytesize < expected_total && Time.now < deadline + result = ssl.read_nonblock(4096, exception: false) + case result + when :wait_readable + IO.select([ssl], nil, nil, 1) + when :wait_writable + IO.select(nil, [ssl], nil, 1) + when String + received << result + end + end + + assert_equal expected_total, received.bytesize + assert_equal "Z" * expected_size + "\n", received + ssl.close + end + end + # ── Wasted iteration detection ───────────────────────────────────── # # TLS 1.3 post-handshake record (NewSessionTicket) that produces 0 app bytes, status is OK From 61760b688e86dd32fb2cc44492d815d10d258cf5 Mon Sep 17 00:00:00 2001 From: kares Date: Thu, 26 Mar 2026 13:24:55 +0100 Subject: [PATCH 4/4] [fix] SSL write data loss on non-blocking partial flush (#242) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit write() unconditionally called netWriteData.clear() after a non-blocking flushData() that may not have flushed all encrypted bytes. This discarded unflushed TLS records, corrupting the encrypted stream and causing 'Broken pipe' or 'Connection reset' errors on subsequent writes — most commonly seen during 'gem push' of large gems over HTTPS. Fix: replace clear() with compact() which preserves unflushed bytes by moving them to the front of the buffer before engine.wrap() appends new encrypted output. Additionally, sysreadImpl() now flushes pending netWriteData before reading. After write_nonblock, encrypted bytes could remain unsent; without flushing first the server would never receive the complete request body (e.g. net/http POST), causing it to time out or reset. --- .../java/org/jruby/ext/openssl/SSLSocket.java | 12 +- src/test/ruby/ssl/test_write_nonblock.rb | 289 ++++++++++++++++++ 2 files changed, 300 insertions(+), 1 deletion(-) create mode 100644 src/test/ruby/ssl/test_write_nonblock.rb diff --git a/src/main/java/org/jruby/ext/openssl/SSLSocket.java b/src/main/java/org/jruby/ext/openssl/SSLSocket.java index 0c87ebe9..55879032 100644 --- a/src/main/java/org/jruby/ext/openssl/SSLSocket.java +++ b/src/main/java/org/jruby/ext/openssl/SSLSocket.java @@ -698,7 +698,9 @@ public int write(ByteBuffer src, boolean blocking) throws SSLException, IOExcept if ( netWriteData.hasRemaining() ) { flushData(blocking); } - netWriteData.clear(); + // use compact() to preserve any encrypted bytes that flushData could not send (non-blocking partial write) + // clear() would discard them, corrupting the TLS record stream: + netWriteData.compact(); final SSLEngineResult result = engine.wrap(src, netWriteData); if ( result.getStatus() == SSLEngineResult.Status.CLOSED ) { throw getRuntime().newIOError("closed SSL engine"); @@ -856,6 +858,14 @@ private IRubyObject sysreadImpl(final ThreadContext context, final IRubyObject l } try { + // Flush any pending encrypted write data before reading. + // After write_nonblock, encrypted bytes may remain in netWriteData that haven't been sent to the server. + // If we read without flushing, the server may not have received the complete request + // (e.g. net/http POST body) and will not send a response. + if ( engine != null && netWriteData.hasRemaining() ) { + flushData(blocking); + } + // So we need to make sure to only block when there is no data left to process if ( engine == null || ! ( appReadData.hasRemaining() || netReadData.position() > 0 ) ) { final Object ex = waitSelect(SelectionKey.OP_READ, blocking, exception); diff --git a/src/test/ruby/ssl/test_write_nonblock.rb b/src/test/ruby/ssl/test_write_nonblock.rb new file mode 100644 index 00000000..bd693b67 --- /dev/null +++ b/src/test/ruby/ssl/test_write_nonblock.rb @@ -0,0 +1,289 @@ +require File.expand_path('test_helper', File.dirname(__FILE__)) + +class TestWriteNonblock < TestCase + + include SSLTestHelper + + # Reproduces the data loss: write a large payload via write_nonblock + # with a slow-reading server (small recv buffer), then read the response. + # The server echoes back the byte count it received. If bytes were lost, + # the count will be less than expected. + def test_write_nonblock_data_integrity + expected_size = 256 * 1024 # 256KB — large enough to overflow TCP buffers + + # Custom server: reads all data until a blank line, counts bytes, sends back the count. + # Deliberately slow: small recv buffer + sleep between reads to create backpressure. + server_proc = Proc.new do |context, ssl| + begin + total = 0 + while (line = ssl.gets) + break if line.strip.empty? + total += line.bytesize + end + ssl.write("RECEIVED #{total}\n") + rescue IOError, OpenSSL::SSL::SSLError => e + # If the TLS stream is corrupted, the server may get an error here + warn "Server error: #{e.class}: #{e.message}" if $VERBOSE + ensure + ssl.close rescue nil + end + end + + [OpenSSL::SSL::TLS1_2_VERSION, OpenSSL::SSL::TLS1_3_VERSION].each do |tls_version| + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = tls_version } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, + :ctx_proc => ctx_proc, :server_proc => server_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + # Small send buffer to increase the chance of partial non-blocking writes + sock.setsockopt(Socket::SOL_SOCKET, Socket::SO_SNDBUF, 4096) + sock.setsockopt(Socket::SOL_SOCKET, Socket::SO_RCVBUF, 4096) + + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + + # Build a large payload: many lines totaling expected_size bytes + line = "X" * 1023 + "\n" # 1024 bytes per line + lines_needed = expected_size / line.bytesize + payload = line * lines_needed + actual_payload_size = payload.bytesize + + # Write it all using write_nonblock (as net/http does) + written = 0 + while written < payload.bytesize + begin + n = ssl.write_nonblock(payload.byteslice(written, payload.bytesize - written)) + written += n + rescue IO::WaitWritable, OpenSSL::SSL::SSLErrorWaitWritable + IO.select(nil, [ssl], nil, 5) + retry + end + end + + # Send terminator + ssl.write("\n") + + # Read the response (this is where the flush-before-read matters) + response = nil + deadline = Time.now + 10 + while Time.now < deadline + begin + response = ssl.gets + break if response + rescue IO::WaitReadable, OpenSSL::SSL::SSLErrorWaitReadable + IO.select([ssl], nil, nil, 5) + end + end + + assert_not_nil response, "No response from server (TLS #{ssl.ssl_version})" + assert_match(/^RECEIVED (\d+)/, response) + received = response[/RECEIVED (\d+)/, 1].to_i + assert_equal actual_payload_size, received, + "Server received #{received} bytes but we sent #{actual_payload_size} " \ + "(lost #{actual_payload_size - received} bytes) on #{ssl.ssl_version}" + + ssl.close + end + end + end + + # Simpler test: write_nonblock followed by sysread should work. + # This is the net/http pattern: POST body via write, then read response. + def test_write_nonblock_then_sysread + server_proc = Proc.new do |context, ssl| + begin + data = +"" + while (line = ssl.gets) + break if line.strip == "END" + data << line + end + ssl.write("OK:#{data.bytesize}\n") + rescue IOError, OpenSSL::SSL::SSLError + ensure + ssl.close rescue nil + end + end + + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, + :ctx_proc => ctx_proc, :server_proc => server_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + sock.setsockopt(Socket::SOL_SOCKET, Socket::SO_SNDBUF, 4096) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + + # Write via write_nonblock + payload = "Y" * 50_000 + "\n" + written = 0 + while written < payload.bytesize + begin + n = ssl.write_nonblock(payload.byteslice(written, payload.bytesize - written)) + written += n + rescue IO::WaitWritable, OpenSSL::SSL::SSLErrorWaitWritable + IO.select(nil, [ssl], nil, 5) + retry + end + end + ssl.write("END\n") + + # Now read response via sysread (the net/http pattern) + IO.select([ssl], nil, nil, 10) + response = ssl.sysread(1024) + assert_match(/^OK:(\d+)/, response) + received = response[/OK:(\d+)/, 1].to_i + assert_equal payload.bytesize, received, "Server received #{received} bytes but sent #{payload.bytesize}" + + ssl.close + end + end + + # Test that multiple write_nonblock calls preserve all data even under + # buffer pressure (many small writes) + def test_many_small_write_nonblock_calls + server_proc = Proc.new do |context, ssl| + begin + total = 0 + while (line = ssl.gets) + break if line.strip == "DONE" + total += line.bytesize + end + ssl.write("TOTAL:#{total}\n") + rescue IOError, OpenSSL::SSL::SSLError + ensure + ssl.close rescue nil + end + end + + ctx_proc = Proc.new { |ctx| ctx.min_version = ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION } + start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true, + :ctx_proc => ctx_proc, :server_proc => server_proc) do |server, port| + sock = TCPSocket.new("127.0.0.1", port) + sock.setsockopt(Socket::SOL_SOCKET, Socket::SO_SNDBUF, 4096) + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + + # Send 500 small lines rapidly via write_nonblock + line = "Z" * 200 + "\n" + expected_total = 0 + 500.times do + written = 0 + while written < line.bytesize + begin + n = ssl.write_nonblock(line.byteslice(written, line.bytesize - written)) + written += n + rescue IO::WaitWritable, OpenSSL::SSL::SSLErrorWaitWritable + IO.select(nil, [ssl], nil, 5) + retry + end + end + expected_total += line.bytesize + end + ssl.write("DONE\n") + + IO.select([ssl], nil, nil, 10) + response = ssl.gets + assert_not_nil response + received = response[/TOTAL:(\d+)/, 1].to_i + assert_equal expected_total, received, "Server received #{received} bytes but sent #{expected_total}" + + ssl.close + end + end + + # Detect the netWriteData.clear() bug by invoking the Java write() directly, bypassing syswriteImpl's `waitSelect`. + # + # Reproducer for #242 (jruby/jruby#8935). + # + # Strategy: + # 1. Saturate the TCP send buffer (server doesn't read) + # 2. Call write(ByteBuffer, false) directly via Java reflection + # 3. Check netWriteData.remaining() — if > 0, data would be discarded by the next write() call's netWriteData.clear() + def test_internal_write_nonblock_unflushed_data_detected + require 'socket' + + tcp_server = TCPServer.new("127.0.0.1", 0) + port = tcp_server.local_address.ip_port + + server_ctx = OpenSSL::SSL::SSLContext.new + server_ctx.cert = @svr_cert + server_ctx.key = @svr_key + server_ctx.min_version = server_ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION + + ssl_server = OpenSSL::SSL::SSLServer.new(tcp_server, server_ctx) + ssl_server.start_immediately = true + + server_thread = Thread.new do + Thread.current.report_on_exception = false + begin + ssl_conn = ssl_server.accept + sleep 30 # Do NOT read — maximize backpressure + ssl_conn.close rescue nil + rescue + end + end + + begin + sock = TCPSocket.new("127.0.0.1", port) + sock.setsockopt(Socket::SOL_SOCKET, Socket::SO_SNDBUF, 4096) + sock.setsockopt(Socket::SOL_SOCKET, Socket::SO_RCVBUF, 4096) + + ssl = OpenSSL::SSL::SSLSocket.new(sock) + ssl.sync_close = true + ssl.connect + + java_cls = Java::OrgJrubyExtOpenssl::SSLSocket.java_class + java_ssl = ssl.to_java(Java::OrgJrubyExtOpenssl::SSLSocket) + + nwd_field = java_cls.declared_field("netWriteData") + nwd_field.accessible = true + # Get the write(ByteBuffer, boolean) method via reflection + write_method = java_cls.declared_method("write", java.nio.ByteBuffer.java_class, Java::boolean) + write_method.accessible = true + + # Phase 1: fill the TCP send buffer via normal write_nonblock + chunk = "H" * 16384 + 100.times do + begin + ssl.write_nonblock(chunk) + rescue IO::WaitWritable, OpenSSL::SSL::SSLErrorWaitWritable + break + rescue IOError, OpenSSL::SSL::SSLError + break + end + end + + # Phase 2: call write(src, false) directly — this bypasses + # syswriteImpl's waitSelect and goes straight to the code path + # that has the clear() bug. + src = java.nio.ByteBuffer.wrap(("I" * 4096).to_java_bytes) + begin + write_method.invoke(java_ssl, src, false) + rescue Java::JavaLangReflect::InvocationTargetException => e + warn "write() threw: #{e.cause}" if $VERBOSE # Expected — write may throw due to the saturated buffer + end + + nwd = nwd_field.value(java_ssl) + remaining = nwd.remaining + + if remaining > 0 + # BUG CONFIRMED: there are unflushed encrypted bytes in netWriteData. + # The next write() call will do netWriteData.clear(), discarding them. + # This is exactly the data loss bug from issue #242. + # + # To prove actual data loss, we would call write() again — the clear() would discard remaining encrypted bytes, + # corrupting the TLS record stream and eventually causing the server to see fewer bytes than the client sent. + assert remaining > 0, "netWriteData has #{remaining} unflushed bytes — next write() would discard them via clear()" + else + omit "Could not produce unflushed netWriteData in loopback (remaining=#{remaining}); bug requires network latency" + end + ensure + ssl.close rescue nil + sock.close rescue nil + tcp_server.close rescue nil + server_thread.kill rescue nil + server_thread.join(2) rescue nil + end + end if defined?(JRUBY_VERSION) +end