[8u] RFR 8209333: Socket reset issue for TLS 1.3 socket close

[Alvarez, David]

Hi,

What Volker describes is exactly what is going on. When I first
backported this issue, I did the same as Andrew suggests, just adding a
finally with the close statement. However, after several works, we have
tracked the issues we were having with SSLException to slight
differences in the exceptions thrown by the TLS1.3 stack compared to
TLS1.2 ([1], another patch that I will backport to 8 once the prereqs
are done). For that reason, I decided to ensure there were no
differences introduced in this backport.

Using the @SuppressWarnings("try") is not the only option, though. We
can replace the try-with resources with a functionally equivalent
block, the closest I know one being defined in this article [2].
However, seeing that the @SuppressWarnings("try") was already in use in
other areas, I decided the extra code would be more confusing.

Thanks,

David
---
[1] https://github.com/openjdk/jdk/pull/1968
[2] https://www.oracle.com/technical-resources/articles/java/trywithresources.html#nb2


On Mon, 2021-01-11 at 12:27 +0100, Volker Simonis wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> Hi everybody,
> 
> I think that David's downport is correct and there's nothing much we
> can do about the warning except suppressing it.
> 
> "try-with resources" was introduced in Java 7 and allowed the
> declaration of resources in the try clause [1].
> 
> Later, in Java 9, "try-with resources" was extended to also accept
> already existing resources as final or "effectively final" variables
> in the try statement [2].
> 
> The initial change uses such an "effectively final" resource (i.e.
> "conContext.inputRecord"):
> 
> try (conContext.inputRecord) {
>     appInput.deplete();
> }
> 
> Unfortunately that exact version can't be downported as is because
> that syntax is simply not supported in Java 8. So we have to declare
> a
> new variable in the try statement as David did:
> 
> try (InputRecord ir = conContext.inputRecord) {
>     appInput.deplete();
> }
> 
> But now javac complains that "ir" is not referenced in body of
> corresponding try statement (which is true, but we know that it
> exists
> and we know that it must be closed if an exception occurs). The
> "SuppressWarnings("try")" annotation exist for this exact scenario.
> Notice that this annotations is already used in the jdk8
> class-library, e.g. in BufferedWriter.java [3] or
> FilterOutputStream.java[4] in similar situations. In Java 9 and
> later,
> javac doesn't emits this warning for final or "effectively final"
> variables which are declared in the try statement but not used in the
> try body.
> 
> Using Andrew H's proposal would certainly work as well, but from my
> point of view it would be a bigger difference compared to the
> original
> change so I'd vote for keeping David's version.
> 
> Thank you and best regards,
> Volker
> 
> [1] 
> https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
> [2] 
> https://docs.oracle.com/en/java/javase/15/language/java-language-changes.html#GUID-A920DB06-0FD1-4F9C-8A9A-15FC979D5DA3
> [3] 
> https://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/7432d3558458/src/share/classes/java/io/BufferedWriter.java#l258
> [4] 
> https://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/7432d3558458/src/share/classes/java/io/FilterOutputStream.java#l155
> 
> On Mon, Jan 11, 2021 at 11:28 AM Andrew Haley <aph at redhat.com> wrote:
> > 
> > On 1/11/21 4:08 AM, Andrew Hughes wrote:
> > > On 21:01 Fri 08 Jan     , Alvarez, David wrote:
> > > > Hi,
> > > > 
> > > > I would like a review for 8209333. This is one of the Socket
> > > > related
> > > > patches that were applied to OpenJDK11 after the backport of
> > > > TLS 1.3 to
> > > > OpenJDK8
> > > > 
> > > > BugId: https://bugs.openjdk.java.net/browse/JDK-8209333
> > > > Webrev:
> > > > http://cr.openjdk.java.net/~alvdavi/webrevs/8209333/webrev.8u.jdk.00/
> > > > 
> > > > The only relevant differences are in SSLSocketImpl.java, as it
> > > > contained a java 9 try-with-resources block [1]. A
> > > > @SuppressWarnings("try") was needed.
> > > 
> > > What warning was emitted? Is it not possible to rewrite this so
> > > as to avoid the warning?
> > 
> > Yes, I think this warning is for a real bug. Should be something
> > like:
> > 
> >             if (!conContext.isInboundClosed()) {
> >                 try {
> >                     // Try the best to use up the input records and
> > close the
> >                     // socket gracefully, without impact the
> > performance too
> >                     // much.
> >                     appInput.deplete();
> >                 } finally {
> >                     conContext.inputRecord.close();
> >                 }
> >             }
> > 
> > --
> > Andrew Haley  (he/him)
> > Java Platform Lead Engineer
> > Red Hat UK Ltd. <https://www.redhat.com>
> > https://keybase.io/andrewhaley
> > EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
> >