Ensure alert is send when SSLException happens during calling SslHand…#6047
Ensure alert is send when SSLException happens during calling SslHand…#6047normanmaurer wants to merge 1 commit into
Conversation
|
@Scottmitch @trustin PTAL |
normanmaurer
force-pushed
the
send_alert_when_error_while_unwrap
branch
from
97c9dac to
0fb8111
Compare
| wrap(ctx, false); | ||
| } catch (Throwable cause) { | ||
| // Fail pending writes. | ||
| pendingUnencryptedWrites.removeAndFailAll(cause); |
There was a problem hiding this comment.
can you clarify why we no longer want to fail/release all pending writes in the event of an Throwable (other than SSLException)? Are we sure there will be no unchecked exceptions, and if there is will we potentially leak, or hold onto memory until some other event occurs (user forces a close, etc..)
There was a problem hiding this comment.
Yes there should only be SSLException.
There was a problem hiding this comment.
I'm a bit more pessimistic I guess ... we are calling the JDK's SSL APIs which can be backed by a boat load of complexity ... can you make me more comfortable and explain why none of these APIs will throw an unchecked exception under any condition, or if they do that we will handle it gracefully in some other way.
There was a problem hiding this comment.
I am just not think catching throwable all the time is hacky and inconsistent. If we really do we should at least do it in a consistent way. So you would prefer this?
There was a problem hiding this comment.
@jasontedor had some valid points about not catching throwable everywhere as well, I don't think there is any graceful way of catching OOM and leaving the JVM in good shape.
There was a problem hiding this comment.
@johnou just like it was before PlatformDependent.throwException(cause);.
There was a problem hiding this comment.
totally missed that, was burried under all the comments,
There was a problem hiding this comment.
@normanmaurer - yes I would prefer we cleanup here bcz we own the resources.
There was a problem hiding this comment.
It's done in the following as part of setHandshakeFailure
There was a problem hiding this comment.
looking at your commit now 🍺
|
@johnou @Scottmitch PTAL at the followup commit 3396ada . Good to go now ? |
|
Nope
|
| ctx.flush(); | ||
| } catch (Exception e) { | ||
| notifyHandshakeFailure(e); | ||
| } catch (SSLException e) { |
There was a problem hiding this comment.
either change this to Throwable or add another catch clause and re-throw to ensure we free our resources?
|
PTAL again @Scottmitch |
|
thanks @normanmaurer ship it! |
…ler.unwrap(...) Motivation: When the SslHandler.unwrap(...) (which is called via decode(...)) method did produce an SSLException it was possible that the produced alert was not send to the remote peer. This could lead to staling connections if the remote peer did wait for such an alert and the connection was not closed. Modifications: - Ensure we try to flush any pending data when a SSLException is thrown during unwrapping. - Fix SniHandlerTest to correct test this - Add explicit new test in SslHandlerTest to verify behaviour with all SslProviders. Result: The alert is correctly send to the remote peer in all cases.
normanmaurer
force-pushed
the
send_alert_when_error_while_unwrap
branch
from
602ac48 to
a94c1f5
Compare
3 participants
…ler.unwrap(...)
Motivation:
When the SslHandler.unwrap(...) (which is called via decode(...)) method did produce an SSLException it was possible that the produced alert was not send to the remote peer. This could lead to staling connections if the remote peer did wait for such an alert and the connection was not closed.
Modifications:
Result:
The alert is correctly send to the remote peer in all cases.