Skip to content

Commit 44432d4

Browse files
committed
[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
1 parent 6418dad commit 44432d4

File tree

2 files changed

+78
-26
lines changed

2 files changed

+78
-26
lines changed

src/main/java/org/jruby/ext/openssl/SSLSocket.java

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -893,11 +893,14 @@ private IRubyObject sysreadImpl(final ThreadContext context, final IRubyObject l
893893
return wouldBlockSymbol(read);
894894
}
895895

896-
if ( read == 0 && status == SSLEngineResult.Status.BUFFER_UNDERFLOW ) {
897-
// If we didn't get any data back because we only read in a partial TLS record,
898-
// instead of spinning until the rest comes in, call waitSelect to either block
899-
// until the rest is available, or throw a "read would block" error if we are in
900-
// non-blocking mode.
896+
if ( read == 0 && netReadData.position() == 0 ) {
897+
// If we didn't get any data back and there is no buffered network data left to process,
898+
// wait for more data from the network instead of spinning until it arrives.
899+
// In blocking mode this blocks; in non-blocking mode it raises/returns "read would block".
900+
//
901+
// We check netReadData.position() rather than status == BUFFER_UNDERFLOW because readAndUnwrap
902+
// may have successfully consumed a non-application record (e.g. a TLS 1.3 NewSessionTicket)
903+
// leaving status == OK with zero app bytes produced and nothing left in the network buffer.
901904
final Object ex = waitSelect(SelectionKey.OP_READ, blocking, exception);
902905
if ( ex instanceof IRubyObject ) return (IRubyObject) ex; // :wait_readable
903906
}

src/test/ruby/ssl/test_read_nonblock_tls13.rb

Lines changed: 70 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,5 @@
11
# frozen_string_literal: false
2-
# Regression tests for:
3-
# https://github.com/jruby/jruby-openssl/issues/271
4-
# https://github.com/jruby/jruby-openssl/issues/305
5-
# https://github.com/jruby/jruby-openssl/issues/317
6-
#
7-
# Root cause: readAndUnwrap() in SSLSocket.java called doHandshake(blocking)
8-
# (the 1-arg overload that hardcodes exception=true) when processing TLS 1.3
9-
# post-handshake records (NewSessionTicket). When selectNow()==0 inside that
10-
# doHandshake, it threw SSLErrorWaitReadable even when the caller passed
11-
# exception:false, violating the non-blocking contract.
12-
#
13-
# Fix: readAndUnwrap now accepts and forwards the exception flag to
14-
# doHandshake(blocking, exception), and returns a WOULD_BLOCK sentinel
15-
# that propagates back through read() and sysreadImpl() as :wait_readable.
16-
#
2+
173
require File.expand_path('test_helper', File.dirname(__FILE__))
184

195
class TestReadNonblockTLS13 < TestCase
@@ -26,8 +12,7 @@ class TestReadNonblockTLS13 < TestCase
2612
# client's send buffer saturates. Yields |ssl, port| to the block.
2713
# This forces selectNow()==0 inside doHandshake when processing
2814
# TLS 1.3 post-handshake records.
29-
def with_saturated_tls13_client
30-
require 'socket'
15+
def with_saturated_tls13_client; require 'socket'
3116

3217
tcp_server = TCPServer.new("127.0.0.1", 0)
3318
port = tcp_server.local_address.ip_port
@@ -88,8 +73,7 @@ def with_saturated_tls13_client
8873
end
8974

9075
# Same as above but for TLS 1.2 (control)
91-
def with_saturated_tls12_client
92-
require 'socket'
76+
def with_saturated_tls12_client; require 'socket'
9377

9478
tcp_server = TCPServer.new("127.0.0.1", 0)
9579
port = tcp_server.local_address.ip_port
@@ -392,8 +376,7 @@ def test_read_nonblock_with_connect_nonblock_tls13
392376
end
393377

394378
# connect_nonblock + saturated buffer + read_nonblock
395-
def test_read_nonblock_connect_nonblock_saturated_tls13
396-
require 'socket'
379+
def test_read_nonblock_connect_nonblock_saturated_tls13; require 'socket'
397380

398381
tcp_server = TCPServer.new("127.0.0.1", 0)
399382
port = tcp_server.local_address.ip_port
@@ -502,4 +485,70 @@ def test_read_nonblock_tls13_concurrent_stress
502485
assert collected.empty?, "Got #{collected.size} exception leaks:\n#{collected.first(5).join("\n")}"
503486
end
504487

488+
# ── Wasted iteration detection ─────────────────────────────────────
489+
#
490+
# TLS 1.3 post-handshake record (NewSessionTicket) that produces 0 app bytes, status is OK
491+
#
492+
# We detect this by inspecting the internal `status` field after read_nonblock returns :wait_readable.
493+
# If status is BUFFER_UNDERFLOW, the extra iteration occurred. If status is OK, sysreadImpl handled
494+
# the read==0/status==OK case directly.
495+
def test_internal_no_wasted_readAndUnwrap_iteration_tls13; require 'socket'
496+
497+
tcp_server = TCPServer.new("127.0.0.1", 0)
498+
port = tcp_server.local_address.ip_port
499+
500+
server_ctx = OpenSSL::SSL::SSLContext.new
501+
server_ctx.cert = @svr_cert
502+
server_ctx.key = @svr_key
503+
server_ctx.ssl_version = "TLSv1_3"
504+
505+
ssl_server = OpenSSL::SSL::SSLServer.new(tcp_server, server_ctx)
506+
ssl_server.start_immediately = true
507+
508+
server_thread = Thread.new do
509+
Thread.current.report_on_exception = false
510+
begin
511+
conn = ssl_server.accept
512+
sleep 5
513+
conn.close rescue nil
514+
rescue
515+
end
516+
end
517+
518+
begin
519+
sock = TCPSocket.new("127.0.0.1", port)
520+
ssl = OpenSSL::SSL::SSLSocket.new(sock)
521+
ssl.sync_close = true
522+
ssl.connect
523+
assert_equal "TLSv1.3", ssl.ssl_version
524+
525+
# Wait for the server's NewSessionTicket to arrive on the wire
526+
# after the blocking connect has finished.
527+
sleep 0.1
528+
529+
# Access the private `status` field via Java reflection
530+
java_cls = Java::OrgJrubyExtOpenssl::SSLSocket.java_class
531+
status_field = java_cls.declared_field("status")
532+
status_field.accessible = true
533+
java_ssl = ssl.to_java(Java::OrgJrubyExtOpenssl::SSLSocket)
534+
535+
result = ssl.read_nonblock(1024, exception: false)
536+
assert_equal :wait_readable, result
537+
538+
status_after = status_field.value(java_ssl).to_s
539+
# If sysreadImpl properly handles read==0 with any status (not just BUFFER_UNDERFLOW),
540+
# only one readAndUnwrap call is made and status stays OK.
541+
assert_equal "OK", status_after,
542+
"Expected status OK (single readAndUnwrap call) but got #{status_after} " \
543+
"(extra wasted iteration through readAndUnwrap occurred)"
544+
545+
ssl.close
546+
ensure
547+
ssl.close rescue nil
548+
sock.close rescue nil
549+
tcp_server.close rescue nil
550+
server_thread.kill rescue nil
551+
server_thread.join(2) rescue nil
552+
end
553+
end if defined?(JRUBY_VERSION)
505554
end

0 commit comments

Comments
 (0)