RFR: 8308144: Uncontrolled memory consumption in SSLFlowDelegate.Reader

[Daniel Fuchs]

On Fri, 17 Nov 2023 04:21:23 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

> Can I please get a review for this change which proposes to fix an issue in the JDK's HttpClient implementation, noted in https://bugs.openjdk.org/browse/JDK-8308144?
> 
> The HttpClient implementation internally uses `java.util.concurrent.Flow.Subscriber` API in the `jdk.internal.net.http.common.SSLFlowDelegate` class. The `SSLFlowDelegate` class at a high level is responsible for taking in SSL network data, decrypting it and passing it along as application data. It's also responsible for accepting application data, encrypting it and passing it along as SSL network data. To do so, it uses the `Flow.Subscriber` API which provide a way to control the pace of the flow of the data.
> 
> The issue at hand is when the `SSLFlowDelegate` receives incoming SSL network data and it tries to decrypt it using `javax.net.ssl.SSLEngine`, the `unwrap` call on that data can return a `BUFFER_UNDERFLOW` status. `BUFFER_UNDERFLOW` implies that the incoming data that the SSLFlowDelegate attempted to decrypt did not have enough bytes to construct a packet out of it. It's then expected that the unwrap call be reattempted by receiving more incoming network data. In its current form, SSLFlowDelegate upon noticing this `BUFFER_UNDERFLOW` status immediately requests more network data so that it can reattempt the decryption and construct application data out of it. This is where the problem resides. When the `BUFFER_UNDERFLOW` state is noticed by SSLFlowDelegate, since the SSLFlowDelegate is flow controlled, it shouldn't eagerly ask for more network data. Instead it should only ask for more network data if the application data subscriber (to whom this decrypted data will be forwarded to 
 by the SSLFlowDelegate), has expressed the desire for any application data. If there's no demand for the application data from the application data subscriber, the SSLFlowDelegate shouldn't demand more network data even if it received a `BUFFER_UNDERFLOW` status when decrypting partial incoming network data. Not honouring the absence of a demand from the application data subscriber implies that the SSLFlowDelegate will ask for more network data whenever it runs into `BUFFER_UNDERFLOW` and will keep accumulating the decrypted application data. Such accumulated data may never be used because of the lack of demand from the application data subscriber, thus unnecessarily increasing the memory consumption of the HttpClient instance(s).
> 
> Credit for identifying, investigating and providing a fix for this issue goes...

Nice fix and nice test. Thanks for that  @zhurs and @jaikiran

src/java.net.http/share/classes/jdk/internal/net/http/common/SSLFlowDelegate.java line 99:

> 97:  *              |   downWriter                          |   downReader
> 98:  *              | supplied to constructor               |  supplied to constructor
> 99:  *              v                                       v

Can we keep the original diagram too? The new diagram is very nice but gives the impression that encrypt/decrypt are completely independent threads of execution and they're not - since consuming some data may probe the engine to request sending of some data.

-------------

PR Review: https://git.openjdk.org/jdk/pull/16704#pullrequestreview-1736703939
PR Review Comment: https://git.openjdk.org/jdk/pull/16704#discussion_r1397129699