Fix SSL write data loss and flush-before-read on non-blocking writes#347
Fix SSL write data loss and flush-before-read on non-blocking writes#347jsvd wants to merge 1 commit intojruby:masterfrom
Conversation
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.
kares
left a comment
kares
left a comment
There was a problem hiding this comment.
Noticed this late, seems like its just Opus wout a human touch?
What would be interesting to know is whether gem push got actually resolved after these or any other context you can share here related to the fix (for me the gem push issue never reproduced with Linux' defaults).
| // Flush any pending encrypted write data before reading, so the | ||
| // remote side receives the complete request before we wait for a response. | ||
| if ( engine != null && netWriteData.hasRemaining() ) { | ||
| flushData(true); |
There was a problem hiding this comment.
btw. was this actually relevant in context of #242?
also any thoughts about forcing a blocking flushData or the comment above in general (doing a write in context of user requesting a non-blocking read)?
Definitely Opus, though I hoped there was enough of a human touch as I spend quite a bit of time experimenting and doing back and forths to arrive at this PR😅 For context, these changes allowed me to publish https://rubygems.org/gems/jrjackson/versions/0.4.21-java. Otherwise I would consistently get the broken pipe errors reported in multiple issues in jruby and jruby-openssl. Before I could just set the ssl debug flag and it seemed to introduced enough of a delay to avoid the broken pipe but now the flags weren't helping anymore, and I had to find a solution. compact is definitely the fix, the other one was a defensive measure for non blocking writes, but likely not necessary (I'm not familiar with the codebase at all). [edit] this was all on macOS. I haven't tested pushing a > 2MB gem on linux |
Apologies, it's really hard to tell these days. 😅 What you said in your last comment is really valuable to know. 💟 Appreciate the response, I'll take a look at the test once I am on #351.. |
3 participants
Two bugs in SSLSocket cause data loss when using write_nonblock followed by sysread (the pattern used by net/http for POST requests):
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.
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 pushon JRuby 10 with large gem files: the server never received the complete POST body and sent close_notify instead of an HTTP response.fixes #242