Skip to content

Fix TlsHandler.FinishWrap null promise masking AuthenticationException#61

Open
maksimkim wants to merge 2 commits into
masterfrom
fix/tls-handler-null-promise-issue-60
Open

Fix TlsHandler.FinishWrap null promise masking AuthenticationException#61
maksimkim wants to merge 2 commits into
masterfrom
fix/tls-handler-null-promise-issue-60

Conversation

@maksimkim
Copy link
Copy Markdown
Owner

Summary

Fixes #60 — When TLS certificate validation fails, a NullReferenceException from Wrap/FinishWrap masks the real AuthenticationException.

Root Cause

Two interrelated bugs combine to replace the original AuthenticationException with a NullReferenceException:

  1. Decode's catch block calls WrapAndFlush in a try/finally with no catch — if WrapAndFlush throws, its exception replaces the original cause.
  2. Wrap method calls _pendingUnencryptedWrites.Remove() without null-checking — Remove() returns null when the queue was drained re-entrantly by HandleFailureRemoveAndFailAll.

Changes

Fix 1: Catch WrapAndFlush exceptions in Decode (TlsHandler.Reader.cs)

Added catch (Exception) { } clause so WrapAndFlush exceptions don't mask the original cause.

Fix 2: Null-check Remove() return in Wrap (TlsHandler.Writer.cs)

Added if (promise is null) { break; } after Remove() to handle the queue being drained externally.

Fix 3: Guard FinishWrap/FinishWrapNonAppData (TlsHandler.Writer.cs)

Added _outboundClosed check at the start of all four overloads to short-circuit writes after the handler enters a failure state.

Fix 4: Protect _sslStream.Dispose() in Close (TlsHandler.cs)

Wrapped _sslStream.Dispose() in try-catch to prevent dispose exceptions from propagating during channel close.

Regression test (TlsHandlerTest.cs)

Added CertValidationRejection_ShouldNotThrowNullReferenceException — configures a TLS client that rejects the server certificate and verifies the TlsHandshakeCompletionEvent contains the real exception, not a NullReferenceException.

Fix four interrelated bugs that cause NullReferenceException to mask the
real AuthenticationException when TLS certificate validation fails:

1. Add catch clause in Decode's error handler so WrapAndFlush exceptions
   don't mask the original cause
2. Null-check Remove() return in Wrap to handle queue drained by
   re-entrant HandleFailure
3. Guard FinishWrap/FinishWrapNonAppData with _outboundClosed check to
   short-circuit writes after failure
4. Wrap _sslStream.Dispose() in Close with try-catch to prevent dispose
   exceptions from propagating

Fixes #60

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@maksimkim maksimkim force-pushed the fix/tls-handler-null-promise-issue-60 branch from 833037a to 088649e Compare April 8, 2026 04:22
Use a custom Stream wrapper (QueueDrainingStreamWrapper) around
MediationStream to simulate the re-entrant HandleFailure scenario:
after SslStream encrypts data and writes ciphertext to MediationStream
(which sets _lastContextWriteTask via FinishWrap), the wrapper drains
the pending write queue and clears _lastContextWriteTask. When Wrap
continues, Remove() returns null and the unfixed code hits
promise.TryComplete() on a null promise, producing NRE.

Verified: test FAILS on all 3 TFMs (net452, net471, netcoreapp2.1)
without Fix 2, and PASSES with Fix 2 applied.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@maksimkim
Copy link
Copy Markdown
Owner Author

/azp run

@maksimkim
Copy link
Copy Markdown
Owner Author

@copilot why ci pipelines were not executed on this PR?

Copy link
Copy Markdown

Copilot AI commented May 14, 2026

@copilot why ci pipelines were not executed on this PR?

I checked the CI config and run history: this repo currently has no regular GitHub Actions CI workflow, and the Azure pipeline YAML (build/pr-validation.yaml) is configured to trigger only for future and release/* branches, not master PRs. Since this PR targets master, CI won’t auto-run unless the branch filters are updated (or a pipeline is manually run with matching configuration).

@maksimkim
Copy link
Copy Markdown
Owner Author

/azp run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants