Skip to content

Commit eec07d0

Browse files
committed
Fix SSL write data loss and flush-before-read on non-blocking writes
Two bugs in SSLSocket cause data loss when using write_nonblock followed by sysread (the pattern used by net/http for POST requests): 1. write() unconditionally calls netWriteData.clear() after a non-blocking flushData(), discarding any encrypted bytes that weren't yet sent to the socket. Fix: use compact() which preserves unsent bytes. 2. sysreadImpl() never flushes pending netWriteData before reading. After the last write_nonblock, encrypted data can remain in the buffer and never reach the server. Fix: flush netWriteData at the start of sysreadImpl(). This was observed as EOFError during `gem push` on JRuby 10 with large gem files: the server never received the complete POST body and sent close_notify instead of an HTTP response.
1 parent a4b2a1a commit eec07d0

File tree

2 files changed

+88
-1
lines changed

2 files changed

+88
-1
lines changed

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -688,7 +688,7 @@ public int write(ByteBuffer src, boolean blocking) throws SSLException, IOExcept
688688
if ( netWriteData.hasRemaining() ) {
689689
flushData(blocking);
690690
}
691-
netWriteData.clear();
691+
netWriteData.compact();
692692
final SSLEngineResult result = engine.wrap(src, netWriteData);
693693
if ( result.getStatus() == SSLEngineResult.Status.CLOSED ) {
694694
throw getRuntime().newIOError("closed SSL engine");
@@ -830,6 +830,12 @@ private IRubyObject sysreadImpl(final ThreadContext context, final IRubyObject l
830830
}
831831

832832
try {
833+
// Flush any pending encrypted write data before reading, so the
834+
// remote side receives the complete request before we wait for a response.
835+
if ( engine != null && netWriteData.hasRemaining() ) {
836+
flushData(true);
837+
}
838+
833839
// So we need to make sure to only block when there is no data left to process
834840
if ( engine == null || ! ( appReadData.hasRemaining() || netReadData.position() > 0 ) ) {
835841
final Object ex = waitSelect(SelectionKey.OP_READ, blocking, exception);
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
# frozen_string_literal: false
2+
require File.expand_path('test_helper', File.dirname(__FILE__))
3+
4+
class TestSSLWriteFlush < TestCase
5+
6+
include SSLTestHelper
7+
8+
# write_nonblock a large payload then read the server's response.
9+
#
10+
# This exercises the write -> read transition used by net/http for POST
11+
# requests. Two bugs in SSLSocket caused data loss here:
12+
# 1. write() called netWriteData.clear() after a partial non-blocking
13+
# flush, discarding encrypted bytes not yet sent to the socket.
14+
# 2. sysreadImpl() did not flush pending netWriteData before reading.
15+
#
16+
# Without the fix the TLS stream is corrupted and the connection breaks
17+
# with EPIPE.
18+
def test_write_nonblock_then_read
19+
data_size = 256 * 1024
20+
data = "X" * data_size
21+
22+
server_proc = proc { |ctx, ssl|
23+
begin
24+
received = ""
25+
begin
26+
while received.bytesize < data_size
27+
received << ssl.readpartial(8192)
28+
end
29+
rescue EOFError
30+
end
31+
ssl.write("GOT:#{received.bytesize}")
32+
ensure
33+
ssl.close rescue nil
34+
end
35+
}
36+
37+
start_server0(PORT, OpenSSL::SSL::VERIFY_NONE, true,
38+
server_proc: server_proc) do |server, port|
39+
sock = TCPSocket.new("127.0.0.1", port)
40+
sock.setsockopt(Socket::SOL_SOCKET, Socket::SO_SNDBUF, 2048)
41+
ssl = OpenSSL::SSL::SSLSocket.new(sock)
42+
ssl.connect
43+
ssl.sync_close = true
44+
45+
remaining = data
46+
while remaining.bytesize > 0
47+
begin
48+
written = ssl.write_nonblock(remaining)
49+
remaining = remaining.byteslice(written..-1)
50+
rescue IO::WaitWritable
51+
IO.select(nil, [ssl])
52+
retry
53+
end
54+
end
55+
56+
response = ""
57+
deadline = Time.now + 30
58+
loop do
59+
remaining_time = deadline - Time.now
60+
break if remaining_time <= 0
61+
if IO.select([ssl], nil, nil, [remaining_time, 1].min)
62+
begin
63+
chunk = ssl.read_nonblock(16384, exception: false)
64+
case chunk
65+
when :wait_readable then next
66+
when nil then break
67+
else response << chunk
68+
end
69+
rescue EOFError
70+
break
71+
end
72+
end
73+
end
74+
75+
assert_match(/^GOT:#{data_size}$/, response,
76+
"Server should receive all #{data_size} bytes")
77+
ssl.close
78+
end
79+
end
80+
81+
end

0 commit comments

Comments
 (0)