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
