Skip to content

fix(ztls): move handshake to goroutine to prevent indefinite hang#966

Open
allornothingai wants to merge 2 commits intoprojectdiscovery:mainfrom
allornothingai:fix/ztls-handshake-hang
Open

fix(ztls): move handshake to goroutine to prevent indefinite hang#966
allornothingai wants to merge 2 commits intoprojectdiscovery:mainfrom
allornothingai:fix/ztls-handshake-hang

Conversation

@allornothingai
Copy link

@allornothingai allornothingai commented Mar 19, 2026

This PR fixes an indefinite hang in the ztls client.

Root Cause: The method was being called synchronously within a case. In Go, case expressions are evaluated before the select blocks, meaning the handshake was executed on the main goroutine, effectively bypassing the context timeout if the network connection hung.

Fix: Moved the call to a separate goroutine and used the block to wait for either the result or the context cancellation.

Payout Wallet (EVM): 0x0c67cbE9e30c66267975eE2D74Eb88036CA65e9F
Payout Wallet (SOL): 3hLpMvUUS685bZz2PzR6vWeA37UrdKq924s7yLMB47eZ

Summary by CodeRabbit

  • Bug Fixes

    • Improved TLS handshake timeout reporting to include underlying context errors for clearer failure diagnostics.
    • Preserved special-case certificate-only handling during handshakes.
  • Refactor

    • Streamlined TLS handshake execution to run once in the background, improving reliability and simplifying timeout behavior.

@neo-by-projectdiscovery-dev
Copy link

neo-by-projectdiscovery-dev bot commented Mar 19, 2026

Neo - PR Security Review

No security issues found

Highlights

  • Incremental update refines the ztls handshake timeout implementation
  • Changes are limited to 3 lines in pkg/tlsx/ztls/ztls.go
  • No exploitable security issues introduced in this commit

Comment @pdneo help for available commands. · Open in Neo

@coderabbitai
Copy link

coderabbitai bot commented Mar 19, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7a208228-6e93-49e5-a060-de93920e09e4

📥 Commits

Reviewing files that changed from the base of the PR and between 80d1951 and 0b844ed.

📒 Files selected for processing (1)
  • pkg/tlsx/ztls/ztls.go
✅ Files skipped from review due to trivial changes (1)
  • pkg/tlsx/ztls/ztls.go

Walkthrough

Changed tlsHandshakeWithTimeout to accept (ctx context.Context, tlsConn *tls.Conn), run tlsConn.Handshake() exactly once inside a goroutine, and propagate its single result via a channel. On ctx.Done() the returned error now wraps ctx.Err() (tagged); tls.ErrCertsOnly is still converted to nil.

Changes

Cohort / File(s) Summary
TLS handshake behavior
pkg/tlsx/ztls/ztls.go
Reordered tlsHandshakeWithTimeout parameters; execute tlsConn.Handshake() only once in a goroutine and send result on errChan. Removed prior synchronous-call branch. On context cancellation return a tagged error that wraps ctx.Err(); still treat tls.ErrCertsOnly as nil.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Function as tlsHandshakeWithTimeout
  participant Goroutine as handshakeGoroutine
  participant TLS as tls.Conn
  participant Context as ctx

  Caller->>Function: call(ctx, tlsConn)
  Function->>Goroutine: start goroutine -> Handshake()
  Goroutine->>TLS: Handshake()
  TLS-->>Goroutine: err (or nil / tls.ErrCertsOnly)
  Goroutine-->>Function: send err on errChan
  alt ctx cancels before err
    Context-->>Function: ctx.Done()
    Function-->>Caller: return tagged Wrap(ctx.Err())
  else err received first
    Function-->>Caller: receive err, convert tls.ErrCertsOnly -> nil, return err
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A single goroutine hops in line,
One handshake done, no double-time.
If timeout taps, the context speaks,
Errors wrapped with gentler tweaks.
Rabbity code — neat, small, and fine. 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(ztls): move handshake to goroutine to prevent indefinite hang' accurately captures the main change - moving the TLS handshake to a goroutine to fix an indefinite hang issue.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.

Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
pkg/tlsx/ztls/ztls.go (1)

324-324: Consider placing ctx as the first parameter.

Go convention is to pass context.Context as the first parameter of a function. This is a minor style nit.

-func (c *Client) tlsHandshakeWithTimeout(tlsConn *tls.Conn, ctx context.Context) error {
+func (c *Client) tlsHandshakeWithTimeout(ctx context.Context, tlsConn *tls.Conn) error {

If you accept this change, also update the call sites at lines 143 and 260.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/tlsx/ztls/ztls.go` at line 324, Change the method signature to accept
context.Context as the first parameter: rename func (c *Client)
tlsHandshakeWithTimeout(tlsConn *tls.Conn, ctx context.Context) error to func (c
*Client) tlsHandshakeWithTimeout(ctx context.Context, tlsConn *tls.Conn) error,
and update all call sites to pass the ctx first (replace calls like
tlsHandshakeWithTimeout(tlsConn, ctx) with tlsHandshakeWithTimeout(ctx,
tlsConn)); ensure imports and any references (tlsHandshakeWithTimeout, Client)
compile after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pkg/tlsx/ztls/ztls.go`:
- Line 324: Change the method signature to accept context.Context as the first
parameter: rename func (c *Client) tlsHandshakeWithTimeout(tlsConn *tls.Conn,
ctx context.Context) error to func (c *Client) tlsHandshakeWithTimeout(ctx
context.Context, tlsConn *tls.Conn) error, and update all call sites to pass the
ctx first (replace calls like tlsHandshakeWithTimeout(tlsConn, ctx) with
tlsHandshakeWithTimeout(ctx, tlsConn)); ensure imports and any references
(tlsHandshakeWithTimeout, Client) compile after the change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d0c210f8-2cce-423d-8af1-b8520925c862

📥 Commits

Reviewing files that changed from the base of the PR and between d13b67f and 80d1951.

📒 Files selected for processing (1)
  • pkg/tlsx/ztls/ztls.go

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.

1 participant