RFR: 8308144: Uncontrolled memory consumption in SSLFlowDelegate.Reader [v3]

[Martin Doerr]

On Mon, 20 Nov 2023 14:58:05 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 t...
>
> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   address review comments

This makes sense because there's no workaround and the risk is low.

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

PR Comment: https://git.openjdk.org/jdk/pull/16704#issuecomment-2112164986