RFR JDK-8214129: SSL session resumption/SNI with TLS1.2 causes StackOverflowError

[Jamil Nimeh]

I will change those additional spots in the code.  Glad you caught 
those.  I think also your suggestion about a comment on those locations 
in the code makes sense.

--Jamil

On 12/5/2018 8:20 PM, Xue-Lei Fan wrote:
> Hi Jamil,
>
> For a defense in depth fix, as you are already there, I may suggest 
> update two more places.
>
> ServerNameExtension.java:
> ------------------------
>   private CHServerNamesSpec(List<SNIServerName> serverNames) {
>       this.serverNames =
> Collections.<SNIServerName>unmodifiableList(serverNames);
>   }
>
> SSLSessionImpl.java:
> --------------------
> this.localSupportedSignAlgs = ... // two places in the constructors
>
>
> The coding style of the use of unmodifiableList() looks a little bit 
> weird for a code reader who does not familar with the limitation of 
> unmodifiableList() method.  What do you think if adding a comment 
> about why we code this way?  Just in case, someone may rollback it later.
>
> Otherwise, looks fine to me.
>
> Thanks,
> Xuelei
>
>
> On 12/5/2018 3:59 PM, Jamil Nimeh wrote:
>> Hello all,
>>
>> This fix covers an issue where large numbers of TLS 1.2 session 
>> resumptions were causing a StackOverflowError to occur.  This was 
>> happening because the SSLSessionImpl constructor creates a new 
>> unmodifiableList from the SNI list attached to the HandshakeContext. 
>> Since that is also an unmodifiableList, you get a new level of 
>> nesting of lists with each successive instantiation of 
>> SSLSessionImpl. Eventually it grows to the point that an iteration of 
>> the list causes a stack overflow.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8214129
>>
>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8214129/webrev.01/
>>
>> Thanks,
>>
>> --Jamil
>>