Skip to content

SslClientHelloHandler.decode() retries with select(ctx, null) when sync onLookupComplete throws #16790

@kwondh5217

Description

@kwondh5217

How this was discovered

I was diagnosing an integration test that was expected to fail (the SNI
hostname was not mapped) but kept passing instead. Tracing through, the
lookup callback was throwing on the unmapped hostname, but the exception
never reached exceptionCaught(...)SslClientHelloHandler.decode()
caught it in its outer catch (Exception) block and retried via
select(ctx, null). The default fallback via select(ctx, null) allowed the handshake
to complete; the test passed when it should have failed.

What I observed

When lookup(...) returns an already-completed Future (sync path) and
onLookupComplete(...) throws an exception:

  • select(ctx, null) is invoked as a retry, allowing the handshake to fall through to the default SslContext path.
  • exceptionCaught(...) is never invoked on the pipeline.

When the same callback is exercised on the async path (an unresolved
Future that is completed later), the exception is correctly routed via
ctx.fireExceptionCaught(...) and no select(ctx, null) retry occurs.

So the same user callback, with the same exception, produces opposite
outcomes depending on whether the user's Promise is completed synchronously
or asynchronously.

Where in the code

handler/src/main/java/io/netty/handler/ssl/SslClientHelloHandler.java:

// decode(), ~L196-202 — outer catch in decode()
} catch (Exception e) {
    if (logger.isDebugEnabled()) {
        logger.debug("Unexpected client hello packet: " + ByteBufUtil.hexDump(in), e);
    }
    select(ctx, null);
}

// select(), ~L217-255
private void select(final ChannelHandlerContext ctx, ByteBuf clientHello) throws Exception {
    final Future<T> future;
    try {
        future = lookup(ctx, clientHello);
        if (future.isDone()) {
            onLookupComplete(ctx, future);              // ← sync path: no try/catch
        } else {
            future.addListener((FutureListener<T>) future1 -> {
                releaseIfNotNull(finalClientHello);
                try {
                    suppressRead = false;
                    try {
                        onLookupComplete(ctx, future1);
                    } catch (DecoderException err) {
                        ctx.fireExceptionCaught(err);   // ← async path: routes to pipeline
                    } catch (Exception cause) {
                        ctx.fireExceptionCaught(new DecoderException(cause));
                    } catch (Throwable cause) {
                        ctx.fireExceptionCaught(cause);
                    }
                } finally { /* readPending */ }
            });
        }
    } catch (Throwable cause) {
        PlatformDependent.throwException(cause);
    } finally {
        releaseIfNotNull(clientHello);
    }
}

On the sync branch, the exception thrown by onLookupComplete propagates up
through select()'s outer catch (Throwable) (re-thrown via
PlatformDependent.throwException) and ends up caught by decode()'s
catch (Exception e), which then calls select(ctx, null).

Suggested fix

Apply the same inner routing to the sync branch that the async branch already
has:

         try {
             future = lookup(ctx, clientHello);
             if (future.isDone()) {
-                onLookupComplete(ctx, future);
+                try {
+                    onLookupComplete(ctx, future);
+                } catch (DecoderException err) {
+                    ctx.fireExceptionCaught(err);
+                } catch (Exception cause) {
+                    ctx.fireExceptionCaught(new DecoderException(cause));
+                } catch (Throwable cause) {
+                    ctx.fireExceptionCaught(cause);
+                }
             } else {
                 suppressRead = true;

With this change the sync path no longer performs the select(ctx, null) retry on user-callback exceptions; instead, the exception is routed via exceptionCaught(...) like on the async path.

I'm willing to submit a PR if this is accepted.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions