TlsHandler.FinishWrap passes a null promise

[dfuenzalida]

TlsHandler.FinishWrap passes a null promise to AbstractChannelHandlerContext.Write when TLS certificate validation fails, masking the real AuthenticationException

[maksimkim]

The exception stacktrace is:

System.NullReferenceException: Object reference not set to an instance of an object.
at DotNetty.Handlers.Tls.TlsHandler.WriteAsync(IByteBuffer buf, IPromise promise)

[maksimkim]

Root Cause Analysis

After tracing through the code, I found two interrelated bugs that combine to mask the real AuthenticationException with a NullReferenceException.

Bug 1: WrapAndFlush exceptions in Decode catch block mask the original exception

In TlsHandler.Reader.cs, the Decode method's catch block (~line 175) calls WrapAndFlush inside a try/finally but with no catch clause:

csharp catch (Exception cause) { try { WrapAndFlush(context); // <-- CAN THROW, no catch! } finally { HandleFailure(cause); } ExceptionDispatchInfo.Capture(cause).Throw(); // <-- NEVER REACHED if WrapAndFlush throws }

If WrapAndFlush throws any exception (e.g., NRE from Bug 2), that exception replaces the original cause (the AuthenticationException). The HandleFailure(cause) in the finally block still runs (properly cleaning up), but ExceptionDispatchInfo.Capture(cause).Throw() is never reached, so the exception that escapes is from WrapAndFlush, not the original TLS error.

Bug 2: Wrap method doesn't null-check Remove() return value

In TlsHandler.Writer.cs, the Wrap method (~line 152):

csharp var promise = _pendingUnencryptedWrites.Remove(); // Can return null! Task task = _lastContextWriteTask; if (task is object) { task.LinkOutcome(promise); // NRE if promise is null } else { _ = promise.TryComplete(); // NRE if promise is null }

BatchingPendingWriteQueue.Remove() returns null when the queue is empty (line 288 of BatchingPendingWriteQueue.cs). The queue can become empty between the Current check (which confirms entries exist) and the Remove() call if HandleFailure is called re-entrantly during buf.ReadBytes(_sslStream, ...). This can happen when:

• During buf.ReadBytes, SslStream encrypts data and writes it via MediationStream → FinishWrap → capturedContext.WriteAsync
• If the downstream write triggers a channel error → ChannelInactive → HandleFailure → RemoveAndFailAll drains the queue
• Control returns to Wrap and Remove() returns null → NRE

Combined Effect (cert validation failure scenario)

1. Unwrap encounters a TLS error (e.g., cert validation failure) → exception thrown
2. Decode's catch block calls WrapAndFlush(context) to send a TLS alert
3. Inside WrapAndFlush → Wrap → Remove() returns null → NRE
4. The NRE from WrapAndFlush propagates (Bug 1), masking the original AuthenticationException
5. User sees NullReferenceException instead of AuthenticationException

Proposed Fixes

Fix 1: Catch WrapAndFlush exceptions in Decode catch block

File: src/DotNetty.Handlers/Tls/TlsHandler.Reader.cs

csharp catch (Exception cause) { try { WrapAndFlush(context); } catch (Exception) { } // Don't mask the original cause finally { HandleFailure(cause); } ExceptionDispatchInfo.Capture(cause).Throw(); }

Fix 2: Null-check Remove() return in Wrap

File: src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs

csharp var promise = _pendingUnencryptedWrites.Remove(); if (promise is null) { break; } // Queue was drained externally

Fix 3: Guard FinishWrap / FinishWrapNonAppData against writes after close

File: src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs

Add _outboundClosed check at the start of all four FinishWrap/FinishWrapNonAppData methods to short-circuit writes after the handler enters a failure state.

Fix 4: Protect _sslStream.Dispose() in Close override

File: src/DotNetty.Handlers/Tls/TlsHandler.cs

Wrap _sslStream.Dispose() in Close with try-catch to prevent dispose exceptions from propagating.

[maksimkim]

Updated Analysis (against current main, post-v1.3.0)

The original analysis above was performed against the master branch. After re-tracing on current main, the originally reported NRE at TlsHandler.WriteAsync(IByteBuffer buf, IPromise promise) has already been fixed on main by PR #55 (released as v1.2.0), and the bug shapes called out in PR #61 do not exist on main — the relevant code has been refactored.

Why the original analysis no longer applies

Between the master branch (where the original analysis was done) and main, TlsHandler and _pendingUnencryptedWrites were rewritten:

• _pendingUnencryptedWrites is no longer BatchingPendingWriteQueue. It is now SslHandlerCoalescingBufferQueue (extends AbstractCoalescingBufferQueue, see TlsHandler.Helper.cs:591). BatchingPendingWriteQueue itself was deleted from the tree in ebb27ab9 (PR Move SimplePromiseAggregator to Http2 codec, remove unused BatchingPendingWriteQueue #58).
• The zero-arg Remove() that returned a possibly-null IPromise no longer exists. The new API is Remove(IByteBufferAllocator alloc, int bytes, IPromise aggregatePromise) and RemoveFirst(IPromise aggregatePromise) — both take a non-null aggregate promise as input and return an IByteBuffer.
• Wrap() (TlsHandler.Writer.cs:150–218) was rewritten. It now creates the promise locally via context.NewPromise() and passes it into Remove, then null-checks the returned buf at line 167 (if (buf is null) { break; }).
• Decode() (TlsHandler.Reader.cs:81–131) was rewritten — its only error path now is HandleUnwrapThrowable(context, cause). HandleUnwrapThrowable (Reader.cs:292–335) already wraps WrapAndFlush in a try/catch that swallows IOException, NotSupportedException, OperationCanceledException, and ArgumentNullException, then re-throws the original cause via ExceptionDispatchInfo.

Mapping the original four "Bugs" to current main

Original bug	Status on current main
Bug 1: Decode's catch has no inner catch for WrapAndFlush	Mitigated. Decode now delegates to HandleUnwrapThrowable, which catches and swallows the four exception types most likely to come out of WrapAndFlush after a handshake failure (importantly, IOException — which is what PR #55's s_sslStreamClosedException is) and re-throws the original cause via ExceptionDispatchInfo.
Bug 2: Wrap() doesn't null-check Remove()'s return	Not applicable. The old Remove() overload is gone. The new Wrap() constructs the aggregate promise locally and null-checks buf at line 167. There is no code path where Wrap would dereference a null promise.
Bug 3: Guard FinishWrap/FinishWrapAsync against null promise after close	Not applicable. All four overloads are only invoked from (a) MediationStream (which uses ?? CapturedContext.VoidPromise() or ?? CapturedContext.NewPromise() — never null) and (b) Wrap's empty-buffer branch (passing the local non-null promise).
Bug 4: Wrap _sslStream.Dispose() in try/catch in Close	Partially in place. HandleFailure (the failure-path cleanup, TlsHandler.cs:300–317) already wraps _sslStream?.Dispose() in try/catch. Close/Disconnect go through CloseOutboundAndChannel (TlsHandler.cs:335–339) which calls _sslStream?.Dispose() without a catch, but this is not on the cert-validation-failure code path.

Where the NRE actually came from on main (pre-v1.2.0) and how PR #55 fixed it

On main between v1.1.0 and v1.2.0, the NRE in WriteAsync happened at await _sslStream.WriteAsync(mem, …) when _sslStream was concurrently set to null by HandleFailure/CloseOutboundAndChannel while a previous async write was still in flight. PR #55 (v1.2.0) fixed this with the standard local-copy + null-check pattern, throwing the pre-allocated IOException("SSLStream closed already") instead of NRE. The thrown IOException is exactly the type that HandleUnwrapThrowable already swallows, so it does not mask the original AuthenticationException carried by cause/TlsHandshakeCompletionEvent.

Cert-validation-failure flow on current main (verified)

1. _sslStream.AuthenticateAsClientAsync(...) task fails with AuthenticationException.
2. HandshakeCompletionCallback schedules HandleHandshakeCompleted on the event loop.
3. HandleHandshakeCompleted (Handshake.cs:179–229):
   • sets State = (oldState | FailedAuthentication) & ~Authenticating,
   • calls _handshakePromise.TrySetException(taskExc),
   • calls TlsUtils.NotifyHandshakeFailure(ctx, cause, true) → user observes TlsHandshakeCompletionEvent with the real AuthenticationException,
   • in finally, calls _pendingUnencryptedWrites?.ReleaseAndFailAll(cause) so any pending writes' promises are completed with the real cause.
4. Any subsequent Write+Flush from upstream goes through Wrap → WriteAsync. _sslStream may now be null (if the channel was closed in response to the handshake event), and PR Fix NullReferenceException in TlsHandler.WriteAsync when SslStream is closed #55's null-check throws IOException("SSLStream closed already") which is propagated to the write promise via OnWriteFailure(ex, promise).

No NullReferenceException is reachable in this flow on current main.

Conclusion

The originally-reported NRE no longer occurs on main. PR #61's fixes target code that has been removed or refactored. No new fix is required on main for issue #60 as originally described.

[maksimkim]

Seems like it is fixed by #55