Fix | Login TLS handshake hangs on Android (Conscrypt): read up to maxBytes during SSL handshake - #2980
Conversation
…xBytes during SSL handshake issue #2979
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2980 +/- ##
============================================
- Coverage 59.25% 59.15% -0.10%
+ Complexity 5058 5023 -35
============================================
Files 153 153
Lines 36339 36343 +4
Branches 6645 6647 +2
============================================
- Hits 21531 21499 -32
+ Misses 11091 11088 -3
- Partials 3717 3756 +39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The read-up-to change lets the handshake input stream re-enter ensureSSLPayload mid-response when a server handshake message (e.g. a large certificate chain) spans multiple TDS packets. endMessage() then ran with no buffered output and tripped 'assert messageStarted'. Make endMessage() a no-op when nothing is buffered. Verified against a multi-TDS-packet server certificate with assertions enabled on desktop (SunJSSE) and Android (Conscrypt), release and debug.
There was a problem hiding this comment.
Pull request overview
Fixes a TLS login-handshake deadlock affecting Android/Conscrypt by making the driver’s SSL handshake InputStream honor the InputStream.read(..) “read up to N bytes” contract, preventing the driver from blocking while trying to fill an oversized TLS-provider buffer.
Changes:
- Update
SSLHandshakeInputStream.readInternal(..)to read up tomaxBytesbased on currently buffered TDS payload, returning the actual count instead of always blocking for/returningmaxBytes. - Relax
SSLHandshakeOutputStream.endMessage()to no-op when no handshake output message is currently started (avoids assertion failures during multi-packet handshake responses).
Address PR review: readInternal returned min(maxBytes, tdsReader.available()), which could be 0 if ensureSSLPayload() read a zero-length-payload TDS packet. Returning 0 for maxBytes > 0 violates the InputStream.read contract and can make SSL engines busy-spin (our own single-byte read() loops while 0). Loop ensureSSLPayload() until at least one payload byte is buffered when maxBytes > 0. This is bounded: readPacket() blocks for server data and terminates on premature EOF, so it cannot spin. The normal path (payload already present) never iterates.
| // non-zero request, which would violate the InputStream.read contract and can make | ||
| // SSL engines busy-spin. Reading the next packet is bounded: readPacket() blocks | ||
| // for server data and terminates on premature EOF, so this cannot spin. | ||
| while (maxBytes > 0 && 0 == tdsReader.available()) |
There was a problem hiding this comment.
Hopefully there are no side effects on the existing mechanism/behavior for SunJSSE path.
There was a problem hiding this comment.
Oracle and IBM runtime runs showed no regressions. Should be pretty safe.
ADO test runs:
https://sqlclientdrivers.visualstudio.com/mssql-jdbc/_build/results?buildId=160194&view=results
https://sqlclientdrivers.visualstudio.com/mssql-jdbc/_build/results?buildId=161468&view=results
Problem description
On Android (which uses Conscrypt as its default TLS provider), establishing a JDBC connection hangs during the login-phase TLS handshake and eventually fails. SQL Server performs a TLS handshake during login even when
encrypt=false, so bothencrypt=falseandencrypt=trueare affected. In a shipped (assertions-off) build the connect blocks untilloginTimeoutand throwsSQLServerException: ... could not establish a secure connection ... Read timed out. In a debuggable build (where the Android Gradle Plugin / d8 force-enables Java assertions) it fails earlier withAssertionError: numMsgsRcvd:1 should be less than numMsgsSent:1fromTDSReader.readPacket— the same root cause, surfaced earlier by an assertion.Root cause:
TDSChannel.SSLHandshakeInputStream.readInternal(byte[], int, int)read exactlymaxBytes(blocking across TDS packets) and always returnedmaxBytes, instead of reading up tomaxBytesand returning the actual number of bytes read as theInputStream.readcontract requires. SunJSSE reads the TLS handshake in small, exact record-sized chunks and never requests more than is available, so the defect is masked. Conscrypt issues a single large read (its buffer size, e.g. 16709 bytes); the driver then blocks trying to fill 16709 bytes while the server has already sent its complete handshake flight (~1.5 KB in one TDS packet) and is waiting for the client's next flight — a deadlock.Fixes existing GitHub issue
Fixes #2979
Fix Description
In
SSLHandshakeInputStream.readInternal, afterensureSSLPayload()guarantees at least one buffered TDS packet, readMath.min(maxBytes, tdsReader.available())bytes and return the actual count instead of blocking for the fullmaxBytes.TDSReader.available()reports only non-blocking buffered bytes, so the handshake input stream now honors the read-up-to contract and hands each buffered chunk to the SSL engine, which can then advance the handshake. AFINESTlog records when a short read occurs. There is no behavior change for SunJSSE, which already read within a single packet.New Public APIs
None.
Testing
encrypt=falseandencrypt=trueconnect successfully on both the release (assertions-off) and debug (assertions-on) variants.encrypt=falseandencrypt=true, with and without-ea, all continue to connect successfully — no change in the common path.